feature/v2-updates #18
Conversation
fivetran-joemarkiewicz
added
documentation
There was a problem hiding this comment.
Hey @fivetran-joemarkiewicz, great work on the PR. I left some in-line comments I'd like for you to check out, let me know if you have any questions.
This looks like it also takes care of PR #15 and allows us to remove the feature/spark branch for this repo! Additionally, did you still want to file feature requests for a video related model in the future?
models/src_pinterest_ads.yml
Outdated
|
|
||
| - name: ad_group_report | ||
| description: source table |
There was a problem hiding this comment.
Can we improve on the description for this table?
There was a problem hiding this comment.
Updated.
models/src_pinterest_ads.yml
Outdated
|
|
||
| - name: advertiser_history | ||
| description: source table |
There was a problem hiding this comment.
Can we improve on the description for this table?
There was a problem hiding this comment.
Updated.
models/src_pinterest_ads.yml
Outdated
|
|
||
| - name: advertiser_report | ||
| description: source table |
There was a problem hiding this comment.
Can we improve on the description for this table?
There was a problem hiding this comment.
Updated.
models/src_pinterest_ads.yml
Outdated
|
|
||
| - name: campaign_report | ||
| description: source table |
There was a problem hiding this comment.
Can we improve on the description for this table?
There was a problem hiding this comment.
Updated.
models/src_pinterest_ads.yml
Outdated
|
|
||
| - name: keyword_history | ||
| description: source table |
There was a problem hiding this comment.
Can we improve on the description for this table?
There was a problem hiding this comment.
Updated.
| - dbt_utils.unique_combination_of_columns: | ||
| combination_of_columns: | ||
| - date_day | ||
| - ad_group_id |
There was a problem hiding this comment.
I have a similar note here, are ad_group_ids unique within advertiser/campaigns?
There was a problem hiding this comment.
Per our ERD I have added the advertiser_id to the grain testing.
| - dbt_utils.unique_combination_of_columns: | ||
| combination_of_columns: | ||
| - date_day | ||
| - campaign_id |
There was a problem hiding this comment.
And a similar note here, are campaign_ids unique within advertisers?
There was a problem hiding this comment.
Per our ERD I updated this to include the advertiser_id to the grain testing.
| combination_of_columns: | ||
| - date_day | ||
| - keyword_id |
There was a problem hiding this comment.
And a similar note here, are keyword_ids unique within advertiser/campaigns/ad groups/pin promotions?
There was a problem hiding this comment.
Per our ERD I updated this to include the ad_group_id, campaign_id, and advertiser_id for the grain testing.
models/docs.md
Outdated
|
|
||
| {% docs advertiser_id %} The ID of the related Advertiser. {% enddocs %} | ||
|
|
||
| {% docs impressions %} The number of impressions that occurred on the day of the record. {% enddocs %} |
There was a problem hiding this comment.
Might be worth to specify that this is both paid and earned.
There was a problem hiding this comment.
Definitely should include this. Thanks for calling it out!
There was a problem hiding this comment.
Updated
models/docs.md
Outdated
|
|
||
| {% docs impressions %} The number of impressions that occurred on the day of the record. {% enddocs %} | ||
|
|
||
| {% docs clicks %} The number of clicks that occurred on the day of the record. {% enddocs %} |
There was a problem hiding this comment.
Might be worth to specify that this is both paid and earned.
There was a problem hiding this comment.
Ditto! Updated.
There was a problem hiding this comment.
Thanks for the review @fivetran-sheringuyen! I just applied the updates from your comments. Please let me know if you have any questions regarding my updates 😄
models/stg_pinterest_ads.yml
Outdated
| description: "{{ doc('campaign_id') }}" | ||
| - name: advertiser_id | ||
| description: "{{ doc('advertiser_id') }}" | ||
| - name: _fivetran_synced |
There was a problem hiding this comment.
This is a great point. I will remove since it isn't used downstream.
models/stg_pinterest_ads.yml
Outdated
| description: "{{ doc('campaign_id') }}" | ||
| - name: advertiser_id | ||
| description: "{{ doc('advertiser_id') }}" | ||
| - name: _fivetran_synced |
There was a problem hiding this comment.
Good point! Just updated
models/stg_pinterest_ads.yml
Outdated
| description: Unique identifier of the owner user. | ||
| - name: updated_at | ||
| description: "{{ doc('updated_at') }}" | ||
| - name: _fivetran_synced |
There was a problem hiding this comment.
I agree, just updated
models/stg_pinterest_ads.yml
Outdated
| description: "{{ doc('advertiser_id') }}" | ||
| tests: | ||
| - not_null | ||
| - name: _fivetran_synced |
There was a problem hiding this comment.
Agree, just updated!
models/stg_pinterest_ads.yml
Outdated
| - name: advertiser_id | ||
| description: The ID of the related Advertiser. | ||
| description: "{{ doc('advertiser_id') }}" | ||
| - name: _fivetran_synced |
There was a problem hiding this comment.
Agreed, just updated
| - dbt_utils.unique_combination_of_columns: | ||
| combination_of_columns: | ||
| - date_day | ||
| - campaign_id |
There was a problem hiding this comment.
Per our ERD I updated this to include the advertiser_id to the grain testing.
| combination_of_columns: | ||
| - date_day | ||
| - keyword_id |
There was a problem hiding this comment.
Per our ERD I updated this to include the ad_group_id, campaign_id, and advertiser_id for the grain testing.
models/docs.md
Outdated
|
|
||
| {% docs advertiser_id %} The ID of the related Advertiser. {% enddocs %} | ||
|
|
||
| {% docs impressions %} The number of impressions that occurred on the day of the record. {% enddocs %} |
There was a problem hiding this comment.
Definitely should include this. Thanks for calling it out!
models/docs.md
Outdated
|
|
||
| {% docs advertiser_id %} The ID of the related Advertiser. {% enddocs %} | ||
|
|
||
| {% docs impressions %} The number of impressions that occurred on the day of the record. {% enddocs %} |
There was a problem hiding this comment.
Updated
models/docs.md
Outdated
|
|
||
| {% docs impressions %} The number of impressions that occurred on the day of the record. {% enddocs %} | ||
|
|
||
| {% docs clicks %} The number of clicks that occurred on the day of the record. {% enddocs %} |
There was a problem hiding this comment.
Ditto! Updated.
There was a problem hiding this comment.
Looks good, thanks for the updates @fivetran-joemarkiewicz !
Are you a current Fivetran customer?
Fivetran created PR
What change(s) does this PR introduce?
🚨 Breaking Changes 🚨
The
pin_promotion_report_pass_through_metricvariable has been renamed topinterest__pin_promotion_report_passthrough_metrics.🎉 Feature Enhancements 🎉
This PR includes the following changes:
Addition of the following staging models which pull from the source counterparts. The inclusion of the additional
_reportsource tables is to generate a more accurate representation of the Pinterest Ads data. For example, not all Ad types are included within thepin_promotion_reporttable. Therefore, the addition of the further grain reports will allow for more flexibility and accurate Pinterest Ad reporting.stg_pinterest_ads__ad_group_reportstg_pinterest_ads__advertiser_reportstg_pinterest_ads__campaign_reportstg_pinterest_ads__keyword_reportstg_pinterest_ads__advertiser_historystg_pinterest_ads__keyword_historyInclusion of additional passthrough metrics:
pinterest__ad_group_report_passthrough_metricspinterest__campaign_report_passthrough_metricspinterest__advertiser_report_passthrough_metricspinterest__keyword_report_passthrough_metricsREADME updates for easier navigation and use of the package.
Addition of identifier variables for each of the source tables to allow for further flexibility in source table direction within the dbt project.
Included grain uniqueness tests for each staging table.
Did you update the CHANGELOG?
Does this PR introduce a breaking change?
Did you update the dbt_project.yml files with the version upgrade (please leverage standard semantic versioning)? (In both your main project and integration_tests)
Is this PR in response to a previously created Bug or Feature Request
How did you test the PR changes?
Select which warehouse(s) were used to test the PR
Provide an emoji that best describes your current mood
💃
Feedback
We are so excited you decided to contribute to the Fivetran community dbt package! We continue to work to improve the packages and would greatly appreciate your feedback on our existing dbt packages or what you'd like to see next.