-
Notifications
You must be signed in to change notification settings - Fork 34
[Bug Fix] Ticket pipeline id casting #151
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
Conversation
* feature/is-closed-support * additional updates * Q2 FY26 Automatic Package Updates (#147) * Q2 FY26: Apply automated update. * Update README.md --------- Co-authored-by: fivetran-catfritz <[email protected]> Co-authored-by: Joe Markiewicz <[email protected]> * Update CHANGELOG.md * Q2 FY26: Update auto-release workflow only. (#149) Co-authored-by: fivetran-catfritz <[email protected]> * Generate dbt docs via GitHub Actions * Apply suggestions from code review Co-authored-by: Jamie Rodriguez <[email protected]> * Update CHANGELOG.md * Generate dbt docs via GitHub Actions --------- Co-authored-by: Fivetran Maintainer Bot <[email protected]> Co-authored-by: fivetran-catfritz <[email protected]> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: Jamie Rodriguez <[email protected]>
…/fivetran/dbt_hubspot_source into bugfix/ticket-pipeline-id-casting
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.
Overall looks good, but I think we also need to explicitly cast ticket.ticket_pipeline_id
and ticket. ticket_pipeline_stage_id
as strings here due to these joins downstream. Seeing No matching signature for operator = for argument types: INT64, STRING Signature: T1 = T1
failures when running this on some internal data
CHANGELOG.md
Outdated
| [`stg_hubspot__ticket_pipeline_stage`](https://fivetran.github.io/dbt_hubspot_source/#!/model/model.hubspot_source.stg_hubspot__ticket_pipeline_stage) | Column datatype | `ticket_pipeline_id` (`int`)<br/> `ticket_pipeline_stage_id` (`int`) | `ticket_pipeline_id` (`string`)<br/> `ticket_pipeline_stage_id` (`string`) | Reverted ticket pipeline identifiers back to their original string format to prevent datatype errors in compilation. [PR #151](https://github.com/fivetran/dbt_hubspot_source/pull/151) | | ||
|
||
## Under the Hood | ||
- Updated seed files to validate that the new identifiers compile as expected in both the source and transform packages.[PR #151](https://github.com/fivetran/dbt_hubspot_source/pull/151) |
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.
- Updated seed files to validate that the new identifiers compile as expected in both the source and transform packages.[PR #151](https://github.com/fivetran/dbt_hubspot_source/pull/151) | |
- Updated seed files to validate that the new identifiers compile as expected in both the source and transform packages. [PR #151](https://github.com/fivetran/dbt_hubspot_source/pull/151) |
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.
Committed
models/src_hubspot.yml
Outdated
NOTE: This field is deprecated in favor of the `is_closed` column. | ||
- name: is_closed | ||
description: Boolean indicating if the pipeline stage is closed. | ||
description: Boolean indicating if the pipeline stage is closed. |
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.
description: Boolean indicating if the pipeline stage is closed. | |
description: Boolean indicating if the pipeline stage is closed. |
Since trailing whitespace is becoming an issue for python 3.12
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.
Removed whitespace
models/src_hubspot.yml
Outdated
Boolean indicating if the pipeline stage is closed. | ||
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.
Boolean indicating if the pipeline stage is closed. | |
Boolean indicating if the pipeline stage is closed. | |
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.
Removed whitespace
models/stg_hubspot__deal.yml
Outdated
- name: is_closed | ||
description: Boolean indicating if the pipeline stage is closed. For the state of the closed deal ("Closed Won" vs "Closed Lost"), refer to the pipeline_stage_label column. |
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.
- name: is_closed | |
description: Boolean indicating if the pipeline stage is closed. For the state of the closed deal ("Closed Won" vs "Closed Lost"), refer to the pipeline_stage_label column. | |
- name: is_closed | |
description: Boolean indicating if the pipeline stage is closed. For the state of the closed deal ("Closed Won" vs "Closed Lost"), refer to the pipeline_stage_label column. |
To avoid introducing any whitespace issues
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.
Removed whitespace
|
||
select | ||
cast(pipeline_id as {{ dbt.type_int() }} ) as ticket_pipeline_id, | ||
pipeline_id as ticket_pipeline_id, |
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.
Is this guaranteed to always come in as a string? Wondering if we should do a cast(pipeline_id as {{ dbt.type_string() }} )
just in case
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.
Updated to cast as strings.
pipeline_id as ticket_pipeline_id, | ||
stage_id as ticket_pipeline_stage_id, |
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.
Same cast-as-string question as above
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.
Updated to cast as strings.
…m/fivetran/dbt_hubspot_source into bugfix/ticket-pipeline-id-casting
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.
@fivetran-jamie Agreed that explicitly casting all ticket pipeline identifiers as a string makes sense given the errors you're seeing. Applied them and resolved the errors you're seeing.
Ready for re-review!
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 great! just some final questions/comments in the changelog
CHANGELOG.md
Outdated
| Data Models | Change Type | Old Behavior | New Behavior | Notes | | ||
| --- | --- | --- | --- | --- | | ||
| [`stg_hubspot__deal_pipeline_stage`](https://fivetran.github.io/dbt_hubspot_source/#!/model/model.hubspot_source.stg_hubspot__deal_pipeline_stage) | Column Renamed | `is_closed_won` | `is_closed` | Column renamed to more accurately match the source data. The renamed `is_closed` column is generated via a coalesce between the previous `closed_won` source column and the newly added `is_closed` column. This renamed column represents whether a deal is closed, regardless of its label as “Closed Won” or “Closed Lost.” You can still use the `pipeline_stage_label` column to differentiate between "Closed Won" and "Closed Lost" stages. ([PR #146](https://github.com/fivetran/dbt_hubspot_source/pull/146)) | | ||
| [`stg_hubspot__ticket`](https://fivetran.github.io/dbt_hubspot_source/#!/model/model.hubspot_source.stg_hubspot__ticket) | Column datatype | `ticket_pipeline_id` (numeric string)<br/> `ticket_pipeline_stage_id` (numeric string) | `ticket_pipeline_id` (`string`)<br/> `ticket_pipeline_stage_id` (`string`) | Explicitly cast ticket pipeline identifiers to string format to prevent datatype errors in compilation; if the values in `ticket` were entirely numerics, the data type could be erroneously interpreted as an integer. ([PR #151](https://github.com/fivetran/dbt_hubspot_source/pull/151)) | |
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.
numeric string
for the old data types is a lil confusing -- would int
be more appropriate? In a similar vein, regarding the below note, do you know for certain that these ticket
fields ever come in as strings?
if the values in
ticket
were entirely numerics, the data type could be erroneously interpreted as an integer.
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.
Updated after our sync.
models/stg_hubspot__deal.yml
Outdated
description: Whether the pipeline stage is currently in use. | ||
- name: is_closed | ||
description: Boolean indicating if the pipeline stage is closed. For the state of the closed deal ("Closed Won" vs "Closed Lost"), refer to the pipeline_stage_label column. | ||
description: Boolean indicating if the pipeline stage is closed. For the state of the closed deal ("Closed Won" vs "Closed Lost"), refer to the pipeline_stage_label column. |
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.
description: Boolean indicating if the pipeline stage is closed. For the state of the closed deal ("Closed Won" vs "Closed Lost"), refer to the pipeline_stage_label column. | |
description: Boolean indicating if the pipeline stage is closed. For the state of the closed deal ("Closed Won" vs "Closed Lost"), refer to the pipeline_stage_label column. |
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.
Removed whitespace.
…m/fivetran/dbt_hubspot_source into bugfix/ticket-pipeline-id-casting
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.
LGTM!
CHANGELOG.md
Outdated
| [`stg_hubspot__ticket`](https://fivetran.github.io/dbt_hubspot_source/#!/model/model.hubspot_source.stg_hubspot__ticket) | Column datatype | `ticket_pipeline_id` (`int` or `string`)<br/> `ticket_pipeline_stage_id` (`int` or `string`) | `ticket_pipeline_id` (`string`)<br/> `ticket_pipeline_stage_id` (`string`) | Explicitly cast ticket pipeline identifiers to string format to prevent datatype errors in compilation. ([PR #151](https://github.com/fivetran/dbt_hubspot_source/pull/151)) | | ||
| [`stg_hubspot__ticket_pipeline`](https://fivetran.github.io/dbt_hubspot_source/#!/model/model.hubspot_source.stg_hubspot__ticket_pipeline) | Column datatype | `ticket_pipeline_id` (`int`) | `ticket_pipeline_id` (`string`) | Changed cast of ticket pipeline identifier to the original string format to prevent datatype errors in compilation. ([PR #151](https://github.com/fivetran/dbt_hubspot_source/pull/151)) | | ||
| [`stg_hubspot__ticket_pipeline_stage`](https://fivetran.github.io/dbt_hubspot_source/#!/model/model.hubspot_source.stg_hubspot__ticket_pipeline_stage) | Column datatype | `ticket_pipeline_id` (`int`)<br/> `ticket_pipeline_stage_id` (`int`) | `ticket_pipeline_id` (`string`)<br/> `ticket_pipeline_stage_id` (`string`) | Changed cast of ticket pipeline identifiers to their original string format to prevent datatype errors in compilation. ([PR #151](https://github.com/fivetran/dbt_hubspot_source/pull/151)) | |
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.
Since we have multiple columns with the same change, I suggest to format these lines like in this release to help with readability.
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.
Updated!
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.
One more small suggestion I left, but looks good for release!
Co-authored-by: fivetran-catfritz <[email protected]>
…m/fivetran/dbt_hubspot_source into bugfix/ticket-pipeline-id-casting
PR Overview
Package version introduced in this PR:
This PR addresses the following Issue/Feature(s):
Summary of changes:
Submission Checklist
Changelog