-
Notifications
You must be signed in to change notification settings - Fork 24
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/total queries #140
Bug/total queries #140
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 Great work! Conducted all the validation and it looks good on my end. Approved!
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 this looks good to me, I just have one quick approach related question before approving.
@@ -14,7 +14,7 @@ log_events as ( | |||
connector_id, | |||
cast( {{ dbt.date_trunc('day', 'created_at') }} as date) as date_day, | |||
event_subtype, | |||
message_data | |||
replace(message_data, 'totalQueries', 'total_queries') as message_data |
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.
Approach question, was there a reason we do these replace statements directly in the end models as opposed to the upstream staging model where we could do both in one place? Is the reason so we don't have nested replace statements (replace(replace(message_data, 'totalQueries', 'total_queries'),message_data, 'operationType', 'operation_type')
which could be hard to maintain.
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 question. My reason was thinking a staging model was a view, so downstream models might be needlessly performing this operation. However, I double checked and staging are tables not views. In light of that--should I move to the staging model or leave as-is do you think? @fivetran-joemarkiewicz
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 thought in not wanting downstream queries to be needlessly performing these operations if the staging modes are views. Truthfully, we should have the staging models be views instead of tables. Instead of making the above change, let's keep your approach. In addition, can you create a FR for us to update the materialization of the staging models in a future breaking change update. Since we want to get this PR out as soon as possible I would prefer we don't include that update now, but it will be good for us to consider in an upcoming release.
@@ -37,7 +41,7 @@ sync_log as ( | |||
*, | |||
{{ fivetran_log.fivetran_log_json_parse(string='message_data', string_path=['table']) }} as table_name, | |||
{{ fivetran_log.fivetran_log_json_parse(string='message_data', string_path=['schema']) }} as schema_name, | |||
{{ fivetran_log.fivetran_log_json_parse(string='message_data', string_path=['operationType']) }} as operation_type, | |||
{{ fivetran_log.fivetran_log_json_parse(string='message_data', string_path=['operation_type']) }} as operation_type, |
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.
Great catch here!
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 with request to create a FR for future materialization update.
PR Overview
This PR will address the following Issue/Feature:
This PR will result in the following new package version:
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:
Updated the seed to have both spellings of
total_queries
andtotalQueries
.I was able to reproduce the error using the updated seed data with the production version of the package.
The result shows both spellings produce totals.
Consistency and integrity tests pass
If you had to summarize this PR in an emoji, which would it be?
💃