-
Notifications
You must be signed in to change notification settings - Fork 25
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
Bug/redshift json parse #114
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.
@fivetran-catfritz Thanks so much for working through this update!! I have only a few final questions to be addressed below. Coincidentally, when testing these changes I ran into a real world scenario where I had late arriving sync events from Fivetran. If we were using the old method those late events would have been missed; however, with this update they were captured successfully!
Additionally, one last request would be to doubly confirm with the original customers data that the JSON fix proposal will address this issue. I have 99% confidence that this does, but it would be great to fully validate that the compiled code works as expected with the customers data.
Let me know if you have any questions!
|
||
{% macro default__fivetran_log_lookback(from_date, datepart='day', interval=7, default_start_date='2010-01-01') %} | ||
|
||
coalesce( |
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.
This may be a strange question, but what scenario would we need a default_start_date? Shouldn't there always by a max(date)
on incremental runs? What scenario would there be where we need the coalesce to select the default start date?
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.
Good point. I trialed removing this and it seems to compile and run fine. However for now I will leave this pending discussion on Tuesday.
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, I just wanted to make sure there likely wouldn't be a scenario where this field would actually be null
. I don't believe including it will hurt so I am comfortable leaving it in.
Co-authored-by: Joe Markiewicz <[email protected]>
Co-authored-by: Joe Markiewicz <[email protected]>
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-catfritz thanks again for working through these updates! This PR looks good to go!
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
PR Overview
This PR will address the following Issue/Feature:
This PR will result in the following new package version:
json_parse
macro and also adding the lookback for the incrementalfivetran_platform__audit_table
. Though tested, these are updates that could introduce unexpected behavior.Please provide the finalized CHANGELOG entry which details the relevant changes included in this PR:
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:
If you had to summarize this PR in an emoji, which would it be?
🔍