-
Notifications
You must be signed in to change notification settings - Fork 112
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
Use an Artifact ID rather than a composite OR statment in several places. #81
Use an Artifact ID rather than a composite OR statment in several places. #81
Conversation
Co-authored-by: Niall Woodward <niall@niallrees.com>
-- This ID provides a reliable ID, regardless of whether running in a local or cloud environment. | ||
sha2_hex(coalesce(dbt_cloud_run_id, command_invocation_id), 256) as artifact_run_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.
@NiallRees - this is the bit that I'd love your view on.
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 idea. Linked issue: #53
select | ||
command_invocation_id, | ||
dbt_cloud_run_id | ||
select artifact_run_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.
Even if we're joining on this new key, can we keep the command_invocation_id
and dbt_cloud_run_id
in these models? Both are potentially useful to users.
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 nvm they weren't in the model output anyway.
In several places we had:
This PR replaces that idiom with a single identifier created in the base artifacts model, made from a coalesce of the two original IDs, then hashed. I think it's much cleaner and more future proof.
Would appreciate feedback on whether
SHA2
is overkill or whetherMD5
might be more pragmatic @NiallRees.