Conversation
fivetran-reneeli
left a comment
fivetran-reneeli
left a comment
There was a problem hiding this comment.
Just had few comments! Can also make the README alignment and LICENSE updates
| description: '{{ doc("impressions") }}' | ||
| - name: region | ||
| description: '{{ doc("region") }}' | ||
| - name: spend |
There was a problem hiding this comment.
note that spend in the src.yml is the preconverted amount so still in millis, since I noticed the definition in the doc is post converted
There was a problem hiding this comment.
oops disregard this
Co-authored-by: Renee Li <91097070+fivetran-reneeli@users.noreply.github.com>
fivetran-joemarkiewicz
left a comment
fivetran-joemarkiewicz
left a comment
There was a problem hiding this comment.
A few requests before approval
| {"name": "campaign_id", "datatype": dbt.type_string()}, | ||
| {"name": "clicks", "datatype": dbt.type_int()}, | ||
| {"name": "country", "datatype": dbt.type_string()}, | ||
| {"name": "date", "datatype": "date"}, |
There was a problem hiding this comment.
Is there a reason we still have this date field if we're doing the conditional below to alias?
There was a problem hiding this comment.
It looks like when we've done this before, we don't include the original column in the first set (ref)
There was a problem hiding this comment.
I was following the format of the other existing models in this package (example). Should they all be updated?
There was a problem hiding this comment.
Huh, let's leave for now since it doesn't seem to be causing any issues. I imagine this is some code that's just not being used. We can keep this as is to adhere to the approach used in the existing models. However, can you create a FR for us to investigate in a future update.
CHANGELOG.md
Outdated
|
|
||
| ## Features | ||
| - Added the following vars to enable/disable the new sources. See the [README](https://github.com/fivetran/dbt_reddit_ads_source/blob/main/README.md#Step-4-Enable-disable-models-and-sources) for more details. | ||
| - `reddit_ads_campaign_country_report_enabled` |
There was a problem hiding this comment.
All the other ad reporting packages follow the following format for enable/disable variables:
<platform_name>__using_<table_name>In order to ensure consistency across packages, we should update the campaign_country_conversions and campaign_country variables to be the following:
vars:
reddit_ads__using_campaign_country_report: true
reddit_ads__using_campaign_country_conversions_report: trueThere was a problem hiding this comment.
I was following the format of ad_reporting__reddit_ads_enabled, but I can update.
4 participants
PR Overview
Package version introduced in this PR:
This PR addresses the following Issue/Feature(s):
Summary of changes:
Submission Checklist
Changelog