[BugFix] Order PPLSimplifyDedupRule before FilterMergeRule in HEP program (#7)

[ryan-gh-bot]

Description

When a user where sits below dedup (with keepempty=false, the default), the synthetic IS NOT NULL filter that PPL emits for dedup ends up adjacent to the user filter above the scan. The HEP program in CalciteToolsHelper registered both FilterMergeRule and PPLSimplifyDedupRule via addRuleCollection, which lets HepPlanner schedule them in either order. When FilterMergeRule fires first, the two adjacent filters are merged into a single filter whose condition is AND(IS NOT NULL(field), <user predicate>). PPLSimplifyDedupRule's bottom operand calls mayBeFilterFromBucketNonNull, which only accepts a pure IS NOT NULL or AND of IS NOT NULLs — the merged condition fails the predicate, no LogicalDedup is produced, and the OpenSearch DedupPushdownRule has nothing to match. Dedup falls back to the row-number window form executed on the coordinator, which on large indices exceeds timeouts or triggers shard failures.

This change switches the HEP program to two sequential addRuleInstance calls (PPLSimplifyDedupRule first to fixpoint, then FilterMergeRule). Sequential addRuleInstance instructions guarantee deterministic ordering, so the simplify rule always sees the original adjacent-filter shape and dedup pushdown is preserved when a where clause is present.

Verification

Before (adversarial test demonstrating the failure mode — FilterMergeRule running before PPLSimplifyDedupRule):

LogicalProject(DEPTNO=[$7])
  LogicalFilter(condition=[<=($8, 1)])
    LogicalProject(EMPNO=[$0], ENAME=[$1], JOB=[$2], MGR=[$3], HIREDATE=[$4], SAL=[$5], COMM=[$6], DEPTNO=[$7], _row_number_dedup_=[ROW_NUMBER() OVER (PARTITION BY $7)])
      LogicalFilter(condition=[AND(>($5, 1000), IS NOT NULL($7))])
        LogicalTableScan(table=[[scott, EMP]])

The ROW_NUMBER window survives. mayBeFilterFromBucketNonNull rejects AND(>($5, 1000), IS NOT NULL($7)) because not every conjunct is IS NOT NULL, so PPLSimplifyDedupRule cannot fire and LogicalDedup is not produced. DedupPushdownRule has nothing to match in Volcano, so the dedup runs on the coordinator. This is captured by the new testFilterMergeBeforeSimplifyDedupBreaksPattern test in CalcitePPLDedupTest.

After (this branch, running the production HEP program from CalciteToolsHelper.optimize on a where + dedup + fields plan):

LogicalProject(DEPTNO=[$7])
  LogicalDedup(dedup_fields=[[$7]], allowed_dedup=[1], keepEmpty=[false], consecutive=[false])
    LogicalFilter(condition=[>($5, 1000)])
      LogicalTableScan(table=[[scott, EMP]])

LogicalDedup is produced. DedupPushdownRule can now match the LogicalDedup → LogicalProject → CalciteLogicalIndexScan pattern at Volcano time and push the dedup down as a composite aggregation + top_hits, restoring the same DSL shape that already worked without a where clause. Captured by the new testWhereThenDedupProducesLogicalDedup test.

Test run on this branch:

CalcitePPLDedupTest > testDedup1 PASSED
CalcitePPLDedupTest > testDedup2 PASSED
CalcitePPLDedupTest > testDedupExpr PASSED
CalcitePPLDedupTest > testDedupKeepEmpty1 PASSED
CalcitePPLDedupTest > testDedupKeepEmpty2 PASSED
CalcitePPLDedupTest > testFilterMergeBeforeSimplifyDedupBreaksPattern PASSED
CalcitePPLDedupTest > testRenameDedup PASSED
CalcitePPLDedupTest > testSortFieldProjectedAwayBeforeDedup PASSED
CalcitePPLDedupTest > testSortThenDedup PASSED
CalcitePPLDedupTest > testSortThenDedupWithEval PASSED
CalcitePPLDedupTest > testWhereThenDedupProducesLogicalDedup PASSED

Full :core:test, :ppl:test, and :opensearch:test runs all pass with no failures.

Related Issues

Resolves #7

Check List

• New functionality includes testing.
• New functionality has been documented.
• New PPL command checklist all confirmed.
• API changes companion pull request created.
• Commits are signed per the DCO using --signoff or -s.
• Public documentation issue/PR created.

---

This pull request was authored by @ryan-gh-bot in response to the maintainer command /fix on #7 (invoked by @RyanL1997). This is a maintainer-triggered automated contribution. Please review carefully before merging.

[RyanL1997]

thanks for the. change - just left some reviews

[RyanL1997]

@ryan-gh-bot looks like we are having duplicated javadoc here - can you verify and fix it?

[ryan-gh-bot]

Done — pushed bd848efb1.

[RyanL1997]

@ryan-gh-bot just for my knowledge why are we removing it?

[ryan-gh-bot]

That hepRuleList field only existed as the argument to the old addRuleCollection(hepRuleList) call. Once the builder switched to two sequential addRuleInstance(...) calls (which take single rules directly), nothing else referenced the list, so keeping it would just be dead code — and dropping it also lets the java.util.List and org.apache.calcite.plan.RelOptRule imports go away. The Javadoc that used to sit on the field moved up onto HEP_PROGRAM, which is now the thing whose ordering actually matters.

[RyanL1997]

@ryan-gh-bot not only here - can you avoid use inline import? we should do proper import for the class.

[ryan-gh-bot]

Done — pushed e9bcf1b49.