IGNITE-28959 Calcite. Merge Join occasionally return wrong results

[zstan]

No description provided.

[Copilot]

Pull request overview

This PR targets an intermittent correctness issue in Calcite’s merge join execution by changing how MergeJoinNode schedules join processing and handles completion, and it extends test coverage to exercise correlated queries under different “disable join strategy” hints.

Changes:

• Updated MergeJoinNode input-push/end logic and removed the previous “finishing” / checkJoinFinished() completion path.
• Extended CorrelatesIntegrationTest#testCorrelatesCollision to run the same assertions under multiple join-disabling hints (to force alternate join strategies).
• Removed unused Parameterized JUnit runner annotation/imports from JoinBuffersExecutionTest.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/rel/MergeJoinNode.java	Alters merge join scheduling and termination logic (removes finishing state and changes when join() is invoked / when downstream is ended).
modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/integration/CorrelatesIntegrationTest.java	Re-runs correlate-collision assertions under multiple join-strategy-disabling hints.
modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/exec/rel/JoinBuffersExecutionTest.java	Removes unused Parameterized test runner annotation/imports.

Suppressed comments (5)

modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/rel/MergeJoinNode.java:558

• This join variant now ends downstream as soon as leftFinished() becomes true. For distributed joins, the class-level distributed contract indicates downstream should not be ended until both inputs are fully completed to avoid exchange inbox reopen/memory leak. Please reintroduce a finishing/draining mode for distributed execution (similar to the removed checkJoinFinished) before ending downstream.

            if (requested > 0 && leftFinished()) {
                requested = 0;
                downstream().end();

                return;
            }

modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/rel/MergeJoinNode.java:728

• This termination block now ends downstream immediately when rightFinished(true) is true. In distributed mode, MergeJoinNode’s distributed javadoc indicates the node should keep draining until both inputs end (to avoid exchange inbox reopen/memory leak). Consider reinstating the prior finishing/draining behavior for distributed joins instead of ending downstream here.

            if (requested > 0 && rightFinished(true)) {
                requested = 0;
                downstream().end();

                return;
            }

modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/rel/MergeJoinNode.java:937

• Full outer join now ends downstream immediately once both sides are finished, without any distributed-specific draining/cleanup path. Since the class-level distributed flag exists specifically to avoid premature downstream end when exchanges are involved, it would be safer to centralize termination/drain behavior (as the removed checkJoinFinished did) and ensure it is consistently applied across join types.

            if (requested > 0 && leftFinished() && rightFinished(true)) {
                requested = 0;
                downstream().end();

                return;
            }

modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/rel/MergeJoinNode.java:997

• Semi join now ends downstream immediately when either input is finished. In distributed mode, the MergeJoinNode distributed contract indicates downstream should only be ended after both inputs complete to prevent exchange inbox reopen/memory leak. Please restore a finishing/draining path for distributed execution rather than ending downstream here.

            if (requested > 0 && (leftFinished() || rightFinished(false))) {
                requested = 0;
                downstream().end();

                return;
            }

modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/rel/MergeJoinNode.java:1060

• Anti join now ends downstream immediately once leftFinished() is true. For distributed joins, the class-level distributed contract suggests the node must continue draining until both inputs end (to avoid exchange inbox reopen/memory leak). Consider reinstating the earlier finishing/draining logic rather than ending downstream here.

            if (requested > 0 && leftFinished()) {
                requested = 0;
                downstream().end();

                return;
            }

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Suppressed comments (1)

modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/integration/CorrelatesIntegrationTest.java:103

• The test is intended to cover merge-join correctness, but using a single NO_* hint per iteration doesn’t guarantee the planner will pick MergeJoin (it can still choose another join type). This can make the regression flaky/ineffective if the plan never uses MergeJoin. Consider explicitly forcing MergeJoin via the MERGE_JOIN hint (optionally combined with the current NO_* hint).

        for (HintDefinition noHint : List.of(NO_NL_JOIN, NO_CNL_JOIN, NO_HASH_JOIN)) {
            log.info(">>> Check with: " + noHint);
            // Collision by correlate variables in the left hand.
            assertQuery("SELECT /*+ %s */ * FROM test1 WHERE ".formatted(noHint) +
                "EXISTS(SELECT * FROM test2 WHERE test1.a=test2.a AND test1.b<>test2.c) " +

[ignitetcbot]

TCBot Test Analysis

• TeamCity: --> Run :: All Results
• Branch: pull/13455/head
• Base: master
• Tested commit: e23d2a9

Possible Blockers (0)

No blockers found.

New Tests (0)

No new tests found.