vlsi commented on code in PR #5242:
URL: https://github.com/apache/calcite/pull/5242#discussion_r4101272514
##########
core/src/test/java/org/apache/calcite/runtime/EnumerablesTest.java:
##########
@@ -342,6 +351,21 @@ class EnumerablesTest {
JoinType.LEFT);
}
+ @Test void testMergeAntiJoinWithRepeatedNullKeys() {
Review Comment:
The two cases exercise different paths: `[1, 2, null, null]` vs `[1]`
reaches the NULL keys after the right input runs out, and `[1, null, null]` vs
`[1, 2]` reaches them inside `advanceLeft` right after a match. A one-line
comment above each case would say so; the repetition of NULL itself is not what
they test. This fits with @rubenada's suggestion to move them into the ANTI
section of the existing method.
##########
linq4j/src/main/java/org/apache/calcite/linq4j/EnumerableDefaults.java:
##########
@@ -5125,7 +5125,7 @@ private boolean advance() {
// mergeJoin assumes inputs sorted in ascending order with nulls
last,
// if we reach a null key, we are done.
Review Comment:
This comment still says "if we reach a null key, we are done", but the next
line continues for LEFT and ANTI.
##########
linq4j/src/main/java/org/apache/calcite/linq4j/EnumerableDefaults.java:
##########
@@ -5234,9 +5234,10 @@ private boolean advanceLeft(TSource left, TKey leftKey) {
while (getLeftEnumerator().moveNext()) {
left = getLeftEnumerator().current();
TKey leftKey2 = outerKeySelector.apply(left);
- if (leftKey2 == null && joinType != JoinType.LEFT) {
+ if (leftKey2 == null && !isLeftOrAntiJoin()) {
Review Comment:
The `@return` of `advanceLeft` still says it returns `false` when a "null
key is found". For LEFT and ANTI a NULL key no longer ends this loop. It was
already wrong for LEFT, but the PR edits the neighboring comment, so it could
fix this one too.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
