Skip to content
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

Change of behavior in 1.2 makes passing tests with 1.1 fail with 1.2 #923

Closed
b-per opened this issue Jun 26, 2024 · 3 comments
Closed

Change of behavior in 1.2 makes passing tests with 1.1 fail with 1.2 #923

b-per opened this issue Jun 26, 2024 · 3 comments
Labels
bug Something isn't working wontfix

Comments

@b-per
Copy link
Contributor

b-per commented Jun 26, 2024

Describe the bug

The fix of the issue #785 in the PR #765 introduces a change of behavior when the 2 tables of the comparison do not contain the same columns.

This led to the fact that integration tests in dbt_project_evaluator passed with dbt_utils 1.1 but failed with 1.2

I wouldn't have expected a minor bump to change the behavior.

@b-per b-per added bug Something isn't working triage labels Jun 26, 2024
@b-per b-per changed the title Change of behaviour in 1.2 makes passing tests with 1.1 fail with 1.2 Change of behavior in 1.2 makes passing tests with 1.1 fail with 1.2 Jun 26, 2024
@dbeatty10
Copy link
Contributor

I see what you are saying in regards to behavior change @b-per:

  • In 1.1, there was not an error when the column names didn't match
  • In 1.2, there is an error when the column names don't match

It looks like this is the root cause:

  • Equality test will now raise an error when the second model has less columns than the first by @rlh1994 in #765

Out of curiosity, what does it do in 1.1 if you move the equality test from the seed to the model?

i.e., move this test to here instead (making sure to remove the exclude_columns part)? Does it error in 1.1? Or does it still succeed?

@b-per
Copy link
Contributor Author

b-per commented Jun 27, 2024

In 1.1 changing the side of the comparison fails with this error:

Binder Error: Referenced column "resource_type" not found in FROM clause!

as we were expecting.

I am not saying that we should revert #785 but I guess this is more of a discussion whether dbt_utils.equality working when the main table has less columns than the compared one was the intended behavior or an unintended and untested one.

@dbeatty10
Copy link
Contributor

TLDR

The original behavior looks like an oversight to me rather than an intentional decision that fully considered all the implications.

Context

The behavior of dbt_utils.equality working when the main table has less columns than the compared one has existed since the 2nd commit in dbt utils: 948dd9b

I don't know if it was intended or unintended, but I buy the argument in #785 that by default dbt_utils.equality should have always held the following properties:

  • commutativity
  • same definition as equality in set theory:
    • A=B if and only if:
      • A is a subset of B
      • B is a subset of A

Behavior change

This behavior did change in 1.2.0, but we're considering this a backward-compatible bug fix: an internal change that fixes incorrect behavior. Since it didn't introduce any backward incompatible API changes, this doesn't require bumping the major version.

dbt 1.7+ has predictable package installs by default via package-lock.yml which should help mitigate any pre-existing projects breaking overnight in a surprising way.

Summary

Upgrading to dbt-utils 1.2.0 may require code changes for some projects, but we're considering this a good thing because it fixes a bug.

@dbeatty10 dbeatty10 closed this as not planned Won't fix, can't repro, duplicate, stale Jun 27, 2024
@dbeatty10 dbeatty10 added wontfix and removed triage labels Jun 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working wontfix
Projects
None yet
Development

No branches or pull requests

2 participants