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

Use EvaluateCondition for conditions in join statements #1926

Merged
merged 2 commits into from Aug 8, 2023

Conversation

adjeiv
Copy link
Contributor

@adjeiv adjeiv commented Aug 7, 2023

Fixes dolthub/dolt#6412
Use the given EvaluateCondition util function to determine whether a join condition is satisfied, which accounts for truthy integer values as mentioned in the linked issue.

return nil, err
} else if res == false {
} else if sql.IsFalse(res) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure whether this should be !sql.IsTrue like the other cases, but I've left it as IsFalse to keep to current behaviour

Copy link
Contributor

Choose a reason for hiding this comment

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

This should definitely be !sql.IsTrue so that it handles null cases correctly.

If you fix that then this PR looks perfect.

@adjeiv adjeiv changed the title Use EvaluateCondition for conditions in merge statements Use EvaluateCondition for conditions in join statements Aug 7, 2023
@nicktobey nicktobey self-requested a review August 7, 2023 19:43
@nicktobey
Copy link
Contributor

Thanks for the PR, @adjeiv! The PR looks good!

I'm reviewing it right now, just to make sure that the slightly different behavior around lateral join (which you noted in your comment) is correct.

@adjeiv adjeiv marked this pull request as ready for review August 7, 2023 20:54
@timsehn
Copy link
Sponsor Contributor

timsehn commented Aug 8, 2023

@nicktobey do we know why the tests failed here?

@nicktobey
Copy link
Contributor

nicktobey commented Aug 8, 2023

@timsehn

The windows tests are experiencing transient OOM issues. There was a discussion about how to fix them, I'll ask about that again. In the meantime, I don't think windows tests failing should block merges if the ubuntu and macos tests are passing.

@timsehn
Copy link
Sponsor Contributor

timsehn commented Aug 8, 2023

Shall we merge this then?

@nicktobey nicktobey merged commit 505d461 into dolthub:main Aug 8, 2023
5 of 6 checks passed
@adjeiv adjeiv deleted the join_integer_filter branch August 9, 2023 21:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect output for LEFT JOINS with integer-valued filter.
4 participants