introduce category field upstream #150

fivetran-reneeli · 2024-11-04T19:25:27Z

PR Overview

This PR will address the following Issue/Feature:
#131 , and internal ticket (see upstream PR for most changes)

This PR will result in the following new package version:

v0.20.0

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

Breaking Change (`--full-refresh` required after upgrading)

Introduced the new category column to certain upstream models and added to relevant unique tests. This association field differentiates records by either HUBSPOT_DEFINED (default label) or USER_DEFINED (custom label) and was introduced to the connector in October 2024. For more information, refer to the upstream CHANGELOG
This will be a breaking change and will require a --full-refresh.

PR Checklist

Basic Validation

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

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

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

[na] 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
[ba] Detailed validation steps have been provided below

Detailed Validation

Please share any and all of your validation steps:

Will have customer confirm this branch allows their tests to pass

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

💃

fivetran-joemarkiewicz

@fivetran-reneeli thanks for working through these changes so quickly! A few requests to go along with this PR in addition to the below review notes:

Please replicate the seed file updates you made in the source PR and add them here so we can ensure we are including the new fields in our testing of the transform package.
I noticed that some of those staging models flow downstream to end models. It doesn't look like the category field automatically flows down into the end table provided fields; however, should we consider these fields in the end models? I don't know if this is required, but given the uniqueness tests were failing in the staging models once these fields were added, do we need to consider these in any filters or joins in the transform package?

fivetran-joemarkiewicz · 2024-11-05T15:27:04Z

CHANGELOG.md

+[PR #150](https://github.com/fivetran/dbt_hubspot/pull/150) includes the following updates:
+
+## Breaking Change (`--full-refresh` required after upgrading)  
+- Introduced the new `category` column to certain upstream models and added to relevant unique tests. This association field differentiates records by either HUBSPOT_DEFINED (default label) or USER_DEFINED (custom label) and was introduced to the connector in October 2024. For more information, refer to the upstream [CHANGELOG](https://github.com/fivetran/dbt_hubspot_source/releases/tag/v0.17.0)


I know it's exhaustive, but can you include essentially the entire breaking change note from the source CHANGELOG to be here as well. We can still link to the source release notes, but since this is what is rendered in the Quickstart UI, it would be great to show all relevant breaking changes here without requiring the customer to click through a new link.

no prob, that makes sense! added the note from source in its entirety

fivetran-joemarkiewicz · 2024-11-05T15:27:31Z

CHANGELOG.md

+## Breaking Change (`--full-refresh` required after upgrading)  
+- Introduced the new `category` column to certain upstream models and added to relevant unique tests. This association field differentiates records by either HUBSPOT_DEFINED (default label) or USER_DEFINED (custom label) and was introduced to the connector in October 2024. For more information, refer to the upstream [CHANGELOG](https://github.com/fivetran/dbt_hubspot_source/releases/tag/v0.17.0)
+
+- This will be a breaking change and will require a `--full-refresh`.


We can probably remove this since we already mention it in the title header.

fivetran-joemarkiewicz · 2024-11-05T15:27:54Z

packages.yml

+  # - package: fivetran/hubspot_source
+  #   version: [">=0.17.0", "<0.18.0"]
+
+  - git: https://github.com/fivetran/dbt_hubspot_source.git
+    revision: bugfix/category_add
+    warn-unpinned: false


Reminder to swap before release.

Bumping the above.

fivetran-reneeli · 2024-11-05T21:17:38Z

Thanks @fivetran-joemarkiewicz , updated the seed files and applied the rest of changes.

I will ask the customer about downstream implications as per our internal discussion.

fivetran-joemarkiewicz

LGTM just a few notes to address before release review, but no reason to block approval

CHANGELOG.md

fivetran-joemarkiewicz · 2024-11-07T22:51:07Z

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
+  hubspot_company_enabled: true # enable when generating docs
+  hubspot_marketing_enabled: true # enable when generating docs
+  hubspot_contact_merge_audit_enabled: true # enable when generating docs
+  hubspot_using_all_email_events: true # enable when generating docs
+  hubspot_merged_deal_enabled: true # enable when generating docs  
+  hubspot_ticket_deal_enabled: true # enable when generating docs  


Disable before merging

fivetran-joemarkiewicz · 2024-11-07T22:51:49Z

integration_tests/seeds/merged_deal_data.csv

+9089908390,9014070905,2023-06-22 11:37:07.210000,HUBSPOT_DEFINED
+9089908390,9077310858,2023-06-22 11:37:07.210000,HUBSPOT_DEFINED
+9089908390,9077311778,2023-06-22 11:37:07.210000,HUBSPOT_DEFINED
+9089908390,9014071335,2023-06-22 11:37:07.210000,HUBSPOT_DEFINED
+1364146343,10829010820,2023-06-22 11:37:07.220000,HUBSPOT_DEFINED
+1364146343,10828874842,2023-06-22 11:37:07.220000,HUBSPOT_DEFINED
+1368273549,10829051678,2023-06-22 11:37:07.227000,HUBSPOT_DEFINED
+1368273549,10829028256,2023-06-22 11:37:07.227000,HUBSPOT_DEFINED
+1373595330,10879517198,2023-06-22 11:37:07.227000,HUBSPOT_DEFINED
+1373595330,10828874855,2023-06-22 11:37:07.227000,HUBSPOT_DEFINED
+1006463008,10064581040,2023-06-22 11:37:07.213000,HUBSPOT_DEFINED
+1006463008,9351825260,2023-06-22 11:37:07.213000,HUBSPOT_DEFINED
+1389291069,12351788138,2023-06-28 11:18:32.940000,HUBSPOT_DEFINED
+1389291069,13817889862,2023-06-28 11:18:32.939000,HUBSPOT_DEFINED


I don't believe the category field is necessary here as it isn't in the source data.

removed! thank you

fivetran-joemarkiewicz · 2024-11-07T22:52:09Z

packages.yml

+  # - package: fivetran/hubspot_source
+  #   version: [">=0.17.0", "<0.18.0"]
+
+  - git: https://github.com/fivetran/dbt_hubspot_source.git
+    revision: bugfix/category_add
+    warn-unpinned: false


Bumping the above.

Co-authored-by: Joe Markiewicz <[email protected]>

… test folder

fivetran-avinash · 2024-11-08T19:32:39Z

CHANGELOG.md

+# dbt_hubspot v0.20.0
+[PR #150](https://github.com/fivetran/dbt_hubspot/pull/150) includes the following updates:
+
+## Breaking Change (`--full-refresh` required after upgrading)


These breaking changes are occurring in dbt_hubspot_source. We should call that out here as Upstream Breaking Changes and link to the dbt_hubspot_source release notes or changelog. You could use this PR you previously wrote for an upstream change as an example for how to reword it (i.e.
"because new columns are being added to this staging models, a --full-refresh is needed to access them"), and also link to the dbt_hubspot_source CHANGELOG section or release notes.

That's weird, this got separated out in the PR notes. Apologies.

Highly recommend making these updates, as it isn't quite clear from this CHANGELOG that these changes are being made in the source.

ah thanks Avinash sorry to not catch this! just updated, lmk what you think

fivetran-avinash

@fivetran-reneeli Some comments before approval!

fivetran-avinash · 2024-11-08T19:42:12Z

integration_tests/dbt_project.yml

+        _fivetran_synced: timestamp
+        property_closedate: timestamp
+        property_createdate: timestamp
+        deal_pipeline_id: "{{ 'varchar(100)'  if target.type in ('redshift','postgres') else 'string'}}"


Looks like lines 111-112 were not added in the dbt_hubspot_source integration_tests/dbt_project.yml, we should probably add them there.

oh right that was added in the transforms because buildkite was failing in this package. but yes added it to hubspot_source

fivetran-avinash · 2024-11-08T19:42:47Z

integration_tests/dbt_project.yml

@@ -172,6 +181,7 @@ seeds:
    deal_pipeline_stage_data:
      +column_types:
        stage_id: "{{ 'varchar(100)'  if target.type in ('redshift','postgres') else 'string'}}"
+        pipeline_id: "{{ 'varchar(100)'  if target.type in ('redshift','postgres') else 'string'}}"


Looks like this line was not added in the dbt_hubspot_source integration_tests/dbt_project.yml, we should probably add them there.

thanks for noticing these!

fivetran-avinash · 2024-11-08T19:43:35Z

integration_tests/dbt_project.yml

@@ -137,6 +145,7 @@ seeds:
      +column_types:
        engagement_id: "{{ 'int64' if target.type == 'bigquery' else 'bigint' }}"
        deal_id: "{{ 'int64' if target.type == 'bigquery' else 'bigint' }}"
+        created_at: timestamp


Should probably add this in integration_tests/dbt_project.yml in the dbt_hubspot_source package as well.

fivetran-avinash · 2024-11-08T19:44:08Z

CHANGELOG.md

+  - `stg_hubspot__ticket_engagement`
+
+## Under the Hood
+- Updated the respective seed files in the integration_tests folder to property test for the new `category` field. 


Looks like you also updated seed files as well to match the dbt_hubspot_source package for consistency's sake. Might be worth calling it out!

thanks updated!

fivetran-avinash

@fivetran-reneeli Looks like my major comment note got separated out from the initial PR. If you can address that, then this is good to go!

fivetran-reneeli · 2024-11-11T16:45:22Z

@fivetran-reneeli Looks like my major comment note got separated out from the initial PR. If you can address that, then this is good to go!

Thanks @fivetran-avinash I updated the CHANGELOG. Let me know what you think

fivetran-avinash · 2024-11-11T16:50:36Z

CHANGELOG.md

+[PR #150](https://github.com/fivetran/dbt_hubspot/pull/150) includes the following updates:
+
+## Upstream Breaking Changes (`--full-refresh` required after upgrading)
+- Introduced a new `category` column to the following upstream models (see dbt_hubspot_source [CHANGELOG](https://github.com/fivetran/dbt_hubspot_source/blob/main/CHANGELOG.md#dbt_hubspot_source-v0170) notes). This association field differentiates records by either HUBSPOT_DEFINED (default label) or USER_DEFINED (custom label) and was introduced to the Hubspot connector in October 2024. See the [connector release notes](https://fivetran.com/docs/connectors/applications/hubspot/changelog#october2024) for more.


Suggested change

- Introduced a new `category` column to the following upstream models (see dbt_hubspot_source [CHANGELOG](https://github.com/fivetran/dbt_hubspot_source/blob/main/CHANGELOG.md#dbt_hubspot_source-v0170) notes). This association field differentiates records by either HUBSPOT_DEFINED (default label) or USER_DEFINED (custom label) and was introduced to the Hubspot connector in October 2024. See the [connector release notes](https://fivetran.com/docs/connectors/applications/hubspot/changelog#october2024) for more.

- Introduced a new `category` column to the following upstream models (see `dbt_hubspot_source` [CHANGELOG](https://github.com/fivetran/dbt_hubspot_source/blob/main/CHANGELOG.md#dbt_hubspot_source-v0170) notes). This association field differentiates records by either HUBSPOT_DEFINED (default label) or USER_DEFINED (custom label) and was introduced to the Hubspot connector in October 2024. See the [connector release notes](https://fivetran.com/docs/connectors/applications/hubspot/changelog#october2024) for more.

fivetran-avinash

@fivetran-reneeli Thanks for bearing all these comments! This is approved now.

One small comment, plus one suggestion: Can you update the title of this PR to describe what the PR is doing? I think for some reason it's taking the title of your commit.

fivetran-reneeli · 2024-11-11T18:51:13Z

@fivetran-reneeli Thanks for bearing all these comments! This is approved now.

One small comment, plus one suggestion: Can you update the title of this PR to describe what the PR is doing? I think for some reason it's taking the title of your commit.

Thanks for noticing that! Appreciate the review!

fivetran-reneeli added 3 commits November 1, 2024 15:31

versioning

69ea5e4

temp depedency and update changelog

8ee9fe1

i dont think docs regen is needed but just in case

8c98a25

fivetran-joemarkiewicz assigned fivetran-reneeli Nov 5, 2024

fivetran-joemarkiewicz self-requested a review November 5, 2024 15:24

fivetran-joemarkiewicz requested changes Nov 5, 2024

View reviewed changes

fivetran-reneeli added 2 commits November 5, 2024 16:14

ensure seed data from source is the same as in transforms

fb3c82f

updaet changelog

5d84090

fivetran-reneeli added 6 commits November 5, 2024 16:28

integration test fixes

e349f40

update seed configs

d59dec1

explicit cast for pipeline_id

959d264

more datatype casting

27b2961

docs

3caa4d5

new schema

ea641c5

fivetran-joemarkiewicz approved these changes Nov 7, 2024

View reviewed changes

fivetran-reneeli and others added 3 commits November 8, 2024 11:59

Update CHANGELOG.md

2b6950e

Co-authored-by: Joe Markiewicz <[email protected]>

rm category from merged_deal and comment out variables in integration…

02f3698

… test folder

new schema

f6f9e7d

fivetran-avinash reviewed Nov 8, 2024

View reviewed changes

updates

948dbfe

fivetran-avinash reviewed Nov 11, 2024

View reviewed changes

updates

753f5b3

fivetran-avinash reviewed Nov 11, 2024

View reviewed changes

fivetran-avinash approved these changes Nov 11, 2024

View reviewed changes

fivetran-reneeli changed the title ~~versioning~~ introduce category field upstream Nov 11, 2024

change deps

f180d8b

fivetran-reneeli merged commit 069d9ac into main Nov 11, 2024
9 checks passed

fivetran-reneeli deleted the bugfix/category_add branch November 11, 2024 20:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

introduce category field upstream #150

introduce category field upstream #150

fivetran-reneeli commented Nov 4, 2024 •

edited

Loading

fivetran-joemarkiewicz left a comment

fivetran-joemarkiewicz Nov 5, 2024

fivetran-reneeli Nov 5, 2024

fivetran-joemarkiewicz Nov 5, 2024

fivetran-joemarkiewicz Nov 5, 2024

fivetran-joemarkiewicz Nov 7, 2024

fivetran-reneeli commented Nov 5, 2024

fivetran-joemarkiewicz left a comment

fivetran-joemarkiewicz Nov 7, 2024

fivetran-joemarkiewicz Nov 7, 2024

fivetran-reneeli Nov 8, 2024

fivetran-joemarkiewicz Nov 7, 2024

fivetran-avinash Nov 8, 2024 •

edited

Loading

fivetran-avinash Nov 11, 2024

fivetran-reneeli Nov 11, 2024

fivetran-avinash left a comment

fivetran-avinash Nov 8, 2024

fivetran-reneeli Nov 11, 2024

fivetran-avinash Nov 8, 2024

fivetran-reneeli Nov 11, 2024

fivetran-avinash Nov 8, 2024

fivetran-reneeli Nov 11, 2024

fivetran-avinash Nov 8, 2024

fivetran-reneeli Nov 11, 2024

fivetran-avinash left a comment

fivetran-reneeli commented Nov 11, 2024

fivetran-avinash Nov 11, 2024

fivetran-avinash left a comment

fivetran-reneeli commented Nov 11, 2024

introduce category field upstream #150

introduce category field upstream #150

Conversation

fivetran-reneeli commented Nov 4, 2024 • edited Loading

PR Overview

Breaking Change (--full-refresh required after upgrading)

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fivetran-reneeli commented Nov 5, 2024

fivetran-joemarkiewicz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fivetran-avinash Nov 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fivetran-avinash left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fivetran-avinash left a comment

Choose a reason for hiding this comment

fivetran-reneeli commented Nov 11, 2024

Choose a reason for hiding this comment

fivetran-avinash left a comment

Choose a reason for hiding this comment

fivetran-reneeli commented Nov 11, 2024

fivetran-reneeli commented Nov 4, 2024 •

edited

Loading

Breaking Change (`--full-refresh` required after upgrading)

fivetran-avinash Nov 8, 2024 •

edited

Loading