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

bring in ad type to queries #33

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

andrew-woelfel
Copy link
Contributor

Are you a current Fivetran customer?
<Andrew Woelfel, Analyst, Xometry>

What change(s) does this PR introduce?
<We would like to add ad_type across all platforms. For example in adwords there are different types of ads you can run (display, search, etc.). Need to add them as columns to easy querying. I have added the columns that I am aware of in certain ad platforms but not all of them.>

Does this PR introduce a breaking change?

  • Yes (please provide breaking change details below.)
  • [X ] No (please provide explanation how the change is non breaking below.)
  • Adds an additional column of data to the queries

Is this PR in response to a previously created Issue

  • Yes, Issue [link issue number here]
  • [X ] No

How did you test the PR changes?

  • CircleCi
  • Other (please provide additional testing details below)

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

  • BigQuery
  • Redshift
  • [X ] 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.

@andrew-woelfel
Copy link
Contributor Author

@fivetran-joemarkiewicz Please let me know if this makes sense or any other details are needed

@fivetran-joemarkiewicz
Copy link
Contributor

Hey @andrew-woelfel thanks so much for opening this PR and contributing back to the package and community! 🎉 ❤️

I completely agree that ad_type would be a great addition to the ad_reporting model. I have actually been fielding a lot of questions lately about ad_types related to these platforms so I see this being a huge value add to others.

With a cursory glance your edits make sense. I will take some time to test this as well as try and find the corresponding ad_types for the remaining platforms before merging into the release branch. In the meantime, I will respond back here if I have any other questions 😄

@fivetran-joemarkiewicz
Copy link
Contributor

@andrew-woelfel I did a bit deeper of a dive into this PR and realized there is going to be quite a bit more work needed to bring in the ad_types to the final roll up model. 😞

It seems for almost all of the platforms, we do not bring the ad_type in. Therefore, we will need to make updates to all of the underlying packages to find where the ad_type lives within the schemas, add them to the models, and then roll them up in this package.

While this will require more effort, I do believe this will be a worth while initiative to add the respective ad_type fields for the platforms to this final roll up model. To help out team scope this work our accordingly, I created a Feature Request where we can chat in more detail about all the platforms and what our next steps will be for this enhancement.

Thanks again so much for raising this PR and I will keep this open, because we will eventually want to merge this PR once we have the relevant fields add to the base packages.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants