add precision + exclude_columns option to equality test

[rlh1994]

resolves #757, resolves #734, resolves #785, replaces #737, resolves #828

This is a:

• documentation update
• bug fix with no breaking changes
• new functionality
• a breaking change

All pull requests from community contributors should target the main branch (default).

Description & motivation

This adds an argument to the equality test that allows for floating point columns (column is_numeric or is_float) to be more easily compared and reducing false errors raised. It is fully backwards compatible (default arguments retain existing behaviour and code), and has been tested on and supports BQ/Postgres/Databricks/Snowflake/Redshift.

It also includes a few new tests both positive and negative assertions.

It also includes the work done in #737 to save rebasing if one of these was merged independently.

Finally, it adds a fix for #785 that ensures when no compare/ignore columns list is provided that both tables have the same columns.

Checklist

• This code is associated with an Issue which has been triaged and accepted for development.
• I have verified that these changes work locally on the following warehouses (Note: it's okay if you do not have access to all warehouses, this helps us understand what has been covered)
  • BigQuery
  • Postgres
  • Redshift
  • Snowflake
  • Databricks
• I followed guidelines to ensure that my changes will work on "non-core" adapters by:
  • dispatching any new macro(s) so non-core adapters can also use them (e.g. the star() source)
  • using the limit_zero() macro in place of the literal string: limit 0
  • using dbt.type_* macros instead of explicit datatypes (e.g. dbt.type_timestamp() instead of TIMESTAMP
• I have updated the README.md (if applicable)
• I have added tests & descriptions to my models (and macros if applicable)
• I have added an entry to CHANGELOG.md

[adammarples]

@rlh1994 this PR would be very useful to me. Any idea why the tests are failing? It looks like errors in seeding on redshift but it's not clear to me why.

[rlh1994]

@adammarples I don't think it's related to this change as it seems to be failing across multiple PRs, I was just waiting for @joellabes to review

[joellabes]

You and @brunocostalopes have done some great stuff here @rlh1994!

I particularly love the robust set of tests.

I will want to come back and do a deeper read of the specific jinja in the equality.sql file itself, but it makes sense to do that in one hit alongside any changes necessary with #785, as I imagine that'll lead to some changes too.

The good news is that the tests are now up and running again on Redshift, so we should be able to move much faster and more confidence.

[rlh1994]

@joellabes I've added it in, but a few notes on this final change:

1. I've made it a separate block, which means that currently it hits the warehouse twice for the model to get the columns - I tried to do this as part of the normal get_columns... part but it seems like set() alters the object it is acting on (at least they way I was using it for the generator that's returned) which cause a load of new failures when it shouldn't have. I'm not aware if there is the equivalent of a deep_copy in dbt/jinja so I've done it this way just to be sure.
2. It feels like this should actually raise a compiler error rather than fail the test itself, but I wasn't sure how to add tests for such a case, let me know your preference on this and I can swap it out to raise an error instead.
3. I think this would now qualify as a breaking change, as previously passing tests would now fail. A workaround would be to add a new argument to the macro to enable/disable this check and set the default (until a breaking change version is released) to be false.
4. I had to make a change to one of the other tests as this highlighted that the tables had different columns (which means it works!)

[nachimehta]

also looking forward to this, at the moment the equality silently passes if your comparison model has more columns than the tested model, which seems like unexpected behavior. I believe your fix rectifies this issue.

[joellabes]

• I'm not aware if there is the equivalent of a deep_copy in dbt/jinja so I've done it this way just to be sure.

Me either, I think this is OK

• It feels like this should actually raise a compiler error rather than fail the test itself, but I wasn't sure how to add tests for such a case, let me know your preference on this and I can swap it out to raise an error instead.

Yeah it should raise a compiler error. I think that means we can't have a test running for it 😬 but better to do the right thing on the end user's side.

• I think this would now qualify as a breaking change, as previously passing tests would now fail. A workaround would be to add a new argument to the macro to enable/disable this check and set the default (until a breaking change version is released) to be false.

They’ll fail because the data is invalid though, right? So I am OK with that - it's a bug that we're fixing as opposed to deviating from documented behaviour

4. I had to make a change to one of the other tests as this highlighted that the tables had different columns (which means it works!)

Ha! Great news 🎉

Will go over this properly soon

[rlh1994]

Hey @joellabes just wondering if you know when you'll have a chance to look at this please?

[rlh1994]

Redshift failing due to a deadlock, not a failing test

[brunocostalopes]

@rlh1994 I noticed you've updated the equality test and renamed the arguments from ignore to exclude, just a note that when discussing with @joellabes he was of the opinion that we should name them "ignore" rather than "exclude". See here: #734 (comment)

[rlh1994]

@rlh1994 I noticed you've updated the equality test and renamed the arguments from ignore to exclude, just a note that when discussing with @joellabes he was of the opinion that we should name them "ignore" rather than "exclude". See here: #734 (comment)

Ah okay, I went with exclude based on this comment here: #829 (comment)

To be honest I have no strong feelings either way and can easily change it once this gets reviewed 😅

[brunocostalopes]

Oh, I missed that PR. So we now have the same feature implemented in #737 (my original PR) #765 (which includes those changes) and #829 (new PR). I wonder if any of these will ever get merged :(

[seub]

@brunocostalopes Sorry I had missed your PR as well! It appears that there's a real need for these ignore/exclude columns! ;)

It would indeed help avoid a stack of redundant PRs if some of them finally got reviewed/approved @joellabes

[gwenwindflower]

Looks good! Left one comment for consideration but otherwise this seems good to me and everything is passing now in CI. Thanks again for your patience on this!

[gwenwindflower]

Love where you landed with the new comments @rlh1994 ! Thanks again for the patience, this is a fantastic addition to the package, really appreciate the work on this one. 💜

[jomccr]

This has already helped me a ton, thanks for this contribution!