Repository navigation
Conversation
ca084e2 to
0c3d7fe
Compare
|
bvolpato
changed the title
|
LGTM. |
|
@rubenada you have LGTM but not approved officially. Still waiting for the nit? |
mihaibudiu
added
the
LGTM-will-merge-soon
It's not a blocking issue. |
vlsi
left a comment
vlsi
left a comment
There was a problem hiding this comment.
Suggestions, none blocking:
-
Please add an end-to-end test next to
EnumerableJoinTest.testMergeJoinAntiWithCompositeKeyAndNullValues. That test covers only the composite key, which already kept NULL keys, so nothing at the SQL level pins this fix. This test fails on the base commit (was "empid=100\nempid=110") and passes with the PR:/** Test case for * <a href="https://issues.apache.org/jira/browse/CALCITE-7817">[CALCITE-7817] * Enumerable merge ANTI join drops left rows with NULL keys</a>. */ @Test void testMergeJoinAntiWithSingleKeyAndNullValues() { tester(false, new HrSchema()) .withHook(Hook.PLANNER, (Consumer<RelOptPlanner>) planner -> { planner.addRule(EnumerableRules.ENUMERABLE_MERGE_JOIN_RULE); planner.removeRule(EnumerableRules.ENUMERABLE_JOIN_RULE); }) .withRel(builder -> builder .scan("s", "emps").as("e1") .sort(builder.field("commission")) .scan("s", "emps").as("e2") .filter(builder.equals(builder.field("deptno"), builder.literal(20))) .sort(builder.field("commission")) .antiJoin( builder.equals(builder.field(2, 0, "commission"), builder.field(2, 1, "commission"))) .project(builder.field("empid")) .build()) .explainHookContains("EnumerableMergeJoin") .returnsUnordered("empid=100", "empid=110", "empid=150"); }
-
The commit subject lacks the
[CALCITE-7817]prefix. It would also help to state in the commit or PR description that the existing ANTI expectations inEnumerablesTest(for example[3]→[3, null], and[]→[null]for "Both empty") encoded the bug.
Add an EnumerableMergeJoin ANTI join test with a single nullable key. The existing end-to-end test uses a composite key, which already kept NULL keys. Move the repeated NULL key cases into the ANTI section of EnumerablesTest.testMergeJoin3. Add a comment to each case that names the code path it exercises. Correct two comments in EnumerableDefaults that said a NULL key always ends the merge join. LEFT and ANTI joins continue to process left rows. The previous ANTI expectations in EnumerablesTest encoded the bug. For example, "[3]" is now "[3, null]", and "Both empty" with NULL keys is now "[null]" instead of "[]".
|
Addressed the review suggestions in 03486d7.
The subject of the first commit still lacks the |
|
5 participants
Jira Link
CALCITE-7817
Changes Proposed
Enumerable merge ANTI joins currently stop processing left rows when they reach a NULL join key. Under strict equality, NULL keys do not match and those left rows must survive the ANTI join.
Preserve left NULL keys in all three merge-join advancement paths.
The existing ANTI expectations in
EnumerablesTestencoded the bug, so this PR corrects them. For example,[3]is now[3, null], and "Both empty" with NULL keys is now[null]instead of[].The ANTI section of
EnumerablesTest.testMergeJoin3has two new cases with repeated NULL keys. One case reaches the NULL keys after the right input runs out. The other case reaches them insideadvanceLeft, immediately after a match. The tests also cover empty inputs, NULL keys on the right, and the overload with an additional predicate.EnumerableJoinTest.testMergeJoinAntiWithSingleKeyAndNullValuescovers the fix end to end. The existing end-to-end ANTI test uses a composite key, which already kept NULL keys.Reproduction
With ascending, NULLS LAST inputs, an equality ANTI join of left
[1, 2, NULL]and right[1]returns[2]. The correct result is[2, NULL]../gradlew :core:test --tests 'org.apache.calcite.runtime.EnumerablesTest'Validation
Both runs used Gradle in a JDK 21 container.
EnumerablesTestruns 53 tests with 1 skipped, andEnumerableJoinTestruns 13 tests.EnumerableDefaults.javafrom the base commit, 4 tests fail: 3 methods inEnumerablesTest, andtestMergeJoinAntiWithSingleKeyAndNullValueswithwas "empid=100\nempid=110".:linq4j:autostyleCheck,:core:autostyleCheck,:linq4j:checkstyleMain,:core:checkstyleTest, andgit diff --checkpass.