-
Notifications
You must be signed in to change notification settings - Fork 32
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
hotfix/pass-through-all-columns #100
hotfix/pass-through-all-columns #100
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks pretty good to me! left one comment
one additional question: can we add hubspot__pass_through_all_columns: true
to one of the dbt run
steps in buildkite?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not pretty but it works (so this is approved)!
i had another idea though that would move this logic to the column macros...
what if in get_[table]_column macros, we did something like
{% macro get_deal_columns() %}
{% set columns = [
{"name": "_fivetran_synced", "datatype": dbt.type_timestamp()},
{"name": "deal_id", "datatype": dbt.type_int()},
{"name": "deal_pipeline_id", "datatype": dbt.type_string()},
{"name": "deal_pipeline_stage_id", "datatype": dbt.type_string()},
{"name": "is_deleted", "datatype": "boolean"},
{"name": "owner_id", "datatype": dbt.type_int()},
{"name": "portal_id", "datatype": dbt.type_int()},
{"name": "property_dealname", "datatype": dbt.type_string()},
{"name": "property_description", "datatype": dbt.type_string()},
{"name": "property_amount", "datatype": dbt.type_int()},
{"name": "property_closedate", "datatype": dbt.type_timestamp()},
{"name": "property_createdate", "datatype": dbt.type_timestamp()}
] %}
{{ fivetran_utils.add_pass_through_columns(columns, var('hubspot__deal_pass_through_columns')) }}
{% if var('hubspot__pass_through_all_columns', false) %}
{% set source_columns = adapter.get_columns_in_relation(ref('stg_hubspot__deal_tmp')) %}
{% for sc in source_columns if sc.name not in columns.name %} -- not sure if that syntax works but you get the gist
{% do columns.append({"name": sc.name, "datatype": dbt.type_string}) %}
{% endfor %}
{% endif %}
{{ return(columns) }}
{% endmacro %}
PR Overview
This PR will address the following Issue/Feature: #99
This PR will result in the following new package version:
v0.9.0
This is not a breaking change in particular, but will be batched with PR #98 which is a breaking change.
Please detail what change(s) this PR introduces and any additional information that should be known during the review of this PR:
This PR includes the following changes:
get_columns_macro.sql
macro that will be used when users define thehubspot__pass_through_all_columns
variable totrue
.hubspot__pass_through_all_columns: true
config will have all the required columns that are necessary for downstream transformations while also bringing in all the fields from their source.CONTACT
,COMPANY
,DEAL
, andTICKET
staging models to ensure all required columns are included when a user defines thehubspot__pass_through_all_columns
variable as true.hubspot__pass_through_all_columns
variable as defined as true or false.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 acknowledge that the following validation checks have been performed prior to marking this PR as “ready for review”:
I validated that these fields returned appropriately using both the
hubspot__pass_through_all_columns
variable being set to true and false. Additionally, I was able to confirm that both variable declarations worked when paired with the downstream transformation. Additionally, I was able to validate that without these changes, the accompanying PR #98 fails ifhubspot__pass_through_all_columns
is defined as true. However, it is resolved with these changes.See corresponding height ticket for more validation details.
Standard Updates
Please acknowledge that your PR contains the following standard updates:
dbt Docs
Please acknowledge that after the above were all completed the below were applied to your branch:
If you had to summarize this PR in an emoji, which would it be?
🎣