fivetran / dbt_microsoft_ads_source

Fivetran data models for Microsoft (Bing) Ads built using dbt.
https://fivetran.github.io/dbt_microsoft_ads_source/
Apache License 2.0
4 stars 7 forks source link

Update/ad-reporting-v2 #20

Closed fivetran-sheringuyen closed 2 years ago

fivetran-sheringuyen commented 2 years ago

Pull Request Are you a current Fivetran customer? Fivetran employee

What change(s) does this PR introduce? Breaking Changes PR [] makes the below updates that may affect your workflow:

Feature Enhancements We have added the below feature enhancements to this package in (PR #)[]:

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

N/A

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

:dancer: **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-sheringuyen commented 2 years ago

Hey @fivetran-joemarkiewicz, re:

I really like the depth you put into the staging model tests. I do wonder if this would break if individuals add more metrics via the passthrough columns? Is this something we should consider?

I think in my head I've only been thinking of customers using that variable to passthrough metrics - in which case, this shouldn't be a problem at all. If they are passing through non-metrics, this would be a concern we should consider -- however, it would also break some of the sum( {{ metric }} ) logic that we have incorporated. But that would open another can of worms one that I think can be avoided since we do name the variables *_metrics so there should be the assumption that passthrough columns should only be metrics.

Additionally, before adding changes for the macro + CTE suggestions, I'd like to discuss that as a team first and walk through the nuances.

fivetran-joemarkiewicz commented 2 years ago

Hey @fivetran-joemarkiewicz, re:

I really like the depth you put into the staging model tests. I do wonder if this would break if individuals add more metrics via the passthrough columns? Is this something we should consider?

I think in my head I've only been thinking of customers using that variable to passthrough metrics - in which case, this shouldn't be a problem at all. If they are passing through non-metrics, this would be a concern we should consider -- however, it would also break some of the sum( {{ metric }} ) logic that we have incorporated. But that would open another can of worms one that I think can be avoided since we do name the variables *_metrics so there should be the assumption that passthrough columns should only be metrics.

Additionally, before adding changes for the macro + CTE suggestions, I'd like to discuss that as a team first and walk through the nuances.

@fivetran-sheringuyen this makes sense to me! Maybe we can add a small clarifying statement in the README to only include metrics in this passthrough variable. This way there is nothing left up to interpretation (although it does seem very obvious to me haha).

I also agree that we should discuss the passthrough + macro behavior closer as a team. We can do so during standup this morning!

fivetran-sheringuyen commented 2 years ago

Hey @fivetran-joemarkiewicz! Just updated the repo for PR changes. Let me know if you have any follow up questions!