-
Notifications
You must be signed in to change notification settings - Fork 1
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 suffix to duplicate passthrough fields #15
Conversation
…o feature/conversions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -20,6 +20,25 @@ vars: | |||
|
|||
reddit_ads__ad_conversions_passthrough_metrics: | |||
- name: 'view_through_conversion_attribution_window_week' | |||
reddit_ads__account_passthrough_metrics: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did we want to keep these new variable configurations for the default dbt_project.yml
Buildkite runs, or should we comment them out?
If we want to keep them, we'd probably want to add these to the source file as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah just wanted to make sure this passes on all warehouses.
What do you mean by source file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bleh, dbt_reddit_ads_source
, just so the dbt_project.yml
files are matching.
Something to do later though!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah gotcha, yah later haha
reddit_ads__ad_passthrough_metrics: | ||
- name: conversions | ||
- name: app_install_metrics_install | ||
alias: installs | ||
|
||
# For validation testing | ||
reddit_ads__conversion_event_types: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should these be commented out before merging since it's just for validation tests, or is there no harm, no foul here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no harm in leaving them IMO, more testing
- Ensures the addition of conversion metrics in [v0.3.0](https://github.com/fivetran/dbt_reddit_ads/blob/main/CHANGELOG.md#dbt_reddit_ads-v030) is truly backwards compatible and will avoid duplicate column errors regardless of any pre-existing configurations. | ||
- Specifically, if you were previously utilizing [passthrough column](https://github.com/fivetran/dbt_reddit_ads?tab=readme-ov-file#passing-through-additional-metrics) variables to include fields called `conversions`, `view_through_conversions`, `total_value`, or `total_items`, the package's version of these fields will take precedence. Your fields will be included as well, but suffixed with a `_c`. | ||
|
||
## Under the Hood |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should call out the modifications to the consistency tests as well regarding the new measures we are now testing/comparing between dev and prod.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
adding
@@ -126,6 +126,14 @@ vars: | |||
- name: "new_custom_field" | |||
alias: "custom_field" | |||
- name: "a_second_field" | |||
reddit_ads__account_conversions_passthrough_metrics: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably provide an example of how the alias would be applied for a few of these variables.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fivetran-jamie I'm wondering if we should call out the backwards-compatibility functionality of conversions fields, or whether it should live in the DECISIONLOG. This update might not be quite intuitive to a customer unless they really go hunting for the macro. What are your thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good callout -- I think the decision log makes the most sense
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fivetran-jamie left a few comments!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fivetran-jamie Approved!
Two final suggestions: Can you add a Documentation Update section to the CHANGELOG and link to the DECISIONLOG there? (Optional, but you could also link to the README as well).
PR Overview
This PR will address the following Issue/Feature:
Though conversions data lives in the new source report tables we included, there are some legacy fields in the pre-existing reports that users may have included and aliased as
conversions
. This will ensure we avoid duplicate column errors by applying a_c
suffix to any conversions fields previously included by the user via pass through column variables.This PR will result in the following new package version:
Could be v0.3.1, as this is a fix to the already breaking v0.3.0 release. Though perhaps there's no harm in making it v0.4.0?
Please provide the finalized CHANGELOG entry which details the relevant changes included in this PR:
PR Checklist
Basic Validation
Please acknowledge that you have successfully performed the following commands locally:
Before marking this PR as "ready for review" the following have been applied:
Detailed Validation
Please share any and all of your validation steps:
Validation tests passing
If you had to summarize this PR in an emoji, which would it be?
🛃