Feature/add conversions #36
Conversation
There was a problem hiding this comment.
@fivetran-jamie these changes look great! I just have a few minor suggestions, but none related to the actual code. I see this being a huge value add for our users!
Once you address the comments in this review let's hold off merging until the other updates in the Google and LinkedIn packages are ready as well. Thanks and great job!
| {# | ||
| Adapted from fivetran_utils.fill_pass_through_columns() macro. | ||
| Ensures that downstream summations work if a connector schema is missing one of your facebook_ads__basic_ad_actions_passthrough_metrics | ||
| #} | ||
| {% if var('facebook_ads__basic_ad_actions_passthrough_metrics') %} | ||
| {% for field in var('facebook_ads__basic_ad_actions_passthrough_metrics') %} | ||
| {% if field.transform_sql %} | ||
| , coalesce(cast({{ field.transform_sql }} as {{ dbt.type_float() }}), 0) as {{ field.alias if field.alias else field.name }} | ||
| {% else %} | ||
| , coalesce(cast({{ field.alias if field.alias else field.name }} as {{ dbt.type_float() }}), 0) as {{ field.alias if field.alias else field.name }} | ||
| {% endif %} | ||
| {% endfor %} |
There was a problem hiding this comment.
Should we consider making this a macro?
There was a problem hiding this comment.
going back and forth on this one, as the code is more complex than what's usually in our staging models, but it is really only used in this one model, so creating a macro wouldn't make us any DRYer. what are your thoughts?
There was a problem hiding this comment.
That's a good point. If it won't make things much dryer then there is no point in wrapping it up in a macro. I am good leaving as is. Thanks!
fivetran-data-model-bot
requested review from
a team,
fivetran-reneeli and
fivetran-avinash
and removed request for
a team
Co-authored-by: Joe Markiewicz <74217849+fivetran-joemarkiewicz@users.noreply.github.com>
|
We just ran into an issue where we wanted to create a solution for exactly this. This will be super helpful. |
fivetran-jamie
added
the
status:blocked
|
FYI for anyone subscribed here, this is currently blocked. We are working with Engineering to ensure that the conversion monetary values ( |
PR Overview
This PR will address the following Issue/Feature:
No issue created
This PR will result in the following new package version:
v0.8.0 -- looking at our UI,
basic_ad_actionsis a child ofbasic_adand is included by default with it, but juuuust in case someone doesn't have the new table, let's make it breaking, as it is a schema change.Please provide the finalized CHANGELOG entry which details the relevant changes included in this PR:
🚨 Breaking Changes 🚨
basic_ad_actionspre-built report in order to grab conversion data.basic_ad_actionsis a child table of the already-requiredbasic_adreport, broken down byaction_type.stg_facebook_ads__basic_ad_actions(and its_tmpcounterpart) staging model. Given that this is a schema change for the package, this a breaking change.Feature Updates
basic_ad_actionssource table, creates afacebook_ads__basic_ad_actions_passthrough_metricsvariable to pass through additional conversion value metrics to downstream models. By default, the package includes only the conversion value calculated using the default attribution window, but your report may include calculations using the other windows defined here. See README for details on how to use the new var.optimization_goalfield tostg_facebook_ads__ad_set_historymodel. This is defined as the optimization goal this ad set is using, possible values of which are defined here.conversion_domainfield tostg_facebook_ads__ad_historymodel. This is defined as the domain you've configured the ad to convert to.Documentation
facebook_ads__basic_ad_passthrough_metricsvariable. See README for details.PR Checklist
Basic Validation
Please acknowledge that you have successfully performed the following commands locally:
Before marking this PR as "ready for review" the following have been applied:
Detailed Validation
Please share any and all of your validation steps:
See Validation tests in transform PR