-
Notifications
You must be signed in to change notification settings - Fork 2
feature/api-v3-updates #27
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
Conversation
| - name: comment_downvotes | ||
| description: '{{ doc("comment_downvotes") }}' | ||
| - name: comment_upvotes | ||
| description: '{{ doc("comment_upvotes") }}' | ||
| - name: comments_page_views | ||
| description: '{{ doc("comments_page_views") }}' |
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.
These columns (and same in the other lines for these cols) only existed in the source and were never brought into staging.
fivetran-joemarkiewicz
left a comment
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-catfritz thanks for making these updates. A few change requests before approving.
| account: "{{ source('reddit_ads', 'account') }}" | ||
| business_account: "{{ source('reddit_ads','business_account') }}" |
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.
Interestingly it doesn't look like either of these variables ultimately are ever used. However, for consistency and to retain the scope of these changes, no action/change required but found this interesting.
| {%- set business_account_or_account = 'business_account' if var('reddit_ads__using_business_account', True) else 'account' -%} | |
| {{ | |
| fivetran_utils.union_data( | |
| table_identifier=business_account_or_account, | |
| database_variable='reddit_ads_database', |
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.
Interesting... I guess in general the union_data models don't use the vars?
…dbt_reddit_ads into feature/api-v3-updates merge
fivetran-catfritz
left a comment
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.
Thanks @fivetran-joemarkiewicz I have made the updates.
| account: "{{ source('reddit_ads', 'account') }}" | ||
| business_account: "{{ source('reddit_ads','business_account') }}" |
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.
Interesting... I guess in general the union_data models don't use the vars?
fivetran-joemarkiewicz
left a comment
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.
Looks good with one CHANGELOG recommendation.
CHANGELOG.md
Outdated
| | [stg_reddit_ads__account_conversions_report_tmp](https://fivetran.github.io/dbt_reddit_ads/#!/model/model.reddit_ads.stg_reddit_ads__account_conversions_report)<br>[stg_reddit_ads__ad_group_conversions_report_tmp](https://fivetran.github.io/dbt_reddit_ads/#!/model/model.reddit_ads.stg_reddit_ads__ad_group_conversions_report)<br>[stg_reddit_ads__ad_conversions_report_tmp](https://fivetran.github.io/dbt_reddit_ads/#!/model/model.reddit_ads.stg_reddit_ads__ad_conversions_report)<br>[stg_reddit_ads__campaign_conversions_report_tmp](https://fivetran.github.io/dbt_reddit_ads/#!/model/model.reddit_ads.stg_reddit_ads__campaign_conversions_report)<br>[stg_reddit_ads__campaign_country_conversions_report_tmp](https://fivetran.github.io/dbt_reddit_ads/#!/model/model.reddit_ads.stg_reddit_ads__campaign_country_conversions_report) | New Columns | | `clicks`<br>`views` | Added to support API v3. Coalesced downstream with the deprecated columns `click_through_conversion_attribution_window_month` and `view_through_conversion_attribution_window_month`. | | ||
| | [stg_reddit_ads__account_conversions_report_tmp](https://fivetran.github.io/dbt_reddit_ads/#!/model/model.reddit_ads.stg_reddit_ads__account_conversions_report)<br>[stg_reddit_ads__ad_group_conversions_report_tmp](https://fivetran.github.io/dbt_reddit_ads/#!/model/model.reddit_ads.stg_reddit_ads__ad_group_conversions_report)<br>[stg_reddit_ads__ad_conversions_report_tmp](https://fivetran.github.io/dbt_reddit_ads/#!/model/model.reddit_ads.stg_reddit_ads__ad_conversions_report)<br>[stg_reddit_ads__campaign_conversions_report_tmp](https://fivetran.github.io/dbt_reddit_ads/#!/model/model.reddit_ads.stg_reddit_ads__campaign_conversions_report)<br>[stg_reddit_ads__campaign_country_conversions_report_tmp](https://fivetran.github.io/dbt_reddit_ads/#!/model/model.reddit_ads.stg_reddit_ads__campaign_country_conversions_report) | Deprecated Columns | `click_through_conversion_attribution_window_month`<br>`view_through_conversion_attribution_window_month` | | Coalesced downstream with the new columns `clicks` and `views`. | |
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.
I'd prefer we make the Note from the first section regarding the new columns to be it's own section for the non tmp models. My recommendation would be to have a new row for the non tmp staging models after the callout of the new fields in the tmp models and highlight the coalesce modification to the fields in the staging model.
…dbt_reddit_ads into feature/api-v3-updates merge
…dbt_reddit_ads into feature/api-v3-updates merge
PR Overview
Package version introduced in this PR:
This PR addresses the following Issue/Feature(s):
Summary of changes:
Submission Checklist
Changelog