fivetran / dbt_snapchat_ads

Fivetran data models for Snapchat Ads built using dbt.
https://fivetran.github.io/dbt_snapchat_ads
Apache License 2.0
1 stars 6 forks source link

Add missing columns to url_report model #14

Closed dumkydewilde closed 1 year ago

dumkydewilde commented 1 year ago

Are you a current Fivetran customer? Yes, consulting analytics engineer for FTD

What change(s) does this PR introduce? This PR adds the missing columns ad_squad_id, ad_squad_name, campaign_id and campaign_name to url_report model and snapchat.yml. These columns come from CTEs that were already present in the url_report model, just not joined and added. Hence, these columns could not be populated downstream in e.g. the ad_reporting package.

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

:metal: **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](https://www.surveymonkey.com/r/DQ7K7WW) on our existing dbt packages or what you'd like to see next.
fivetran-joemarkiewicz commented 1 year ago

Hi @dumkydewilde thanks so much for opening this (and the other ad reporting) PR to address the original issue you raised!

My team and I will review these changes and let you know if we have any questions before proceeding to integrated into the next release of the packages. I do just want to make sure there was not an intentional reason we left these out in the final releases.

Would you be able to confirm that when bringing in the ad_squad and campaign ctes, you don't see any duplication of ads. I will test with your branch locally, but just wanted to confirm before moving forward.

dumkydewilde commented 1 year ago

Thanks for getting back @fivetran-joemarkiewicz! I was indeed curious why it was left out originally, but so far we haven't come across any issues in our own reporting when enabling it locally. I will double check on Monday though and get back to you then.

fivetran-joemarkiewicz commented 1 year ago

Sounds great, thanks so much @dumkydewilde. Let me know and if all looks good on your end come next week, we can move forward with these suggested changes.

dumkydewilde commented 1 year ago

Hi @fivetran-joemarkiewicz, just double checked for any anomalies in our Snapchat data running the pipeline with the updates from this PR and couldn't find anything. Please do check on your side as well of course, but looking at the models I also wouldn't expect anything being impacted since it is all just left joins adding columns to existing rows.

fivetran-joemarkiewicz commented 1 year ago

@dumkydewilde thanks so much for the confirmation here! I will double check on my end, but will look to do a more thorough review of your PRs this week and will hope to integrate them into the packages soon.

I will follow up in these PRs if there are any other clarifying questions I may have during my reviews.