-
Notifications
You must be signed in to change notification settings - Fork 496
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
Safe subtract #748
Safe subtract #748
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.
this is looking great! sorry for the delay in getting back to you with a review 🌟
A couple of changes but overall very excited to get this in
@@ -0,0 +1,5 @@ | |||
field_1,field_2,field_3,expected |
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.
Wonderful 😍
macros/sql/safe_subtract.sql
Outdated
{%- if field_list is not iterable or field_list is string or field_list is mapping -%} | ||
|
||
{%- set error_message = ' | ||
Warning: the `safe_subtract` macro now takes a single list argument instead of \ | ||
string arguments. The {}.{} model triggered this warning. \ | ||
'.format(model.package_name, model.name) -%} | ||
|
||
{%- do exceptions.warn(error_message) -%} |
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.
In general, I don't think this note is necessary - we had it for safe_add
as part of its behaviour migration, but this is a net-new macro that is being built the right way.
If someone does {{ dbt_utils.safe_subtract('first', 'second') }}
it will render as coalesce(f, 0) - coalesce(i, 0), ...
which will send them back to double check the docs anyway.
So two options:
- Remove altogether
- Change it to an exception and don't say "now takes", because it never didn't take that form
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.
@joellabes That makes total sense to me. I think since the shift is still fairly new it makes sense to throw an exception and I have updated it accordingly, but let me know if you would prefer to remove altogether instead.
@joellabes looks like the redshift integration tests failed after my updates. Can you share the errors or any insights you may have about the reason? I'd also love to get this in. |
@joellabes Sorry not used to CI being publicly visible! I was able to access the details myself. It looks like the seed files aren't loading due to a permission error:
|
@dchess I'm looking into this! |
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.
Redshift is passing again, so we're in business! Thanks for getting this over the line @dchess
(And thanks to @dave-connors-3 and @dbeatty10 for the assist with CI here!)
resolves #745
This is a:
All pull requests from community contributors should target the
main
branch (default).Description & motivation
As discussed in #745, this new macro will provide a method for taking the difference across nullable fields similar in functionality to
safe_add()
. Currently it is necessary to multiply fields by -1 with thesafe_add()
macro or use coalesce statements in sql directly to achieve the same result.Checklist
star()
source)limit_zero()
macro in place of the literal string:limit 0
dbt.type_*
macros instead of explicit datatypes (e.g.dbt.type_timestamp()
instead ofTIMESTAMP