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

Simplify base events this run #64

Conversation

georgewoodhead
Copy link
Contributor

@georgewoodhead georgewoodhead commented Nov 28, 2023

Description & motivation

Refactor the snowplow_media_player_base_events_this_run table into a single model for a cleaner and less error prone approach, similar to what we do in the unified package.

The model uses a single macro get_context_fields which gets all the specified fields for a specified context. If the context isn't enabled it will cast nulls for those fields so they are still available downstream. This means we now only need to specify the context fields once in one file.

{{ get_context_fields(
      enabled=var('snowplow__enable_media_ad_break', false),
      context='contexts_com_snowplowanalytics_snowplow_media_ad_break_1',
      prefix='media_ad_break_',
      fields=[
        {'field':'name', 'dtype':'string'},
        {'field':'breakId', 'dtype':'string'},
        {'field':'breakType', 'dtype':'string'},
      ]) }}

Each item in fields must match what is set in the iglu schema {'field':'breakType', 'dtype':'string'} . The outputted field has an alias prefix that will match the postgres/redshift alias prefix created from the base macro query.


In doing the refactor we also resolve issue #60. An integration test run for a v2 only configuration has been added to cover this case.

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)

@snowplowcla snowplowcla added the cla:yes [Auto generated] Snowplow Contributor License Agreement has been signed. label Nov 28, 2023
@georgewoodhead georgewoodhead force-pushed the feature/simplify-base-events-this-run branch 3 times, most recently from 3a68e11 to 9ec160a Compare November 29, 2023 17:04
@georgewoodhead georgewoodhead marked this pull request as ready for review November 29, 2023 17:09
@georgewoodhead georgewoodhead requested a review from a team as a code owner December 4, 2023 10:56
Copy link
Contributor

@matus-tomlein matus-tomlein left a comment

Choose a reason for hiding this comment

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

LGTM! Very nice refactor, so much cleaner and easier to follow than my macros!

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.

Just one actual change required, the rest are just questions/nitpicks!

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.

Just one actual change required, the rest are just questions/nitpicks!

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.

Looks good to me!

@georgewoodhead georgewoodhead force-pushed the feature/simplify-base-events-this-run branch from 58ee495 to ca476e4 Compare December 7, 2023 11:54
@georgewoodhead georgewoodhead merged commit 0f8a74f into release/snowplow-media-player/0.7.0 Dec 7, 2023
4 checks passed
@georgewoodhead georgewoodhead deleted the feature/simplify-base-events-this-run branch December 7, 2023 15:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla:yes [Auto generated] Snowplow Contributor License Agreement has been signed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants