update macro used for passing through all columns to ensure quoting #129

fivetran-reneeli · 2024-10-04T19:20:44Z

PR Overview

This PR will address the following Issue/Feature:
#128
This PR will result in the following new package version: 0.16.0

While changes are made 'behind the scenes' to now allow models to successfully run with both hubspot__pass_through_all_columns and hubspot__<>_pass_through_columns, this may be a breaking change due to leveraging the remove_duplicate_and_prefix_from_columns macro. This is a breaking change because this macro can remove duplicate fields, resulting in an impact to your schema. See the v0.12.0 release notes for more information.

Please provide the finalized CHANGELOG entry which details the relevant changes included in this PR:

Breaking Changes

Switched from using the fivetran_utils.remove_prefix_from_columns macro to the hubspot_source.remove_duplicate_and_prefix_from_columns macro for when hubspot__pass_through_all_columns is enabled and you are passing through all columns in the stg_hubspot__company, stg_hubspot__contact, stg_hubspot__deal, and stg_hubspot__ticket models. This also ensures the source fields passed through are all quoted from the onset. This is a breaking change because this macro can remove duplicate fields, resulting in an impact to your schema. See the v0.12.0 release notes for more information.

Bug Fixes

Introduced hubspot-specific version of the fivetran_utils.pass_through_columns macro titled hubspot_add_pass_through_columns, which introduces quoting around the source fields being brought in as passthrough columns. This will ensure that your warehouse reads the sql correctly, particularly if the field contains special characters or syntax.

Under the Hood

Updated seed data to include fields with special syntax in order to test the above changes.

PR Checklist

Basic Validation

Please acknowledge that you have successfully performed the following commands locally:

dbt run –full-refresh && dbt test
dbt run (if incremental models are present) && dbt test

Before marking this PR as "ready for review" the following have been applied:

The appropriate issue has been linked, tagged, and properly assigned
All necessary documentation and version upgrades have been applied
docs were regenerated (unless this PR does not include any code or yml updates)
BuildKite integration tests are passing
Detailed validation steps have been provided below

Detailed Validation

Please share any and all of your validation steps:

see internal ticket

If you had to summarize this PR in an emoji, which would it be?

💃

macros/hubspot_add_pass_through_columns.sql

Co-authored-by: Joe Markiewicz <74217849+fivetran-joemarkiewicz@users.noreply.github.com>

…res, update changelog

fivetran-joemarkiewicz

@fivetran-reneeli this PR looks great! However, I do have a request to make this a breaking change. I reviewed how we handled the switch from the fivetran_utils. remove_prefix_columns to remove_duplicate_and_prefix_from_columns and it was in the v0.12.0 release which it's listed as a breaking change.

This should be a breaking change because the duplicate removal nature applies a preference on the property_hs_* column names and will only keep the fields with this naming convention. For example, if a customer has propert_hs_contact_name and the contact_name field, the macro will not select the contact_name and will instead retain the property_hs_contact_name field in addition to renaming it to contact_name.

Because of the above, customers could experience certain columns being removed from their staging and downstream models if they are passing through all columns. As a result, we will need to make this a breaking change and apply a breaking change to the downstream dbt_hubspot package as well.

CHANGELOG.md

Co-authored-by: Joe Markiewicz <74217849+fivetran-joemarkiewicz@users.noreply.github.com>

fivetran-joemarkiewicz

@fivetran-reneeli LGTM with a few final CHANGELOG change requests. Nothing required to block the approval and move to release review. Thanks for working through these updates!

fivetran-joemarkiewicz · 2024-10-16T18:50:48Z

CHANGELOG.md

+[PR #129](https://github.com/fivetran/dbt_hubspot_source/pull/129) includes the following updates:
+
+## Breaking Changes
+- Switched from using the `fivetran_utils.remove_prefix_from_columns` macro to the `hubspot_source.remove_duplicate_and_prefix_from_columns` macro for when `hubspot__pass_through_all_columns` is enabled and you are passing through all columns in the `stg_hubspot__company`, `stg_hubspot__contact`, `stg_hubspot__deal`, and `stg_hubspot__ticket` models. This also ensures the source fields passed through are all quoted from the onset. This is a breaking change because this macro can remove duplicate fields, resulting in an impact to your schema. See the [v0.12.0 release notes](https://github.com/fivetran/dbt_hubspot_source/releases/tag/v0.12.0) for more information.


I don't think we necessarily need to link the v0.12.0 release notes since that may cause more confusion. I think what you have before is great, no need to add the last sentence.

Also the stg_hubspot__ticket model was not updated in this release for this change. You can include this in the below bugfix however.

CHANGELOG.md

Co-authored-by: Joe Markiewicz <74217849+fivetran-joemarkiewicz@users.noreply.github.com>

fivetran-reneeli · 2024-10-16T19:42:19Z

Thanks @fivetran-joemarkiewicz made the changes to the CHANGELOG. Will move on to release review

fivetran-avinash · 2024-10-16T19:53:26Z

integration_tests/dbt_project.yml

+    # hubspot_service_enabled: true # enable when generating docs
+    # hubspot_deal_enabled: true # enable when generating docs
+    # hubspot_contact_enabled: true # enable when generating docs
+    hubspot_sales_enabled: true # enable when generating docs


Should lines 15-16 be commented out before merging?

thanks Avinash-- commented out

fivetran-avinash · 2024-10-16T19:54:13Z

integration_tests/dbt_project.yml

@@ -65,15 +67,37 @@ vars:
    hubspot_email_event_dropped_identifier: "email_event_dropped_data"
    hubspot_merged_deal_identifier: "merged_deal_data"

+    # hubspot__pass_through_all_columns: true
+    hubspot__company_pass_through_columns:


Should lines 71-73 be commented out now (or completely removed) now that we've thoroughly tested and validated this solution works, or is there a reason for keeping them in?

No particular reason for keeping them in-- will comment it out

fivetran-avinash

@fivetran-reneeli Nice work, particularly with the cross-warehouse seed testing. Just a few quick comments before approval!

fivetran-avinash

@fivetran-reneeli lgtm!

fivetran-reneeli added 6 commits October 4, 2024 15:18

updates apply

e40bb44

rm the test field in seed data since not all warehouses like the syntax

dc38bc4

docs

d5198a5

new schema

b047a69

try to adapt add pass through column macro

896498b

adjust add pass through macro

fb9ee19

fivetran-joemarkiewicz reviewed Oct 9, 2024

View reviewed changes

macros/hubspot_add_pass_through_columns.sql Outdated Show resolved Hide resolved

fivetran-reneeli and others added 9 commits October 9, 2024 15:07

Update macros/hubspot_add_pass_through_columns.sql

6886a74

Co-authored-by: Joe Markiewicz <74217849+fivetran-joemarkiewicz@users.noreply.github.com>

use custom hubspot pass thru columns, add company seed data for postg…

a56799f

…res, update changelog

docs

fa09f4a

add databricks and snowflake seed, adjust configs

1ed3424

disable

403cf3a

change schema

e08ad5a

new schema

16cd3dc

column type config fix

5d8d1bb

new schema

81e26f2

fivetran-reneeli requested a review from fivetran-joemarkiewicz October 10, 2024 15:39

fivetran-reneeli added 2 commits October 10, 2024 12:30

rm the hubspot_source prefix

77c4261

new schema

6742939

fivetran-joemarkiewicz requested changes Oct 11, 2024

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

CHANGELOG.md Outdated Show resolved Hide resolved

fivetran-joemarkiewicz reviewed Oct 11, 2024

View reviewed changes

CHANGELOG.md Show resolved Hide resolved

fivetran-reneeli and others added 4 commits October 15, 2024 15:38

Update CHANGELOG.md

afa5879

Co-authored-by: Joe Markiewicz <74217849+fivetran-joemarkiewicz@users.noreply.github.com>

Update CHANGELOG.md

ea99532

Co-authored-by: Joe Markiewicz <74217849+fivetran-joemarkiewicz@users.noreply.github.com>

update versioning and some docs

bd58f38

more updates

900fdc9

fivetran-reneeli mentioned this pull request Oct 15, 2024

v0.19.0- upstream breaking changes fivetran/dbt_hubspot#147

Merged

7 tasks

fivetran-reneeli added 4 commits October 15, 2024 16:02

new schema

c7819c9

docs

e34eded

rm validation test config from source

bdccd3d

new schema

019f8e3

fivetran-reneeli added 2 commits October 15, 2024 18:25

fix databricks seed error?

00c6305

try again

caca80b

fivetran-joemarkiewicz assigned fivetran-reneeli Oct 16, 2024

fivetran-reneeli requested a review from fivetran-joemarkiewicz October 16, 2024 18:06

fivetran-joemarkiewicz approved these changes Oct 16, 2024

View reviewed changes

fivetran-reneeli and others added 2 commits October 16, 2024 15:33

Update CHANGELOG.md

ab9a102

Co-authored-by: Joe Markiewicz <74217849+fivetran-joemarkiewicz@users.noreply.github.com>

changelog updates

5bf7c7d

fivetran-avinash reviewed Oct 16, 2024

View reviewed changes

quote some test configs

1169162

fivetran-avinash approved these changes Oct 16, 2024

View reviewed changes

fivetran-reneeli merged commit 5a4301c into main Oct 16, 2024
8 checks passed

fivetran-reneeli deleted the bugfix/column_quote_all_passthrough_columns branch October 16, 2024 21:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

update macro used for passing through all columns to ensure quoting #129

update macro used for passing through all columns to ensure quoting #129

fivetran-reneeli commented Oct 4, 2024 •

edited

Loading

fivetran-joemarkiewicz left a comment

fivetran-joemarkiewicz left a comment

fivetran-joemarkiewicz Oct 16, 2024

fivetran-joemarkiewicz Oct 16, 2024

fivetran-reneeli commented Oct 16, 2024

fivetran-avinash Oct 16, 2024

fivetran-reneeli Oct 16, 2024

fivetran-avinash Oct 16, 2024

fivetran-reneeli Oct 16, 2024

fivetran-avinash left a comment

fivetran-avinash left a comment

update macro used for passing through all columns to ensure quoting #129

update macro used for passing through all columns to ensure quoting #129

Conversation

fivetran-reneeli commented Oct 4, 2024 • edited Loading

PR Overview

Breaking Changes

Bug Fixes

Under the Hood

PR Checklist

Basic Validation

Detailed Validation

If you had to summarize this PR in an emoji, which would it be?

fivetran-joemarkiewicz left a comment

Choose a reason for hiding this comment

fivetran-joemarkiewicz left a comment

Choose a reason for hiding this comment

fivetran-joemarkiewicz Oct 16, 2024

Choose a reason for hiding this comment

fivetran-joemarkiewicz Oct 16, 2024

Choose a reason for hiding this comment

fivetran-reneeli commented Oct 16, 2024

fivetran-avinash Oct 16, 2024

Choose a reason for hiding this comment

fivetran-reneeli Oct 16, 2024

Choose a reason for hiding this comment

fivetran-avinash Oct 16, 2024

Choose a reason for hiding this comment

fivetran-reneeli Oct 16, 2024

Choose a reason for hiding this comment

fivetran-avinash left a comment

Choose a reason for hiding this comment

fivetran-avinash left a comment

Choose a reason for hiding this comment

fivetran-reneeli commented Oct 4, 2024 •

edited

Loading