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

Add missing columns to url_report model #14

Conversation

dumkydewilde
Copy link

@dumkydewilde dumkydewilde commented Feb 9, 2023

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?

  • Yes

Does this PR introduce a breaking change?

  • Yes (please provide breaking change details below.)
  • No (please provide an explanation as to how the change is non-breaking below.)
    This change only adds new columns, this should be handled gracefully by all databases.

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)

  • Yes

Is this PR in response to a previously created Bug or Feature Request

How did you test the PR changes?

  • Buildkite
  • Local (please provide additional testing details below)
    Added the local package to the package.yml to override the package of an existing project using snapchat_ads. Then ran dbt build -s +snapchat_ads succesfully.

Select which warehouse(s) were used to test the PR

  • BigQuery
  • Redshift
  • Snowflake
  • Postgres
  • Databricks
  • Other (provide details below)

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.

@fivetran-joemarkiewicz
Copy link
Contributor

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
Copy link
Author

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
Copy link
Contributor

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
Copy link
Author

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
Copy link
Contributor

@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.

Copy link
Contributor

@fivetran-joemarkiewicz fivetran-joemarkiewicz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dumkydewilde these changes look great and tested on my data and noticed the updates did not cause any issues and I could see the ad squad and campaign information in the final url table.

I made a few small edits in your branch to get it ready for merging, and with that this looks good go! I will kick off our internal release process. Be sure to follow these PRs as we work to integrate them in the next couple days. Realistically, they will probably go live early next week.

@fivetran-joemarkiewicz fivetran-joemarkiewicz merged commit 488c9ac into fivetran:main Feb 21, 2023
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.

2 participants