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

release/v0.10.2 #36

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

release/v0.10.2 #36

wants to merge 7 commits into from

Conversation

fivetran-joemarkiewicz
Copy link
Contributor

@fivetran-joemarkiewicz fivetran-joemarkiewicz commented Sep 18, 2024

PR Overview

This PR will address the following Issue/Feature: Issue #32

This PR will result in the following new package version: v0.10.2

This will not be a breaking change since this will not impact the end results of the models. This will simply choose to partition by source_relation if that feature is being used or not.

Please provide the finalized CHANGELOG entry which details the relevant changes included in this PR:

Bug Fixes

  • Updated the is_most_recent_record window function in the following models to exclude the source_relation field from the partition statement when pinterest_ads_union_schemas or pinterest_ads_union_databases variables are empty. Also, modified it to skip the window function if the upstream table is empty, using the new result_if_table_exists() and is_table_empty() macros. This change addresses Redshift's issue with partitioning by constant expressions.
    • stg_pinterest_ads__ad_group_history
    • stg_pinterest_ads__advertiser_history
    • stg_pinterest_ads__campaign_history
    • stg_pinterest_ads__keyword_history
    • stg_pinterest_ads__pin_promotion_history

Contributors

PR Checklist

Basic Validation

Please acknowledge that you have successfully performed the following commands locally:

  • dbt run –full-refresh && dbt test
  • [n/a] dbt run (if incremental models are present) && dbt test

Before marking this PR as "ready for review" the following have been applied:

  • The appropriate issue has been linked, tagged, and properly assigned
  • All necessary documentation and version upgrades have been applied
  • docs were regenerated (unless this PR does not include any code or yml updates)
  • BuildKite integration tests are passing
  • Detailed validation steps have been provided below

Detailed Validation

Please share any and all of your validation steps:

In addition to relying on the standard buildkite integration tests, I was able to confirm the compilation state based off these changes for the following scenarios. Please note, this is only validating the ad_group_history model. However, since the code changes are consistent across the other models this should be appropriate coverage.

  • ✅ When running on an empty schema (the results should just be 1=1 as is_most_recent_record).
    • image
  • ✅ When running on a single populated schema. We do not partition by source_relation since that field is a constant expression when not using the union data feature.
    • image
  • ✅ When using the union data feature and running on two populated schemas the window function appropriately partitions by the source_relation since that is no longer a constant expression and should be included in the window function to differentiate sources.
    • image
  • ✅ When using the union data feature and running on two empty schemas we do not want to use the window function at all in order to avoid a similar constant expression issue. Therefore, the result is appropriately a simple 1=1 as is_most_recent_version.
    • image

Additionally, I was able to test that these changes work as expected and do not cause any issues when using this version in downstream dependencies:

If you had to summarize this PR in an emoji, which would it be?

🥧

* pinterest-source-32

* Change log and macros

* Change log again

---------

Co-authored-by: JessicaAKelley <kelleyajessica@gmail.com>
@fivetran-joemarkiewicz fivetran-joemarkiewicz marked this pull request as ready for review September 18, 2024 15:11
Copy link
Contributor

@fivetran-reneeli fivetran-reneeli 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!

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.

3 participants