GH-48268: [C++][Acero] Enhance the type checking for hash join residual filter#48272
GH-48268: [C++][Acero] Enhance the type checking for hash join residual filter#48272zanmato1984 merged 1 commit intoapache:mainfrom
Conversation
| return filter; | ||
| }; | ||
|
|
||
| if (filter.IsBound()) { |
There was a problem hiding this comment.
IsBound() is implied by literal(true) so one check should suffice.
There was a problem hiding this comment.
I suppose there's already a unit test for that?
There was a problem hiding this comment.
Not any particular test, but most existing tests exercise this path, both if true and if false. So I think we are good.
| { | ||
| // Literal false, null, and scalar false, null. | ||
| for (Expression filter : | ||
| {literal(false), literal(NullScalar()), equal(literal(0), literal(1)), |
There was a problem hiding this comment.
The null type null (literal(NullScalar())) is invalid. Replacing it with a boolean type null.
github-actions
bot
added
awaiting committer review
|
This should fix the hash join test failure for big endian in #48166. cc @pitrou @Vishwanatha-HD |
pitrou
left a comment
pitrou
left a comment
There was a problem hiding this comment.
+1, thanks @zanmato1984 . Just a question below.
| return filter; | ||
| }; | ||
|
|
||
| if (filter.IsBound()) { |
There was a problem hiding this comment.
I suppose there's already a unit test for that?
|
After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 6f62c2a. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. It also includes information about 1 possible false positive for unstable benchmarks that are known to sometimes produce them. |
2 participants
Rationale for this change
Type checking for hash join filter isn't enforced for some corner cases (literal filter expression). Some invalid tests are introduced.
What changes are included in this PR?
Enforce the type checking for all cases. Also fix the problematic test cases. Also refined the trivial residual filter handling in swiss join.
Are these changes tested?
Test included.
Are there any user-facing changes?
None.