From 52f45f9c59340e7d58ffadd992dfd3cb9519d6ed Mon Sep 17 00:00:00 2001 From: ryan-gh-bot Date: Thu, 28 May 2026 22:21:16 +0000 Subject: [PATCH 1/3] [BugFix] Order PPLSimplifyDedupRule before FilterMergeRule in HEP program (#7) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When a user WHERE sits below dedup, the synthetic IS NOT NULL filter that PPL emits as part of dedup keepempty=false 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), ); PPLSimplifyDedupRule's bottom operand calls mayBeFilterFromBucketNonNull, which only accepts a pure IS NOT NULL or AND-of-IS-NOT-NULLs, so 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. Switch 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. Signed-off-by: ryan-gh-bot --- .../sql/calcite/utils/CalciteToolsHelper.java | 26 ++++-- .../ppl/calcite/CalcitePPLAbstractTest.java | 15 ++++ .../sql/ppl/calcite/CalcitePPLDedupTest.java | 81 +++++++++++++++++++ 3 files changed, 115 insertions(+), 7 deletions(-) diff --git a/core/src/main/java/org/opensearch/sql/calcite/utils/CalciteToolsHelper.java b/core/src/main/java/org/opensearch/sql/calcite/utils/CalciteToolsHelper.java index 54b9d4ffbaf..966d587a977 100644 --- a/core/src/main/java/org/opensearch/sql/calcite/utils/CalciteToolsHelper.java +++ b/core/src/main/java/org/opensearch/sql/calcite/utils/CalciteToolsHelper.java @@ -36,7 +36,6 @@ import java.sql.PreparedStatement; import java.sql.SQLException; import java.time.Instant; -import java.util.List; import java.util.Properties; import java.util.function.Consumer; import org.apache.calcite.adapter.enumerable.EnumerableConvention; @@ -59,7 +58,6 @@ import org.apache.calcite.plan.Convention; import org.apache.calcite.plan.RelOptCluster; import org.apache.calcite.plan.RelOptPlanner; -import org.apache.calcite.plan.RelOptRule; import org.apache.calcite.plan.RelOptSchema; import org.apache.calcite.plan.RelOptTable; import org.apache.calcite.plan.RelOptTable.ViewExpander; @@ -551,12 +549,26 @@ public RelNode visit(TableScan scan) { } } - /** Try to optimize the plan by using HepPlanner */ - private static final List hepRuleList = - List.of(FilterMergeRule.Config.DEFAULT.toRule(), PPLSimplifyDedupRule.DEDUP_SIMPLIFY_RULE); - + /** + * Try to optimize the plan by using HepPlanner. + * + *

Rule order matters: {@link PPLSimplifyDedupRule#DEDUP_SIMPLIFY_RULE} must run to fixpoint + * before {@link FilterMergeRule}. The simplify rule's bottom operand only matches a pure {@code + * IS NOT NULL} (or AND-of-{@code IS NOT NULL}) bucket-non-null filter; if {@code FilterMergeRule} + * runs first when a user {@code WHERE} sits below the synthetic {@code IS NOT NULL} filter that + * PPL emits as part of {@code dedup}, the two adjacent filters are merged into a single filter + * whose condition includes the user predicate, the simplify rule's predicate fails, no {@link + * org.opensearch.sql.calcite.plan.rel.LogicalDedup} is produced, and dedup pushdown to the + * OpenSearch storage engine is silently disabled. Using separate {@code addRuleInstance} calls + * (rather than {@code addRuleCollection}) enforces deterministic ordering: dedup simplification + * fires first against the original adjacent-filter shape, then any remaining adjacent filters are + * merged. + */ private static final HepProgram HEP_PROGRAM = - new HepProgramBuilder().addRuleCollection(hepRuleList).build(); + new HepProgramBuilder() + .addRuleInstance(PPLSimplifyDedupRule.DEDUP_SIMPLIFY_RULE) + .addRuleInstance(FilterMergeRule.Config.DEFAULT.toRule()) + .build(); public static RelNode optimize(RelNode plan, CalcitePlanContext context) { Util.discard(context); diff --git a/ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLAbstractTest.java b/ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLAbstractTest.java index ab07cd9b5c1..3d746975d73 100644 --- a/ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLAbstractTest.java +++ b/ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLAbstractTest.java @@ -45,6 +45,7 @@ import org.opensearch.sql.calcite.CalcitePlanContext; import org.opensearch.sql.calcite.CalciteRelNodeVisitor; import org.opensearch.sql.calcite.SysLimit; +import org.opensearch.sql.calcite.utils.CalciteToolsHelper; import org.opensearch.sql.common.setting.Settings; import org.opensearch.sql.datasource.DataSourceService; import org.opensearch.sql.exception.ExpressionEvaluationException; @@ -110,6 +111,20 @@ public RelNode getRelNode(String ppl) { return root; } + /** + * Get the root RelNode of the given PPL query after running the production HEP program from + * {@code CalciteToolsHelper}. Use this in regression tests that exercise rules registered in the + * production HEP program (e.g. {@code PPLSimplifyDedupRule}) — those rules need to see the raw + * planner output, not the post-FilterMerge form returned by {@link #getRelNode(String)}. + */ + public RelNode getRelNodeAfterCalciteHep(String ppl) { + CalcitePlanContext context = createBuilderContext(); + Query query = (Query) plan(pplParser, ppl); + planTransformer.analyze(query.getPlan(), context); + RelNode root = context.relBuilder.build(); + return CalciteToolsHelper.optimize(root, context); + } + private RelNode mergeAdjacentFilters(RelNode relNode) { HepProgram program = new HepProgramBuilder().addRuleInstance(FilterMergeRule.Config.DEFAULT.toRule()).build(); diff --git a/ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLDedupTest.java b/ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLDedupTest.java index ca1a789b0f4..ddbedfe6bd7 100644 --- a/ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLDedupTest.java +++ b/ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLDedupTest.java @@ -353,4 +353,85 @@ public void testSortFieldProjectedAwayBeforeDedup() { + " LogicalTableScan(table=[[scott, EMP]])\n"; verifyLogical(root, expectedLogical); } + + /** + * Regression test for issue #7: when a user {@code where} sits below {@code dedup}, the HEP + * program in {@code CalciteToolsHelper} must still produce a {@link + * org.opensearch.sql.calcite.plan.rel.LogicalDedup}. Before the fix, {@code FilterMergeRule} + * fired ahead of {@code PPLSimplifyDedupRule} and merged the user predicate into the + * bucket-non-null filter; the simplify rule's bottom operand then rejected the merged condition + * (it only accepts pure {@code IS NOT NULL}/AND-of-{@code IS NOT NULL}), no {@code LogicalDedup} + * was produced, and dedup pushdown to the OpenSearch storage engine was silently disabled. The + * fix is to enforce ordering: simplify-dedup runs to fixpoint first, then filter merging. + */ + /** + * Regression test for issue #7: when a user {@code where} sits below {@code dedup}, the HEP + * program in {@code CalciteToolsHelper} must still produce a {@link + * org.opensearch.sql.calcite.plan.rel.LogicalDedup}. Before the fix, both rules were registered + * via {@code addRuleCollection} so {@code FilterMergeRule} could fire ahead of {@code + * PPLSimplifyDedupRule}, merge the user predicate into the bucket-non-null filter, and silently + * disable dedup pushdown to the OpenSearch storage engine. The fix is to register the two rules + * with separate {@code addRuleInstance} calls in the order: simplify-dedup first, then + * filter-merge. + */ + @Test + public void testWhereThenDedupProducesLogicalDedup() { + // Use a where predicate on a DIFFERENT column from the dedup column. With the same column, + // Calcite's RexSimplify can fold AND(IS_NOT_NULL(x), >(x, c)) down to >(x, c), masking the + // bug. The issue's reproducer (where on @timestamp, dedup on namespace) hits this exact + // shape. + String ppl = "source=EMP | where SAL > 1000 | dedup 1 DEPTNO | fields DEPTNO"; + RelNode optimized = getRelNodeAfterCalciteHep(ppl); + String optimizedPlan = optimized.explain(); + org.junit.Assert.assertTrue( + "where + dedup must produce a LogicalDedup so OpenSearch DedupPushdownRule can match;" + + " actual plan was:\n" + + optimizedPlan, + optimizedPlan.contains("LogicalDedup")); + // The window-form leftover would indicate the simplify rule did not fire — assert it is gone. + org.junit.Assert.assertFalse( + "ROW_NUMBER window must be consumed by PPLSimplifyDedupRule when where + dedup are" + + " combined; actual plan was:\n" + + optimizedPlan, + optimizedPlan.contains("ROW_NUMBER")); + } + + /** + * Adversarial regression test: simulates the pathological order described in issue #7 by forcing + * FilterMergeRule to run to fixpoint BEFORE PPLSimplifyDedupRule. This documents the failure mode + * the fix in {@code CalciteToolsHelper} prevents — once the bucket-non-null filter has been + * merged with the user {@code WHERE}, {@code mayBeFilterFromBucketNonNull} can never accept the + * combined condition, so {@code PPLSimplifyDedupRule} is permanently unable to produce a {@code + * LogicalDedup}. The production fix enforces order at the program level (sequential {@code + * addRuleInstance} calls), making this hazard unreachable. + */ + @Test + public void testFilterMergeBeforeSimplifyDedupBreaksPattern() { + String ppl = "source=EMP | where SAL > 1000 | dedup 1 DEPTNO | fields DEPTNO"; + // getRelNode already runs FilterMergeRule on the raw plan, simulating the pathological + // schedule where FilterMergeRule fires before PPLSimplifyDedupRule. + RelNode mergedFirst = getRelNode(ppl); + org.apache.calcite.plan.hep.HepProgram simplifyOnly = + new org.apache.calcite.plan.hep.HepProgramBuilder() + .addRuleInstance( + org.opensearch.sql.calcite.plan.rule.PPLSimplifyDedupRule.DEDUP_SIMPLIFY_RULE) + .build(); + org.apache.calcite.plan.hep.HepPlanner planner = + new org.apache.calcite.plan.hep.HepPlanner(simplifyOnly); + planner.setRoot(mergedFirst); + RelNode result = planner.findBestExp(); + String plan = result.explain(); + org.junit.Assert.assertFalse( + "If FilterMergeRule runs before PPLSimplifyDedupRule, the simplify rule must NOT recover" + + " — the merged AND(IS_NOT_NULL, user_predicate) filter fails the bucket-non-null" + + " predicate. This documents why the production HEP program enforces ordering via" + + " separate addRuleInstance calls (PPLSimplifyDedupRule first, then FilterMergeRule)." + + " Actual plan was:\n" + + plan, + plan.contains("LogicalDedup")); + org.junit.Assert.assertTrue( + "Plan should still contain ROW_NUMBER window form when simplify fails. Actual plan was:\n" + + plan, + plan.contains("ROW_NUMBER")); + } } From bd848efb190aaf92667a85ba75bd118a900ccb71 Mon Sep 17 00:00:00 2001 From: ryan-gh-bot Date: Fri, 29 May 2026 01:13:23 +0000 Subject: [PATCH 2/3] [BugFix] Consolidate duplicated javadoc on dedup regression test (#8) Address review feedback on PR #8: testWhereThenDedupProducesLogicalDedup had two adjacent javadoc blocks above the @Test method. Merge them into a single javadoc that retains the failure-mechanism explanation (mayBeFilterFromBucketNonNull rejecting the merged condition) along with the addRuleCollection -> addRuleInstance fix detail. Signed-off-by: ryan-gh-bot --- .../sql/ppl/calcite/CalcitePPLDedupTest.java | 22 ++++++------------- 1 file changed, 7 insertions(+), 15 deletions(-) diff --git a/ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLDedupTest.java b/ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLDedupTest.java index ddbedfe6bd7..d778269443c 100644 --- a/ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLDedupTest.java +++ b/ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLDedupTest.java @@ -354,25 +354,17 @@ public void testSortFieldProjectedAwayBeforeDedup() { verifyLogical(root, expectedLogical); } - /** - * Regression test for issue #7: when a user {@code where} sits below {@code dedup}, the HEP - * program in {@code CalciteToolsHelper} must still produce a {@link - * org.opensearch.sql.calcite.plan.rel.LogicalDedup}. Before the fix, {@code FilterMergeRule} - * fired ahead of {@code PPLSimplifyDedupRule} and merged the user predicate into the - * bucket-non-null filter; the simplify rule's bottom operand then rejected the merged condition - * (it only accepts pure {@code IS NOT NULL}/AND-of-{@code IS NOT NULL}), no {@code LogicalDedup} - * was produced, and dedup pushdown to the OpenSearch storage engine was silently disabled. The - * fix is to enforce ordering: simplify-dedup runs to fixpoint first, then filter merging. - */ /** * Regression test for issue #7: when a user {@code where} sits below {@code dedup}, the HEP * program in {@code CalciteToolsHelper} must still produce a {@link * org.opensearch.sql.calcite.plan.rel.LogicalDedup}. Before the fix, both rules were registered - * via {@code addRuleCollection} so {@code FilterMergeRule} could fire ahead of {@code - * PPLSimplifyDedupRule}, merge the user predicate into the bucket-non-null filter, and silently - * disable dedup pushdown to the OpenSearch storage engine. The fix is to register the two rules - * with separate {@code addRuleInstance} calls in the order: simplify-dedup first, then - * filter-merge. + * via {@code addRuleCollection}, so {@code FilterMergeRule} could fire ahead of {@code + * PPLSimplifyDedupRule} and merge the user predicate into the bucket-non-null filter; the + * simplify rule's bottom operand then rejected the merged condition (it only accepts pure {@code + * IS NOT NULL}/AND-of-{@code IS NOT NULL}), no {@code LogicalDedup} was produced, and dedup + * pushdown to the OpenSearch storage engine was silently disabled. The fix is to register the two + * rules with separate {@code addRuleInstance} calls in the order simplify-dedup first (to + * fixpoint), then filter-merge. */ @Test public void testWhereThenDedupProducesLogicalDedup() { From e9bcf1b49e72b40a6d0624e2b68ca7a556695ae7 Mon Sep 17 00:00:00 2001 From: ryan-gh-bot Date: Fri, 29 May 2026 01:18:24 +0000 Subject: [PATCH 3/3] [BugFix] Replace inline FQNs with proper imports in dedup regression test (#8) Add static imports for assertTrue/assertFalse and proper imports for HepPlanner, HepProgram, HepProgramBuilder, PPLSimplifyDedupRule, and LogicalDedup, replacing all inline fully-qualified names in the regression tests added for #7. Pure refactor; no behavior change. Signed-off-by: ryan-gh-bot --- .../sql/ppl/calcite/CalcitePPLDedupTest.java | 43 ++++++++++--------- 1 file changed, 23 insertions(+), 20 deletions(-) diff --git a/ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLDedupTest.java b/ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLDedupTest.java index d778269443c..503e5080365 100644 --- a/ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLDedupTest.java +++ b/ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLDedupTest.java @@ -5,9 +5,17 @@ package org.opensearch.sql.ppl.calcite; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; + +import org.apache.calcite.plan.hep.HepPlanner; +import org.apache.calcite.plan.hep.HepProgram; +import org.apache.calcite.plan.hep.HepProgramBuilder; import org.apache.calcite.rel.RelNode; import org.apache.calcite.test.CalciteAssert; import org.junit.Test; +import org.opensearch.sql.calcite.plan.rel.LogicalDedup; +import org.opensearch.sql.calcite.plan.rule.PPLSimplifyDedupRule; public class CalcitePPLDedupTest extends CalcitePPLAbstractTest { @@ -356,15 +364,14 @@ public void testSortFieldProjectedAwayBeforeDedup() { /** * Regression test for issue #7: when a user {@code where} sits below {@code dedup}, the HEP - * program in {@code CalciteToolsHelper} must still produce a {@link - * org.opensearch.sql.calcite.plan.rel.LogicalDedup}. Before the fix, both rules were registered - * via {@code addRuleCollection}, so {@code FilterMergeRule} could fire ahead of {@code - * PPLSimplifyDedupRule} and merge the user predicate into the bucket-non-null filter; the - * simplify rule's bottom operand then rejected the merged condition (it only accepts pure {@code - * IS NOT NULL}/AND-of-{@code IS NOT NULL}), no {@code LogicalDedup} was produced, and dedup - * pushdown to the OpenSearch storage engine was silently disabled. The fix is to register the two - * rules with separate {@code addRuleInstance} calls in the order simplify-dedup first (to - * fixpoint), then filter-merge. + * program in {@code CalciteToolsHelper} must still produce a {@link LogicalDedup}. Before the + * fix, both rules were registered via {@code addRuleCollection}, so {@code FilterMergeRule} could + * fire ahead of {@code PPLSimplifyDedupRule} and merge the user predicate into the + * bucket-non-null filter; the simplify rule's bottom operand then rejected the merged condition + * (it only accepts pure {@code IS NOT NULL}/AND-of-{@code IS NOT NULL}), no {@code LogicalDedup} + * was produced, and dedup pushdown to the OpenSearch storage engine was silently disabled. The + * fix is to register the two rules with separate {@code addRuleInstance} calls in the order + * simplify-dedup first (to fixpoint), then filter-merge. */ @Test public void testWhereThenDedupProducesLogicalDedup() { @@ -375,13 +382,13 @@ public void testWhereThenDedupProducesLogicalDedup() { String ppl = "source=EMP | where SAL > 1000 | dedup 1 DEPTNO | fields DEPTNO"; RelNode optimized = getRelNodeAfterCalciteHep(ppl); String optimizedPlan = optimized.explain(); - org.junit.Assert.assertTrue( + assertTrue( "where + dedup must produce a LogicalDedup so OpenSearch DedupPushdownRule can match;" + " actual plan was:\n" + optimizedPlan, optimizedPlan.contains("LogicalDedup")); // The window-form leftover would indicate the simplify rule did not fire — assert it is gone. - org.junit.Assert.assertFalse( + assertFalse( "ROW_NUMBER window must be consumed by PPLSimplifyDedupRule when where + dedup are" + " combined; actual plan was:\n" + optimizedPlan, @@ -403,17 +410,13 @@ public void testFilterMergeBeforeSimplifyDedupBreaksPattern() { // getRelNode already runs FilterMergeRule on the raw plan, simulating the pathological // schedule where FilterMergeRule fires before PPLSimplifyDedupRule. RelNode mergedFirst = getRelNode(ppl); - org.apache.calcite.plan.hep.HepProgram simplifyOnly = - new org.apache.calcite.plan.hep.HepProgramBuilder() - .addRuleInstance( - org.opensearch.sql.calcite.plan.rule.PPLSimplifyDedupRule.DEDUP_SIMPLIFY_RULE) - .build(); - org.apache.calcite.plan.hep.HepPlanner planner = - new org.apache.calcite.plan.hep.HepPlanner(simplifyOnly); + HepProgram simplifyOnly = + new HepProgramBuilder().addRuleInstance(PPLSimplifyDedupRule.DEDUP_SIMPLIFY_RULE).build(); + HepPlanner planner = new HepPlanner(simplifyOnly); planner.setRoot(mergedFirst); RelNode result = planner.findBestExp(); String plan = result.explain(); - org.junit.Assert.assertFalse( + assertFalse( "If FilterMergeRule runs before PPLSimplifyDedupRule, the simplify rule must NOT recover" + " — the merged AND(IS_NOT_NULL, user_predicate) filter fails the bucket-non-null" + " predicate. This documents why the production HEP program enforces ordering via" @@ -421,7 +424,7 @@ public void testFilterMergeBeforeSimplifyDedupBreaksPattern() { + " Actual plan was:\n" + plan, plan.contains("LogicalDedup")); - org.junit.Assert.assertTrue( + assertTrue( "Plan should still contain ROW_NUMBER window form when simplify fails. Actual plan was:\n" + plan, plan.contains("ROW_NUMBER"));