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

bug fix: account for incorrect comparisons involving null values #67

Merged

Conversation

leoebfolsom
Copy link
Contributor

@leoebfolsom leoebfolsom commented Mar 27, 2023

Description & motivation

Currently, the compare_all_columns macro does a poor job of distinguishing between missing data and null values.

We must check to see if the primary key exists in both of the two tables to determine whether the value in that table is null or missing.

With this update, the code checks that a primary key exists in both tables before declaring a perfect_match. This was already accounted for in other logical checks (e.g., null_in_a, null_in_b).

There are also errors in the conflicting_values logic leading to incorrectly counting conflicting_values as false when comparing a null to a not null value when the PKs are present in both tables.

Checklist

  • I have verified that these changes work locally
  • I have updated the README.md (if applicable)
  • I have added tests & descriptions to my models (and macros if applicable)

@leoebfolsom leoebfolsom changed the title account for comparison of null value to missing value bug fix: account for comparison of null value to missing value Mar 27, 2023
@leoebfolsom
Copy link
Contributor Author

Issue/bug report: #68

@leoebfolsom leoebfolsom changed the title bug fix: account for comparison of null value to missing value IN PROGRESS bug fix: account for comparison of null value to missing value Mar 27, 2023
@leoebfolsom leoebfolsom changed the title IN PROGRESS bug fix: account for comparison of null value to missing value bug fix: account for incorrect comparisons involving null values Mar 27, 2023
Copy link
Contributor

@joellabes joellabes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for adding the tests! 🚢

@joellabes joellabes merged commit 3d02772 into dbt-labs:main Mar 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants