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

Fix check_seeds macro #4146

Merged
merged 2 commits into from
Aug 23, 2023
Merged

Fix check_seeds macro #4146

merged 2 commits into from
Aug 23, 2023

Conversation

aalan3
Copy link
Contributor

@aalan3 aalan3 commented Aug 23, 2023

We need to go through some tests and see if they are still correct. This type of error is very subtle since NULL = false in SQL becomes just NULL.

@aalan3
Copy link
Contributor Author

aalan3 commented Aug 23, 2023

@0xRobin do you have any insight on this? (since you worked on this previously in #3676). I think the legacy version of this check result_model = expected_seed should have resulted in pretty much the same issue.

@0xRobin
Copy link
Collaborator

0xRobin commented Aug 23, 2023

ah yes you're right, the check is not null-safe (just as the equality operator is not).
I propose we follow trino docs here and start using the null-safe IS DISTINCT FROM or IS NOT DISTINCT FROM
https://trino.io/docs/current/functions/comparison.html#is-distinct-from-and-is-not-distinct-from
(as well as changing the legacy version to use <=>, the spark equivalent)
https://spark.apache.org/docs/3.0.0-preview/sql-ref-null-semantics.html#comparision-operators-

We might have missed some issues without knowing due to this.
Might be best to do a full test run on spark and trino to see if we have failing tests?

@aalan3
Copy link
Contributor Author

aalan3 commented Aug 23, 2023

Yes we should do a full test run to try to find issues. We currently only test when something is changed but we can do a full test in CI.

I'll do a separate PR for legacy since CI is breaking here.

@aalan3 aalan3 merged commit 16243c1 into main Aug 23, 2023
1 of 3 checks passed
@aalan3 aalan3 deleted the check-seeds-macro branch August 23, 2023 15:26
@github-actions github-actions bot locked and limited conversation to collaborators Aug 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants