Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for picture-in-picture playback of videos across multiple pages/screens (close #70) #71

Merged
merged 3 commits into from
Jun 19, 2024

Conversation

matus-tomlein
Copy link
Contributor

@matus-tomlein matus-tomlein commented Jun 13, 2024

Description & motivation

Issue #70

A video played in picture-in-picture (PIP) mode may produce events events with multiple different page view IDs. Currently we assume that a playback belongs to only a single page view ID.

This PR allows for the play to cover multiple page view IDs and add a column in the media base table that lists all of the page view IDs. The existing page_view_id column is kept and refers to the first page view id in the play.

Checklist

  • I have verified that these changes work locally
  • I have updated the README.md (if applicable)
  • I have added tests & descriptions to my models (and macros if applicable)
  • I have raised a documentation PR if applicable (Link here if required)

@matus-tomlein matus-tomlein marked this pull request as ready for review June 13, 2024 13:54
@matus-tomlein matus-tomlein requested a review from a team as a code owner June 13, 2024 13:54
Copy link
Contributor

@rlh1994 rlh1994 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change the target branch please, otherwise looks good

@matus-tomlein matus-tomlein changed the base branch from main to release/0.8.0 June 18, 2024 08:48
@matus-tomlein matus-tomlein force-pushed the issue/70-pip_multi_page_view_id_playback branch from 1d8fad0 to ff1b6ee Compare June 18, 2024 14:00
…ort multiple aggregations with different order by
@matus-tomlein
Copy link
Contributor Author

@rlh1994 can you please take another look? I had to add a CTE in the base_this_run table because Redshift couldn't handle two listagg aggregations in one CTE with different ordering.

Copy link
Contributor

@rlh1994 rlh1994 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Redshift tests paying off already!

@matus-tomlein matus-tomlein merged commit 479a29c into release/0.8.0 Jun 19, 2024
5 checks passed
@matus-tomlein matus-tomlein deleted the issue/70-pip_multi_page_view_id_playback branch June 19, 2024 08:33
matus-tomlein added a commit that referenced this pull request Jun 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants