-
Notifications
You must be signed in to change notification settings - Fork 29
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
make patch update to account for upstream changes #182
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.
Same as source -- looks pretty good but can you regen the docs to include the macro update?
And my question regarding the constant expression error stands -- was this only coming up when ticket_field_history
was missing?
packages.yml
Outdated
# - package: fivetran/zendesk_source | ||
# version: [">=0.14.0", "<0.15.0"] | ||
- package: calogica/dbt_date | ||
version: [">=0.9.0", "<1.0.0"] No newline at end of file | ||
version: [">=0.9.0", "<1.0.0"] | ||
|
||
- git: https://github.com/fivetran/dbt_zendesk_source.git | ||
revision: bugfix/redshift_empty_tables | ||
warn-unpinned: false |
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.
reminder to switch to v0.15.* before merging
Thanks @fivetran-jamie ! Regenerated the docs. Let's keep correspondence re: ticket_field_history in the source |
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
@@ -1,3 +1,10 @@ | |||
# dbt_zendesk v0.20.0 | |||
|
|||
## [Upstream Under-the-Hood Updates](https://github.com/fivetran/dbt_zendesk_source/blob/main/CHANGELOG.md) from `zendesk_source` Package |
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.
Upstream and "from zendesk_source
Package" is redundant. I'd suggest keeping one or the other but not both.
I'd link directly to the release notes for the source package rather than the CHANGELOG, as the CHANGELOG will change over time. Either that or the release hyperlink (https://github.com/fivetran/dbt_zendesk/blob/main/CHANGELOG.md#dbt_zendesk_source-v0200) I think will be the link? Might want to check.
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.
thanks! updated
@@ -1,3 +1,5 @@ | |||
# Zendesk Support Modeling dbt Package ([Docs](https://fivetran.github.io/dbt_zendesk/)) |
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.
Should we move the H1 heading in the dbt_zendesk_source
package too? No worries if it's out of scope.
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.
yes good reminder
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.
done!
packages.yml
Outdated
@@ -1,5 +1,10 @@ | |||
packages: | |||
- package: fivetran/zendesk_source | |||
version: [">=0.14.0", "<0.15.0"] | |||
# - package: fivetran/zendesk_source |
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.
Reminder to switch deps before merging.
- (Affects Redshift only) Updates the `union_zendesk_connections` macro to use a limit 1 instead of limit 0 for empty tables. | ||
- When a table is empty, Redshift ignores explicit data casts and will materialize every column as a `varchar`. Redshift users may experience errors in downstream transformations as a consequence. | ||
- For each staging model, if the source table is not found, the package will create a empty table with 0 rows for non-Redshift warehouses and a table with 1 all-`null` row for Redshift destinations. The 1 row will ensure that Redshift will respect the package's datatype casts. | ||
|
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.
Should we create a Documentation section to mention the movement of the H1 heading?
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.
Hmm I don't see other release notes in other packages mentioning this, and it's so minor I don't think it would have much impact?
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.
Oh I guess I just added them in my recent ones. https://github.com/fivetran/dbt_zuora/releases/tag/v0.3.1
But it's not a big deal either way.
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.
A few comments and suggestions to consider, but otherwise approved pending the current quickstart.yml issue.
PR Overview
This PR will address the following Issue/Feature:
Internal ticket where empty tables using Redshift will ignore explicit data casts and cast everything as varchar by default, which presents problems downstreams in transformations. By creating a table with 1 row with all null records for each empty source table, this ensures that Redshift keeps the explicit datatype casts.
This PR will result in the following new package version: v0.20.0
Not a breaking change in itself, but needed to upgrade the downstream transforms package so it can be picked up by Quickstart.
Please provide the finalized CHANGELOG entry which details the relevant changes included in this PR:
Upstream Under-the-Hood Updates from
zendesk_source
Packageunion_zendesk_connections
macro to use a limit 1 instead of limit 0 for empty tables.varchar
. Redshift users may experience errors in downstream transformations as a consequence.null
row for Redshift destinations. The 1 row will ensure that Redshift will respect the package's datatype casts.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 share any and all of your validation steps:
Recreate the issue using an empty schema on Redshift and running the transforms package on prod:
Running on dev:
(although the constant expressions error is still present)
If you had to summarize this PR in an emoji, which would it be?
💃