[BugFix] Restore dedup pushdown when combined with WHERE clause (#5482)

[ryan-gh-bot]

Description

PPL dedup is initially compiled into a ROW_NUMBER() OVER (PARTITION BY field) window pattern with an IS NOT NULL(field) bucket-non-null filter directly above the scan. PPLSimplifyDedupRule folds that pattern into a LogicalDedup which DedupPushdownRule then rewrites into a composite + top_hits aggregation against the shard.

The HEP program registers FilterMergeRule.DEFAULT and PPLSimplifyDedupRule.DEDUP_SIMPLIFY_RULE via addRuleCollection. When a user where precedes dedup, the user's filter sits adjacent to the bucket-non-null filter; FilterMergeRule can merge them into AND(IS NOT NULL(field), <user predicate>). The previous mayBeFilterFromBucketNonNull operand predicate required every conjunct of the AND to be IS NOT NULL($ref) (allMatch), so the merged shape failed the match. PPLSimplifyDedupRule never fired, no LogicalDedup was produced, and DedupPushdownRule had nothing to match — dedup fell through to the in-memory ROW_NUMBER window form on the coordinator, which exceeds timeouts on large indices.

Fix

Loosen the operand predicate at the source rather than reordering HEP rules.

• PlanUtils.mayBeFilterFromBucketNonNull now accepts the un-merged IS NOT NULL($ref) shape or any AND whose conjuncts contain at least oneIS NOT NULL($ref) (anyMatch instead of allMatch). The predicate is order-independent — the simplify rule fires whether or not FilterMergeRule ran first.
• PPLSimplifyDedupRule.apply splits the matched filter's conjuncts into (a) the IS NOT NULL on a partition key (absorbed by LogicalDedup) and (b) everything else (emitted as a separate filter below LogicalDedup), so any user predicate folded into the AND is preserved verbatim. A defensive bail-out: if no IS NOT NULL on a partition key is present in the matched filter, the rule returns without transforming.
• PlanUtils.isNotNullOnRef is promoted to public so PPLSimplifyDedupRule.isNotNullOnPartitionKey can reuse it.

Verification

• CalcitePPLDedupTest.testDedupAfterWhereProducesLogicalDedup runs the rule sequence (PPLSimplifyDedupRule, FilterMergeRule) on a where + dedup plan and asserts that LogicalDedup is produced regardless of rule order. The user predicate appears as a separate filter below LogicalDedup, ready for downstream pushdown:

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

• CalcitePPLDedupTest.testDedupAfterWhereProducesLogicalDedupRegardlessOfRuleOrder exercises the swapped order (FilterMergeRule first) and asserts the same result, pinning the user-visible contract rather than any rule-ordering invariant.

• CalciteExplainIT.testDedupAfterWherePushDown (new) — end-to-end against a live cluster: runs source=opensearch-sql_test_index_account | where age > 25 | dedup gender and asserts LogicalDedup is present in the explain output and EnumerableWindow is absent (i.e., dedup is actually pushed down and the in-memory window fallback the bug caused is gone).

• All existing CalcitePPLDedupTest cases continue to pass.

Related Issues

Resolves#5482

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.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 38fc6c3)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review Possible Issue When the filter is only partially pushable, the code bails out without transforming. However, the original dedup pattern (with the filter still present) remains in the plan. If the residual filter is later removed or merged by another rule pass, a subsequent optimizer iteration might re-match this same dedup pattern, potentially causing an infinite loop or redundant rule applications. The code should either mark the node as transformed or ensure the pattern cannot re-match. if (call.rels[1] instanceofLogicalFilterfilter) { // Push the filter into the scan, then synthesize an identity project so the standard// apply() can run on the resulting Dedup → Project → Scan shape. If the filter is only// partially pushable, pushDownFilter returns a Filter wrapping the residual condition over// the new scan; we can't strip a residual filter without breaking semantics, so bail.finalCalciteLogicalIndexScanscan = call.rel(2); if (!(scan.pushDownFilter(filter) instanceofCalciteLogicalIndexScannewScan)) { return; } RelBuilderrelBuilder = call.builder(); Possible Issue If dedupColumns contains a non-RexInputRef element (e.g., a literal or expression), partitionKeyIndices will not include it, but the dedup pattern may still be valid. The defensive check at line 148 will bail out even though a valid IS_NOT_NULL conjunct on an actual partition key might exist. This could silently skip legitimate dedup simplifications when partition keys include expressions. Set<Integer> partitionKeyIndices = newHashSet<>(); for (RexNodekey : dedupColumns) { if (keyinstanceofRexInputRefref) { partitionKeyIndices.add(ref.getIndex()); } } List<RexNode> bucketNonNullConjuncts = newArrayList<>(); List<RexNode> remainingConjuncts = newArrayList<>(); for (RexNodeconjunct : RelOptUtil.conjunctions(bucketNonNullFilter.getCondition())) { if (isNotNullOnPartitionKey(conjunct, partitionKeyIndices)) { bucketNonNullConjuncts.add(conjunct); } else { remainingConjuncts.add(conjunct); } } // Defensive: if no IS_NOT_NULL conjunct on a partition key is present, this filter is not// actually a bucket-non-null filter — bail out without transforming. The loose operand// predicate may have matched on an unrelated AND that happens to contain an IS_NOT_NULL on// some other ref.if (bucketNonNullConjuncts.isEmpty()) { return; }

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 38fc6c3

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Add type check before casting The code assumes call.rel(2) is always a CalciteLogicalIndexScan when call.rels[1] is a LogicalFilter, but this assumption isn't validated. If the operand structure changes or the rule matches unexpectedly, a ClassCastException could occur. Add a type check before casting. opensearch/src/main/java/org/opensearch/sql/opensearch/planner/rules/DedupPushdownRule.java [51-59] if (call.rels[1] instanceof LogicalFilter filter) { // Push the filter into the scan, then synthesize an identity project so the standard // apply() can run on the resulting Dedup → Project → Scan shape. If the filter is only // partially pushable, pushDownFilter returns a Filter wrapping the residual condition over // the new scan; we can't strip a residual filter without breaking semantics, so bail. - final CalciteLogicalIndexScan scan = call.rel(2);+ if (!(call.rel(2) instanceof CalciteLogicalIndexScan scan)) {+ return;+ } if (!(scan.pushDownFilter(filter) instanceof CalciteLogicalIndexScan newScan)) { return; } Suggestion importance[1-10]: 7 __ Why: Valid safety improvement. The operand structure is defined by the rule configuration, but adding an explicit type check before casting call.rel(2) to CalciteLogicalIndexScan prevents potential ClassCastException if the rule configuration changes or matches unexpectedly, improving robustness.	Medium
Possible issue	Validate no predicates are lost When no bucket-non-null conjuncts are found, the method returns early without transforming. However, the extracted remainingConjuncts are discarded, potentially losing user predicates. Consider logging a warning or validating that remainingConjuncts is also empty to ensure no predicates are silently dropped. core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.java [148-150] if (bucketNonNullConjuncts.isEmpty()) { + // Defensive: if no IS_NOT_NULL conjunct on a partition key is present, this filter is not+ // actually a bucket-non-null filter — bail out without transforming.+ if (!remainingConjuncts.isEmpty()) {+ // Log or handle unexpected case where user predicates exist but no bucket guards found+ throw new IllegalStateException("Expected bucket-non-null filter but found only user predicates");+ } return; } Suggestion importance[1-10]: 3 __ Why: The suggestion raises a valid concern about potentially discarding remainingConjuncts, but the current behavior is intentional: if no bucket-non-null conjuncts exist, the filter isn't a dedup pattern and the rule shouldn't transform. The remainingConjuncts remain in the original filter. Throwing an exception would be too aggressive for a defensive check.	Low

---

Previous suggestions

Suggestions up to commit 95e2aeb

Category	Suggestion	Impact
General	Handle partial filter pushdown explicitly The pattern match checks if pushDownFilter returns a CalciteLogicalIndexScan, but doesn't verify whether the filter was fully pushed down or if a residual filter remains. If pushDownFilter returns a LogicalFilter wrapping a scan (indicating partial pushdown), the cast fails silently and the method returns. Consider explicitly checking the return type and handling partial pushdown scenarios. opensearch/src/main/java/org/opensearch/sql/opensearch/planner/rules/DedupPushdownRule.java [57-59] -if (!(scan.pushDownFilter(filter) instanceof CalciteLogicalIndexScan newScan)) {+var pushResult = scan.pushDownFilter(filter);+if (!(pushResult instanceof CalciteLogicalIndexScan newScan)) {+ // Partial pushdown or unsupported filter - bail out return; } Suggestion importance[1-10]: 4 __ Why: The suggestion correctly identifies that the code checks for partial pushdown via the instanceof pattern match and bails out if a residual filter remains (as stated in the comment on lines 53-55). Extracting to a variable improves readability slightly, but the logic is already correct and the comment explains the behavior. The improvement is marginal.	Low
	Validate all conjuncts are classified The loop iterates over all conjunctions and classifies them into two lists. If the filter condition is complex or deeply nested, RelOptUtil.conjunctions may not flatten all levels of nested ANDs. Consider adding validation to ensure all conjuncts are properly classified, or document the assumption that only top-level ANDs are expected. core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.java [137-143] -for (RexNode conjunct : RelOptUtil.conjunctions(bucketNonNullFilter.getCondition())) {+List<RexNode> allConjuncts = RelOptUtil.conjunctions(bucketNonNullFilter.getCondition());+for (RexNode conjunct : allConjuncts) { if (isNotNullOnPartitionKey(conjunct, partitionKeyIndices)) { bucketNonNullConjuncts.add(conjunct); } else { remainingConjuncts.add(conjunct); } } +// Validate that all conjuncts were classified+assert bucketNonNullConjuncts.size() + remainingConjuncts.size() == allConjuncts.size(); Suggestion importance[1-10]: 2 __ Why: The suggestion adds an assertion to validate that all conjuncts are classified, but RelOptUtil.conjunctions is a standard Calcite utility that flattens top-level ANDs reliably. The assertion would always pass in normal operation and only catch programming errors. The concern about deeply nested ANDs is unfounded since conjunctions handles this. The added validation has minimal practical value.	Low
Possible issue	Validate remaining conjuncts before early return When no bucket-non-null conjuncts are found, the method returns early without transforming. However, the extracted remainingConjuncts list is not checked for emptiness. If remainingConjuncts is non-empty but bucketNonNullConjuncts is empty, user predicates could be silently dropped. Consider logging a warning or validating that remainingConjuncts is also empty before returning. core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.java [148-150] if (bucketNonNullConjuncts.isEmpty()) { + if (!remainingConjuncts.isEmpty()) {+ // Log or handle unexpected case: filter has user predicates but no bucket-non-null guards+ } return; } Suggestion importance[1-10]: 3 __ Why: The suggestion raises a valid edge case where remainingConjuncts could be non-empty while bucketNonNullConjuncts is empty. However, the early return is intentional per the defensive comment (lines 144-147), and silently dropping user predicates is unlikely since the operand predicate should prevent matching unrelated filters. The suggestion to log or validate is reasonable but has low impact.	Low

Suggestions up to commit a66ad5b

Category	Suggestion	Impact
General	Handle non-RexInputRef partition keys The code only handles RexInputRef instances when building partitionKeyIndices, silently ignoring other node types. If dedupColumns contains non-RexInputRef nodes (e.g., expressions), those partition keys won't be tracked, potentially causing incorrect classification of conjuncts. Consider handling or explicitly documenting this limitation. core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.java [130-134] for (RexNode key : dedupColumns) { if (key instanceof RexInputRef ref) { partitionKeyIndices.add(ref.getIndex()); + } else {+ // Log or handle non-RexInputRef partition keys if they're expected+ call.getPlanner().getContext().unwrap(org.slf4j.Logger.class)+ .warn("Unexpected partition key type: {}", key.getClass().getSimpleName()); } } Suggestion importance[1-10]: 4 __ Why: The suggestion correctly identifies that non-RexInputRef nodes in dedupColumns are silently ignored. However, based on the PR context and the dedup pattern, partition keys are expected to be simple column references (RexInputRef). Adding logging for unexpected types could help catch edge cases, but the current behavior appears intentional for the dedup use case. The suggestion provides moderate value for defensive programming.	Low
	Add logging for defensive check When no bucket-non-null conjuncts are found, the method returns early without transforming the plan. However, this leaves the original plan unchanged, which may cause downstream rules to fail or produce incorrect results. Consider logging a warning or throwing an exception to make this defensive check more explicit and aid debugging. core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.java [148-150] if (bucketNonNullConjuncts.isEmpty()) { + // Log or handle the case where the filter doesn't match the expected pattern+ call.getPlanner().getContext().unwrap(org.slf4j.Logger.class)+ .warn("Expected bucket-non-null filter pattern not found; skipping transformation"); return; } Suggestion importance[1-10]: 3 __ Why: While adding logging for the defensive check could aid debugging, the early return is already well-documented with a comment explaining why the transformation is skipped. The suggestion to add logging is a minor improvement that would help debugging but doesn't address a critical issue. The improved code also assumes a specific logging framework is available in the context, which may not be the case.	Low
	Log filter pushdown failures The pushDownFilter method may return null if the filter cannot be pushed down, but the code silently returns without any indication of why the transformation failed. This makes debugging difficult when pushdown unexpectedly fails. Consider logging the failure reason or the filter condition that couldn't be pushed. opensearch/src/main/java/org/opensearch/sql/opensearch/planner/rules/DedupPushdownRule.java [56-59] AbstractRelNode scanWithFilter = scan.pushDownFilter(filter); if (scanWithFilter == null) { + // Log when filter pushdown fails to aid debugging+ call.getPlanner().getContext().unwrap(org.slf4j.Logger.class)+ .debug("Filter pushdown failed for condition: {}", filter.getCondition()); return; } Suggestion importance[1-10]: 3 __ Why: Adding debug logging when pushDownFilter returns null could help with debugging, but this is a normal control flow path where the rule simply doesn't match. The null return indicates the filter cannot be pushed down, which is expected behavior in some cases. The suggestion provides marginal debugging value but doesn't fix any bug or improve correctness.	Low

Suggestions up to commit f67458f

Category	Suggestion	Impact
General	Add fallback for failed filter pushdown When pushDownFilter returns null, the method returns early without any transformation. This silently fails the optimization. Consider logging this case or falling back to the standard path to ensure the query still executes correctly, even if the filter cannot be pushed down. opensearch/src/main/java/org/opensearch/sql/opensearch/planner/rules/DedupPushdownRule.java [56-59] AbstractRelNode scanWithFilter = scan.pushDownFilter(filter); if (scanWithFilter == null) { + // Filter cannot be pushed down; fall back to standard dedup path+ // or log the failure for debugging return; } Suggestion importance[1-10]: 4 __ Why: The suggestion identifies a valid concern about the early return when pushDownFilter returns null. However, the current behavior is intentional - when filter pushdown fails, the rule simply doesn't match and other rules can handle the query. Adding a log for debugging could be helpful, but the suggestion to "fall back to standard dedup path" is vague since the early return already allows the standard path (without the filter optimization) to proceed via the else branch at line 70. The comment addition would provide minimal value.	Low
	Handle edge case for missing bucket guards When bucketNonNullConjuncts is empty, the method returns early without transforming. However, remainingConjuncts may contain valid user predicates that should still be processed. Consider logging a warning or handling the case where only non-partition-key predicates exist to avoid silently dropping valid filters. core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.java [148-150] if (bucketNonNullConjuncts.isEmpty()) { + // Log or handle case where filter doesn't match expected dedup pattern+ // but may contain valid user predicates return; } Suggestion importance[1-10]: 3 __ Why: The suggestion correctly identifies the early return when bucketNonNullConjuncts is empty, but the concern about "silently dropping valid filters" is misplaced. The code comment at lines 144-147 explicitly states this is a defensive check to bail out when the filter doesn't match the expected bucket-non-null pattern. The remainingConjuncts are only relevant when there are actual bucket guards present. Adding a log might be marginally useful for debugging, but doesn't address a real correctness issue.	Low

Suggestions up to commit 0bf8f7c

Category	Suggestion	Impact
General	Remove redundant operand check The !rexCall.getOperands().isEmpty() check is redundant because IS_NOT_NULL always has exactly one operand by definition. Accessing getOperands().get(0) without this check is safe and simplifies the logic. core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.java [174-180] private static boolean isNotNullOnPartitionKey(RexNode rex, Set<Integer> partitionKeyIndices) { return rex instanceof RexCall rexCall && rexCall.isA(SqlKind.IS_NOT_NULL) - && !rexCall.getOperands().isEmpty() && rexCall.getOperands().get(0) instanceof RexInputRef ref && partitionKeyIndices.contains(ref.getIndex()); } Suggestion importance[1-10]: 4 __ Why: While IS_NOT_NULL typically has exactly one operand, the check !rexCall.getOperands().isEmpty() provides defensive programming against malformed expressions. Removing it offers minimal benefit and slightly reduces code robustness. The same pattern is used consistently in PlanUtils.isNotNullOnRef at line 826, suggesting it's an intentional defensive practice.	Low
	Handle non-reference partition keys Handle non-RexInputRef partition keys gracefully. If dedupColumns contains complex expressions that aren't simple column references, they won't be added to partitionKeyIndices, potentially causing valid bucket-non-null filters to be incorrectly rejected. Consider logging a warning or adding a comment explaining this limitation. core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.java [130-134] for (RexNode key : dedupColumns) { if (key instanceof RexInputRef ref) { partitionKeyIndices.add(ref.getIndex()); } + // Note: Complex expressions in dedupColumns are not tracked as partition keys.+ // This may cause the rule to bail out if bucket-non-null filters reference them. } Suggestion importance[1-10]: 3 __ Why: The suggestion correctly identifies that non-RexInputRef partition keys won't be tracked, but this appears to be intentional design rather than a bug. Adding a comment could improve code clarity, but the impact is minimal since the defensive check at line 148 already handles this case by bailing out when no matching filters are found.	Low

Suggestions up to commit 19f506d

Category	Suggestion	Impact
General	Make filter assertion more specific The assertion checking for "1000" is too generic and may match unintended occurrences in the plan. Consider checking for a more specific pattern like ">($7, 1000)" or "SAL > 1000" to ensure the user's WHERE clause filter is actually present in the expected location. ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLDedupTest.java [380-386] String rawExplain = raw.explain(); Assert.assertTrue( "Raw plan should contain the bucket-non-null filter:\n" + rawExplain, rawExplain.contains("IS NOT NULL")); Assert.assertTrue( "Raw plan should contain the user where filter:\n" + rawExplain, - rawExplain.contains("1000"));+ rawExplain.contains(">") && rawExplain.contains("1000")); Suggestion importance[1-10]: 4 __ Why: The suggestion correctly identifies that checking for "1000" alone could match unintended occurrences. However, the improved code rawExplain.contains(">") && rawExplain.contains("1000") is still too generic and doesn't guarantee the filter is in the expected location. A more specific pattern would be better, but the suggestion does improve robustness slightly.	Low

[RyanL1997]

Hi @ryan-gh-bot , thanks for the change, left some reviews.

[github-actions]

Persistent review updated to latest commit 19f506d

[RyanL1997]

@ryan-gh-bot the rule-reordering fix works but treats a symptom rather than the root cause. The actual brittleness is in PlanUtils.mayBeFilterFromBucketNonNull — its allMatch(isNotNullOnRef) requirement means any future code path that produces a merged conjunction (a new rule, a different optimization order, a SQL-side caller) will silently re-disable dedup pushdown the same way. The HEP ordering is now a load-bearing invariant with nothing enforcing it; if anyone later adds a third rule to HEP_PROGRAM or refactors it, the bug comes back silently. The new testDedupAfterWhereWithFilterMergeFirstFailsToSimplify test pins the implementation strategy rather than the user-visible behavior.

Please revise to take option 1 from the issue instead:

1. Extend mayBeFilterFromBucketNonNull (core/.../utils/PlanUtils.java) to accept AND(IS_NOT_NULL(partition_col), <rest>) — i.e., recognize an AND whose operands contain an IS_NOT_NULL on the partition column rather than requiring every conjunct to be one.
2. In PPLSimplifyDedupRule.apply, split the non-IS_NOT_NULL conjuncts off and emit them as a separate filter below the new LogicalDedup, so the user predicate is preserved and pushdown still works.
3. Revert the addRuleCollection → addRuleInstance change in CalciteToolsHelper.HEP_PROGRAM — once the predicate is order-independent, rule order no longer matters.
4. Replace testDedupAfterWhereWithFilterMergeFirstFailsToSimplify (which pins buggy behavior) with a test that asserts the user-visible contract: with where upstream, dedup still produces a LogicalDedup regardless of rule order. Keep testDedupAfterWhereProducesLogicalDedup.

Please sanity-check that the loosened operand can't accidentally match user-written IS_NOT_NULL(x) AND ... filters outside the dedup pattern — the four-level operand chain (project → filter with row_number <= N → project containing ROW_NUMBER-dedup → filter) should be specific enough, but worth confirming.

[ryan-gh-bot]

Working on /@triage (invoked by @RyanL1997) — posting result when done.

[ryan-gh-bot]

Done — pushed 0bf8f7c73.

[github-actions]

Persistent review updated to latest commit 0bf8f7c

[dai-chen]

@ryan-gh-bot the rule-reordering fix works but treats a symptom rather than the root cause. The actual brittleness is in PlanUtils.mayBeFilterFromBucketNonNull — its allMatch(isNotNullOnRef) requirement means any future code path that produces a merged conjunction (a new rule, a different optimization order, a SQL-side caller) will silently re-disable dedup pushdown the same way. The HEP ordering is now a load-bearing invariant with nothing enforcing it; if anyone later adds a third rule to HEP_PROGRAM or refactors it, the bug comes back silently. The new testDedupAfterWhereWithFilterMergeFirstFailsToSimplify test pins the implementation strategy rather than the user-visible behavior.

Please revise to take option 1 from the issue instead:

1. Extend mayBeFilterFromBucketNonNull (core/.../utils/PlanUtils.java) to accept AND(IS_NOT_NULL(partition_col), <rest>) — i.e., recognize an AND whose operands contain an IS_NOT_NULL on the partition column rather than requiring every conjunct to be one.
2. In PPLSimplifyDedupRule.apply, split the non-IS_NOT_NULL conjuncts off and emit them as a separate filter below the new LogicalDedup, so the user predicate is preserved and pushdown still works.
3. Revert the addRuleCollection → addRuleInstance change in CalciteToolsHelper.HEP_PROGRAM — once the predicate is order-independent, rule order no longer matters.
4. Replace testDedupAfterWhereWithFilterMergeFirstFailsToSimplify (which pins buggy behavior) with a test that asserts the user-visible contract: with where upstream, dedup still produces a LogicalDedup regardless of rule order. Keep testDedupAfterWhereProducesLogicalDedup.

Please sanity-check that the loosened operand can't accidentally match user-written IS_NOT_NULL(x) AND ... filters outside the dedup pattern — the four-level operand chain (project → filter with row_number <= N → project containing ROW_NUMBER-dedup → filter) should be specific enough, but worth confirming.

I'm weighing the pros and cons of these two options:

• Option 1 — make PPLSimplifyDedupRule run first: If this rule runs before FilterMergeRule, it matches the original unmerged pattern and fixes the problem directly. The concern is that some other rule could run ahead of it and reintroduce the mismatch.
• Option 2 - current PR approach: Does this cover all the plan shapes visitDedupe can produce, or does it only fix the plain where + dedup case? e.g. dedup N, multiple dedup fields, and the sort/collation cases. Or these cases are not supported by DSL query anyway?

[penghuo]

1. Add end-to-end test asserts dedup is actually pushed down when where precedes dedup.
2. Refactor PR description to match the implementation.

[github-actions]

Persistent review updated to latest commit ae86ece

[RyanL1997]

Hi @penghuo, thanks for the feedback:

Add end-to-end test asserts dedup is actually pushed down when where precedes dedup.

added CalciteExplainIT.testDedupAfterWherePushDown

Refactor PR description to match the implementation.

PR description refactored — now describes the actual implementation (loosen predicate at source, not reorder HEP rules) instead of the bot's earlier reorder approach.

[RyanL1997]

Walked the axes against buildDedupNotNull / buildDedupOrNull and reasoned about each. The fix lives in two places — the mayBeFilterFromBucketNonNull operand predicate, and the conjunct split inside PPLSimplifyDedupRule#apply. As long as the bottom filter of the rule's 4-level operand chain still includes an IS NOT NULL($pk) somewhere (alone or in an AND), the rule fires and apply cleanly separates the partition-key guards from the user predicate.

Axis	Effect on bucket-non-null filter shape	Covered?	Why
dedup N (N ≥ 2)	N lives in the upper _row_number_ <= N filter, not the bucket-non-null filter. Bottom-filter shape is identical regardless of N.	Yes	Predicate match path is unchanged; apply reads N from the upper filter as before.
Multiple dedup fields (dedup a, b)	Bucket-non-null becomes AND(IS NOT NULL(a), IS NOT NULL(b)). With a user where, FilterMergeRule produces AND(IS NOT NULL(a), IS NOT NULL(b), <user predicate>).	Yes	The anyMatch predicate matches (multiple IS NOT NULL conjuncts present). apply absorbs all IS NOT NULL-on-partition-key conjuncts into LogicalDedup and emits the user predicate as a separate filter below it.
Sort/collation (| sort x | dedup a / | where p | sort x | dedup a)	stripInputSort pulls the prior sort out and threads it into ROW_NUMBER's ORDER BY. Bucket-non-null filter shape is unchanged.	Yes	apply extracts collation from the window. The user where can sit below or above the sort; either way the bottom filter is the same bucket-non-null shape (possibly merged with the user predicate).
keepempty=true (with or without where)	buildDedupOrNull does not emit a bucket-non-null filter at all. The 4-level operand chain has no matching bottom filter.	No, by design	The rule's operand chain doesn't match, so the rule never fires. Documented in code: PPLSimplifyDedupRule.java:101 (Since we cannot push down dedup with keepEmpty=true, we don't simplify that pattern) and DedupPushdownRule.java:218 (Cannot push dedup operator if keepEmpty=true). Unchanged by this PR.

Net: this PR covers the keepempty=false cases (the default and only pushable shape) across all four where + dedup <variant> combinations — simple, dedup N, multi-field, and sort/collation. The keepempty=true case is an existing, intentional non-pushable shape, unchanged here.

[RyanL1997]

Checking on the CI failures

[github-actions]

Persistent review updated to latest commit 7725e0b

[github-actions]

Persistent review updated to latest commit f67458f

[github-actions]

Persistent review updated to latest commit a66ad5b

[github-actions]

Persistent review updated to latest commit 95e2aeb

[RyanL1997]

The current CI failure is unrelated.

The failure is:

 * What went wrong:
Execution failed for task ':doctest:startPrometheus'.
> java.net.SocketTimeoutException: Read timed out

[github-actions]

Persistent review updated to latest commit 38fc6c3

[ryan-gh-bot]

Description

PPL dedup is initially compiled into a ROW_NUMBER() OVER (PARTITION BY field) window pattern with an IS NOT NULL(field) bucket-non-null filter directly above the scan. PPLSimplifyDedupRule folds that pattern into a LogicalDedup which DedupPushdownRule then rewrites into a composite + top_hits aggregation against the shard.

The HEP program registers FilterMergeRule.DEFAULT and PPLSimplifyDedupRule.DEDUP_SIMPLIFY_RULE via addRuleCollection. When a user where precedes dedup, the user's filter sits adjacent to the bucket-non-null filter; FilterMergeRule can merge them into AND(IS NOT NULL(field), <user predicate>). The previous mayBeFilterFromBucketNonNull operand predicate required every conjunct of the AND to be IS NOT NULL($ref) (allMatch), so the merged shape failed the match. PPLSimplifyDedupRule never fired, no LogicalDedup was produced, and DedupPushdownRule had nothing to match — dedup fell through to the in-memory ROW_NUMBER window form on the coordinator, which exceeds timeouts on large indices.

Fix

Loosen the operand predicate at the source rather than reordering HEP rules.

• PlanUtils.mayBeFilterFromBucketNonNull now accepts the un-merged IS NOT NULL($ref) shape or any AND whose conjuncts contain at least oneIS NOT NULL($ref) (anyMatch instead of allMatch). The predicate is order-independent — the simplify rule fires whether or not FilterMergeRule ran first.
• PPLSimplifyDedupRule.apply splits the matched filter's conjuncts into (a) the IS NOT NULL on a partition key (absorbed by LogicalDedup) and (b) everything else (emitted as a separate filter below LogicalDedup), so any user predicate folded into the AND is preserved verbatim. A defensive bail-out: if no IS NOT NULL on a partition key is present in the matched filter, the rule returns without transforming.
• PlanUtils.isNotNullOnRef is promoted to public so PPLSimplifyDedupRule.isNotNullOnPartitionKey can reuse it.

Verification

• CalcitePPLDedupTest.testDedupAfterWhereProducesLogicalDedup runs the rule sequence (PPLSimplifyDedupRule, FilterMergeRule) on a where + dedup plan and asserts that LogicalDedup is produced regardless of rule order. The user predicate appears as a separate filter below LogicalDedup, ready for downstream pushdown:

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

• CalcitePPLDedupTest.testDedupAfterWhereProducesLogicalDedupRegardlessOfRuleOrder exercises the swapped order (FilterMergeRule first) and asserts the same result, pinning the user-visible contract rather than any rule-ordering invariant.

• CalciteExplainIT.testDedupAfterWherePushDown (new) — end-to-end against a live cluster: runs source=opensearch-sql_test_index_account | where age > 25 | dedup gender and asserts LogicalDedup is present in the explain output and EnumerableWindow is absent (i.e., dedup is actually pushed down and the in-memory window fallback the bug caused is gone).

• All existing CalcitePPLDedupTest cases continue to pass.

Related Issues

Resolves#5482

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.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 38fc6c3)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review Possible Issue When the filter is only partially pushable, the code bails out without transforming. However, the original dedup pattern (with the filter still present) remains in the plan. If the residual filter is later removed or merged by another rule pass, a subsequent optimizer iteration might re-match this same dedup pattern, potentially causing an infinite loop or redundant rule applications. The code should either mark the node as transformed or ensure the pattern cannot re-match. if (call.rels[1] instanceofLogicalFilterfilter) { // Push the filter into the scan, then synthesize an identity project so the standard// apply() can run on the resulting Dedup → Project → Scan shape. If the filter is only// partially pushable, pushDownFilter returns a Filter wrapping the residual condition over// the new scan; we can't strip a residual filter without breaking semantics, so bail.finalCalciteLogicalIndexScanscan = call.rel(2); if (!(scan.pushDownFilter(filter) instanceofCalciteLogicalIndexScannewScan)) { return; } RelBuilderrelBuilder = call.builder(); Possible Issue If dedupColumns contains a non-RexInputRef element (e.g., a literal or expression), partitionKeyIndices will not include it, but the dedup pattern may still be valid. The defensive check at line 148 will bail out even though a valid IS_NOT_NULL conjunct on an actual partition key might exist. This could silently skip legitimate dedup simplifications when partition keys include expressions. Set<Integer> partitionKeyIndices = newHashSet<>(); for (RexNodekey : dedupColumns) { if (keyinstanceofRexInputRefref) { partitionKeyIndices.add(ref.getIndex()); } } List<RexNode> bucketNonNullConjuncts = newArrayList<>(); List<RexNode> remainingConjuncts = newArrayList<>(); for (RexNodeconjunct : RelOptUtil.conjunctions(bucketNonNullFilter.getCondition())) { if (isNotNullOnPartitionKey(conjunct, partitionKeyIndices)) { bucketNonNullConjuncts.add(conjunct); } else { remainingConjuncts.add(conjunct); } } // Defensive: if no IS_NOT_NULL conjunct on a partition key is present, this filter is not// actually a bucket-non-null filter — bail out without transforming. The loose operand// predicate may have matched on an unrelated AND that happens to contain an IS_NOT_NULL on// some other ref.if (bucketNonNullConjuncts.isEmpty()) { return; }

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 38fc6c3

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Add type check before casting The code assumes call.rel(2) is always a CalciteLogicalIndexScan when call.rels[1] is a LogicalFilter, but this assumption isn't validated. If the operand structure changes or the rule matches unexpectedly, a ClassCastException could occur. Add a type check before casting. opensearch/src/main/java/org/opensearch/sql/opensearch/planner/rules/DedupPushdownRule.java [51-59] if (call.rels[1] instanceof LogicalFilter filter) { // Push the filter into the scan, then synthesize an identity project so the standard // apply() can run on the resulting Dedup → Project → Scan shape. If the filter is only // partially pushable, pushDownFilter returns a Filter wrapping the residual condition over // the new scan; we can't strip a residual filter without breaking semantics, so bail. - final CalciteLogicalIndexScan scan = call.rel(2);+ if (!(call.rel(2) instanceof CalciteLogicalIndexScan scan)) {+ return;+ } if (!(scan.pushDownFilter(filter) instanceof CalciteLogicalIndexScan newScan)) { return; } Suggestion importance[1-10]: 7 __ Why: Valid safety improvement. The operand structure is defined by the rule configuration, but adding an explicit type check before casting call.rel(2) to CalciteLogicalIndexScan prevents potential ClassCastException if the rule configuration changes or matches unexpectedly, improving robustness.	Medium
Possible issue	Validate no predicates are lost When no bucket-non-null conjuncts are found, the method returns early without transforming. However, the extracted remainingConjuncts are discarded, potentially losing user predicates. Consider logging a warning or validating that remainingConjuncts is also empty to ensure no predicates are silently dropped. core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.java [148-150] if (bucketNonNullConjuncts.isEmpty()) { + // Defensive: if no IS_NOT_NULL conjunct on a partition key is present, this filter is not+ // actually a bucket-non-null filter — bail out without transforming.+ if (!remainingConjuncts.isEmpty()) {+ // Log or handle unexpected case where user predicates exist but no bucket guards found+ throw new IllegalStateException("Expected bucket-non-null filter but found only user predicates");+ } return; } Suggestion importance[1-10]: 3 __ Why: The suggestion raises a valid concern about potentially discarding remainingConjuncts, but the current behavior is intentional: if no bucket-non-null conjuncts exist, the filter isn't a dedup pattern and the rule shouldn't transform. The remainingConjuncts remain in the original filter. Throwing an exception would be too aggressive for a defensive check.	Low

---

Previous suggestions

Suggestions up to commit 95e2aeb

Category	Suggestion	Impact
General	Handle partial filter pushdown explicitly The pattern match checks if pushDownFilter returns a CalciteLogicalIndexScan, but doesn't verify whether the filter was fully pushed down or if a residual filter remains. If pushDownFilter returns a LogicalFilter wrapping a scan (indicating partial pushdown), the cast fails silently and the method returns. Consider explicitly checking the return type and handling partial pushdown scenarios. opensearch/src/main/java/org/opensearch/sql/opensearch/planner/rules/DedupPushdownRule.java [57-59] -if (!(scan.pushDownFilter(filter) instanceof CalciteLogicalIndexScan newScan)) {+var pushResult = scan.pushDownFilter(filter);+if (!(pushResult instanceof CalciteLogicalIndexScan newScan)) {+ // Partial pushdown or unsupported filter - bail out return; } Suggestion importance[1-10]: 4 __ Why: The suggestion correctly identifies that the code checks for partial pushdown via the instanceof pattern match and bails out if a residual filter remains (as stated in the comment on lines 53-55). Extracting to a variable improves readability slightly, but the logic is already correct and the comment explains the behavior. The improvement is marginal.	Low
	Validate all conjuncts are classified The loop iterates over all conjunctions and classifies them into two lists. If the filter condition is complex or deeply nested, RelOptUtil.conjunctions may not flatten all levels of nested ANDs. Consider adding validation to ensure all conjuncts are properly classified, or document the assumption that only top-level ANDs are expected. core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.java [137-143] -for (RexNode conjunct : RelOptUtil.conjunctions(bucketNonNullFilter.getCondition())) {+List<RexNode> allConjuncts = RelOptUtil.conjunctions(bucketNonNullFilter.getCondition());+for (RexNode conjunct : allConjuncts) { if (isNotNullOnPartitionKey(conjunct, partitionKeyIndices)) { bucketNonNullConjuncts.add(conjunct); } else { remainingConjuncts.add(conjunct); } } +// Validate that all conjuncts were classified+assert bucketNonNullConjuncts.size() + remainingConjuncts.size() == allConjuncts.size(); Suggestion importance[1-10]: 2 __ Why: The suggestion adds an assertion to validate that all conjuncts are classified, but RelOptUtil.conjunctions is a standard Calcite utility that flattens top-level ANDs reliably. The assertion would always pass in normal operation and only catch programming errors. The concern about deeply nested ANDs is unfounded since conjunctions handles this. The added validation has minimal practical value.	Low
Possible issue	Validate remaining conjuncts before early return When no bucket-non-null conjuncts are found, the method returns early without transforming. However, the extracted remainingConjuncts list is not checked for emptiness. If remainingConjuncts is non-empty but bucketNonNullConjuncts is empty, user predicates could be silently dropped. Consider logging a warning or validating that remainingConjuncts is also empty before returning. core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.java [148-150] if (bucketNonNullConjuncts.isEmpty()) { + if (!remainingConjuncts.isEmpty()) {+ // Log or handle unexpected case: filter has user predicates but no bucket-non-null guards+ } return; } Suggestion importance[1-10]: 3 __ Why: The suggestion raises a valid edge case where remainingConjuncts could be non-empty while bucketNonNullConjuncts is empty. However, the early return is intentional per the defensive comment (lines 144-147), and silently dropping user predicates is unlikely since the operand predicate should prevent matching unrelated filters. The suggestion to log or validate is reasonable but has low impact.	Low

Suggestions up to commit a66ad5b

Category	Suggestion	Impact
General	Handle non-RexInputRef partition keys The code only handles RexInputRef instances when building partitionKeyIndices, silently ignoring other node types. If dedupColumns contains non-RexInputRef nodes (e.g., expressions), those partition keys won't be tracked, potentially causing incorrect classification of conjuncts. Consider handling or explicitly documenting this limitation. core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.java [130-134] for (RexNode key : dedupColumns) { if (key instanceof RexInputRef ref) { partitionKeyIndices.add(ref.getIndex()); + } else {+ // Log or handle non-RexInputRef partition keys if they're expected+ call.getPlanner().getContext().unwrap(org.slf4j.Logger.class)+ .warn("Unexpected partition key type: {}", key.getClass().getSimpleName()); } } Suggestion importance[1-10]: 4 __ Why: The suggestion correctly identifies that non-RexInputRef nodes in dedupColumns are silently ignored. However, based on the PR context and the dedup pattern, partition keys are expected to be simple column references (RexInputRef). Adding logging for unexpected types could help catch edge cases, but the current behavior appears intentional for the dedup use case. The suggestion provides moderate value for defensive programming.	Low
	Add logging for defensive check When no bucket-non-null conjuncts are found, the method returns early without transforming the plan. However, this leaves the original plan unchanged, which may cause downstream rules to fail or produce incorrect results. Consider logging a warning or throwing an exception to make this defensive check more explicit and aid debugging. core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.java [148-150] if (bucketNonNullConjuncts.isEmpty()) { + // Log or handle the case where the filter doesn't match the expected pattern+ call.getPlanner().getContext().unwrap(org.slf4j.Logger.class)+ .warn("Expected bucket-non-null filter pattern not found; skipping transformation"); return; } Suggestion importance[1-10]: 3 __ Why: While adding logging for the defensive check could aid debugging, the early return is already well-documented with a comment explaining why the transformation is skipped. The suggestion to add logging is a minor improvement that would help debugging but doesn't address a critical issue. The improved code also assumes a specific logging framework is available in the context, which may not be the case.	Low
	Log filter pushdown failures The pushDownFilter method may return null if the filter cannot be pushed down, but the code silently returns without any indication of why the transformation failed. This makes debugging difficult when pushdown unexpectedly fails. Consider logging the failure reason or the filter condition that couldn't be pushed. opensearch/src/main/java/org/opensearch/sql/opensearch/planner/rules/DedupPushdownRule.java [56-59] AbstractRelNode scanWithFilter = scan.pushDownFilter(filter); if (scanWithFilter == null) { + // Log when filter pushdown fails to aid debugging+ call.getPlanner().getContext().unwrap(org.slf4j.Logger.class)+ .debug("Filter pushdown failed for condition: {}", filter.getCondition()); return; } Suggestion importance[1-10]: 3 __ Why: Adding debug logging when pushDownFilter returns null could help with debugging, but this is a normal control flow path where the rule simply doesn't match. The null return indicates the filter cannot be pushed down, which is expected behavior in some cases. The suggestion provides marginal debugging value but doesn't fix any bug or improve correctness.	Low

Suggestions up to commit f67458f

Category	Suggestion	Impact
General	Add fallback for failed filter pushdown When pushDownFilter returns null, the method returns early without any transformation. This silently fails the optimization. Consider logging this case or falling back to the standard path to ensure the query still executes correctly, even if the filter cannot be pushed down. opensearch/src/main/java/org/opensearch/sql/opensearch/planner/rules/DedupPushdownRule.java [56-59] AbstractRelNode scanWithFilter = scan.pushDownFilter(filter); if (scanWithFilter == null) { + // Filter cannot be pushed down; fall back to standard dedup path+ // or log the failure for debugging return; } Suggestion importance[1-10]: 4 __ Why: The suggestion identifies a valid concern about the early return when pushDownFilter returns null. However, the current behavior is intentional - when filter pushdown fails, the rule simply doesn't match and other rules can handle the query. Adding a log for debugging could be helpful, but the suggestion to "fall back to standard dedup path" is vague since the early return already allows the standard path (without the filter optimization) to proceed via the else branch at line 70. The comment addition would provide minimal value.	Low
	Handle edge case for missing bucket guards When bucketNonNullConjuncts is empty, the method returns early without transforming. However, remainingConjuncts may contain valid user predicates that should still be processed. Consider logging a warning or handling the case where only non-partition-key predicates exist to avoid silently dropping valid filters. core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.java [148-150] if (bucketNonNullConjuncts.isEmpty()) { + // Log or handle case where filter doesn't match expected dedup pattern+ // but may contain valid user predicates return; } Suggestion importance[1-10]: 3 __ Why: The suggestion correctly identifies the early return when bucketNonNullConjuncts is empty, but the concern about "silently dropping valid filters" is misplaced. The code comment at lines 144-147 explicitly states this is a defensive check to bail out when the filter doesn't match the expected bucket-non-null pattern. The remainingConjuncts are only relevant when there are actual bucket guards present. Adding a log might be marginally useful for debugging, but doesn't address a real correctness issue.	Low

Suggestions up to commit 0bf8f7c

Category	Suggestion	Impact
General	Remove redundant operand check The !rexCall.getOperands().isEmpty() check is redundant because IS_NOT_NULL always has exactly one operand by definition. Accessing getOperands().get(0) without this check is safe and simplifies the logic. core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.java [174-180] private static boolean isNotNullOnPartitionKey(RexNode rex, Set<Integer> partitionKeyIndices) { return rex instanceof RexCall rexCall && rexCall.isA(SqlKind.IS_NOT_NULL) - && !rexCall.getOperands().isEmpty() && rexCall.getOperands().get(0) instanceof RexInputRef ref && partitionKeyIndices.contains(ref.getIndex()); } Suggestion importance[1-10]: 4 __ Why: While IS_NOT_NULL typically has exactly one operand, the check !rexCall.getOperands().isEmpty() provides defensive programming against malformed expressions. Removing it offers minimal benefit and slightly reduces code robustness. The same pattern is used consistently in PlanUtils.isNotNullOnRef at line 826, suggesting it's an intentional defensive practice.	Low
	Handle non-reference partition keys Handle non-RexInputRef partition keys gracefully. If dedupColumns contains complex expressions that aren't simple column references, they won't be added to partitionKeyIndices, potentially causing valid bucket-non-null filters to be incorrectly rejected. Consider logging a warning or adding a comment explaining this limitation. core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.java [130-134] for (RexNode key : dedupColumns) { if (key instanceof RexInputRef ref) { partitionKeyIndices.add(ref.getIndex()); } + // Note: Complex expressions in dedupColumns are not tracked as partition keys.+ // This may cause the rule to bail out if bucket-non-null filters reference them. } Suggestion importance[1-10]: 3 __ Why: The suggestion correctly identifies that non-RexInputRef partition keys won't be tracked, but this appears to be intentional design rather than a bug. Adding a comment could improve code clarity, but the impact is minimal since the defensive check at line 148 already handles this case by bailing out when no matching filters are found.	Low

Suggestions up to commit 19f506d

Category	Suggestion	Impact
General	Make filter assertion more specific The assertion checking for "1000" is too generic and may match unintended occurrences in the plan. Consider checking for a more specific pattern like ">($7, 1000)" or "SAL > 1000" to ensure the user's WHERE clause filter is actually present in the expected location. ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLDedupTest.java [380-386] String rawExplain = raw.explain(); Assert.assertTrue( "Raw plan should contain the bucket-non-null filter:\n" + rawExplain, rawExplain.contains("IS NOT NULL")); Assert.assertTrue( "Raw plan should contain the user where filter:\n" + rawExplain, - rawExplain.contains("1000"));+ rawExplain.contains(">") && rawExplain.contains("1000")); Suggestion importance[1-10]: 4 __ Why: The suggestion correctly identifies that checking for "1000" alone could match unintended occurrences. However, the improved code rawExplain.contains(">") && rawExplain.contains("1000") is still too generic and doesn't guarantee the filter is in the expected location. A more specific pattern would be better, but the suggestion does improve robustness slightly.	Low

[RyanL1997]

Hi @ryan-gh-bot , thanks for the change, left some reviews.

[github-actions]

Persistent review updated to latest commit 19f506d

[RyanL1997]

@ryan-gh-bot the rule-reordering fix works but treats a symptom rather than the root cause. The actual brittleness is in PlanUtils.mayBeFilterFromBucketNonNull — its allMatch(isNotNullOnRef) requirement means any future code path that produces a merged conjunction (a new rule, a different optimization order, a SQL-side caller) will silently re-disable dedup pushdown the same way. The HEP ordering is now a load-bearing invariant with nothing enforcing it; if anyone later adds a third rule to HEP_PROGRAM or refactors it, the bug comes back silently. The new testDedupAfterWhereWithFilterMergeFirstFailsToSimplify test pins the implementation strategy rather than the user-visible behavior.

Please revise to take option 1 from the issue instead:

1. Extend mayBeFilterFromBucketNonNull (core/.../utils/PlanUtils.java) to accept AND(IS_NOT_NULL(partition_col), <rest>) — i.e., recognize an AND whose operands contain an IS_NOT_NULL on the partition column rather than requiring every conjunct to be one.
2. In PPLSimplifyDedupRule.apply, split the non-IS_NOT_NULL conjuncts off and emit them as a separate filter below the new LogicalDedup, so the user predicate is preserved and pushdown still works.
3. Revert the addRuleCollection → addRuleInstance change in CalciteToolsHelper.HEP_PROGRAM — once the predicate is order-independent, rule order no longer matters.
4. Replace testDedupAfterWhereWithFilterMergeFirstFailsToSimplify (which pins buggy behavior) with a test that asserts the user-visible contract: with where upstream, dedup still produces a LogicalDedup regardless of rule order. Keep testDedupAfterWhereProducesLogicalDedup.

Please sanity-check that the loosened operand can't accidentally match user-written IS_NOT_NULL(x) AND ... filters outside the dedup pattern — the four-level operand chain (project → filter with row_number <= N → project containing ROW_NUMBER-dedup → filter) should be specific enough, but worth confirming.

[ryan-gh-bot]

Working on /@triage (invoked by @RyanL1997) — posting result when done.

[ryan-gh-bot]

Done — pushed 0bf8f7c73.

[github-actions]

Persistent review updated to latest commit 0bf8f7c

[dai-chen]

@ryan-gh-bot the rule-reordering fix works but treats a symptom rather than the root cause. The actual brittleness is in PlanUtils.mayBeFilterFromBucketNonNull — its allMatch(isNotNullOnRef) requirement means any future code path that produces a merged conjunction (a new rule, a different optimization order, a SQL-side caller) will silently re-disable dedup pushdown the same way. The HEP ordering is now a load-bearing invariant with nothing enforcing it; if anyone later adds a third rule to HEP_PROGRAM or refactors it, the bug comes back silently. The new testDedupAfterWhereWithFilterMergeFirstFailsToSimplify test pins the implementation strategy rather than the user-visible behavior.

Please revise to take option 1 from the issue instead:

1. Extend mayBeFilterFromBucketNonNull (core/.../utils/PlanUtils.java) to accept AND(IS_NOT_NULL(partition_col), <rest>) — i.e., recognize an AND whose operands contain an IS_NOT_NULL on the partition column rather than requiring every conjunct to be one.
2. In PPLSimplifyDedupRule.apply, split the non-IS_NOT_NULL conjuncts off and emit them as a separate filter below the new LogicalDedup, so the user predicate is preserved and pushdown still works.
3. Revert the addRuleCollection → addRuleInstance change in CalciteToolsHelper.HEP_PROGRAM — once the predicate is order-independent, rule order no longer matters.
4. Replace testDedupAfterWhereWithFilterMergeFirstFailsToSimplify (which pins buggy behavior) with a test that asserts the user-visible contract: with where upstream, dedup still produces a LogicalDedup regardless of rule order. Keep testDedupAfterWhereProducesLogicalDedup.

Please sanity-check that the loosened operand can't accidentally match user-written IS_NOT_NULL(x) AND ... filters outside the dedup pattern — the four-level operand chain (project → filter with row_number <= N → project containing ROW_NUMBER-dedup → filter) should be specific enough, but worth confirming.

I'm weighing the pros and cons of these two options:

• Option 1 — make PPLSimplifyDedupRule run first: If this rule runs before FilterMergeRule, it matches the original unmerged pattern and fixes the problem directly. The concern is that some other rule could run ahead of it and reintroduce the mismatch.
• Option 2 - current PR approach: Does this cover all the plan shapes visitDedupe can produce, or does it only fix the plain where + dedup case? e.g. dedup N, multiple dedup fields, and the sort/collation cases. Or these cases are not supported by DSL query anyway?

[penghuo]

1. Add end-to-end test asserts dedup is actually pushed down when where precedes dedup.
2. Refactor PR description to match the implementation.

[github-actions]

Persistent review updated to latest commit ae86ece

[RyanL1997]

Hi @penghuo, thanks for the feedback:

Add end-to-end test asserts dedup is actually pushed down when where precedes dedup.

added CalciteExplainIT.testDedupAfterWherePushDown

Refactor PR description to match the implementation.

PR description refactored — now describes the actual implementation (loosen predicate at source, not reorder HEP rules) instead of the bot's earlier reorder approach.

[RyanL1997]

Walked the axes against buildDedupNotNull / buildDedupOrNull and reasoned about each. The fix lives in two places — the mayBeFilterFromBucketNonNull operand predicate, and the conjunct split inside PPLSimplifyDedupRule#apply. As long as the bottom filter of the rule's 4-level operand chain still includes an IS NOT NULL($pk) somewhere (alone or in an AND), the rule fires and apply cleanly separates the partition-key guards from the user predicate.

Axis	Effect on bucket-non-null filter shape	Covered?	Why
dedup N (N ≥ 2)	N lives in the upper _row_number_ <= N filter, not the bucket-non-null filter. Bottom-filter shape is identical regardless of N.	Yes	Predicate match path is unchanged; apply reads N from the upper filter as before.
Multiple dedup fields (dedup a, b)	Bucket-non-null becomes AND(IS NOT NULL(a), IS NOT NULL(b)). With a user where, FilterMergeRule produces AND(IS NOT NULL(a), IS NOT NULL(b), <user predicate>).	Yes	The anyMatch predicate matches (multiple IS NOT NULL conjuncts present). apply absorbs all IS NOT NULL-on-partition-key conjuncts into LogicalDedup and emits the user predicate as a separate filter below it.
Sort/collation (| sort x | dedup a / | where p | sort x | dedup a)	stripInputSort pulls the prior sort out and threads it into ROW_NUMBER's ORDER BY. Bucket-non-null filter shape is unchanged.	Yes	apply extracts collation from the window. The user where can sit below or above the sort; either way the bottom filter is the same bucket-non-null shape (possibly merged with the user predicate).
keepempty=true (with or without where)	buildDedupOrNull does not emit a bucket-non-null filter at all. The 4-level operand chain has no matching bottom filter.	No, by design	The rule's operand chain doesn't match, so the rule never fires. Documented in code: PPLSimplifyDedupRule.java:101 (Since we cannot push down dedup with keepEmpty=true, we don't simplify that pattern) and DedupPushdownRule.java:218 (Cannot push dedup operator if keepEmpty=true). Unchanged by this PR.

Net: this PR covers the keepempty=false cases (the default and only pushable shape) across all four where + dedup <variant> combinations — simple, dedup N, multi-field, and sort/collation. The keepempty=true case is an existing, intentional non-pushable shape, unchanged here.

[RyanL1997]

Checking on the CI failures

[github-actions]

Persistent review updated to latest commit 7725e0b

[github-actions]

Persistent review updated to latest commit f67458f

[github-actions]

Persistent review updated to latest commit a66ad5b

[github-actions]

Persistent review updated to latest commit 95e2aeb

[RyanL1997]

The current CI failure is unrelated.

The failure is:

 * What went wrong:
Execution failed for task ':doctest:startPrometheus'.
> java.net.SocketTimeoutException: Read timed out

[github-actions]

Persistent review updated to latest commit 38fc6c3

[ryan-gh-bot]

Description

PPL dedup is initially compiled into a ROW_NUMBER() OVER (PARTITION BY field) window pattern with an IS NOT NULL(field) bucket-non-null filter directly above the scan. PPLSimplifyDedupRule folds that pattern into a LogicalDedup which DedupPushdownRule then rewrites into a composite + top_hits aggregation against the shard.

The HEP program registers FilterMergeRule.DEFAULT and PPLSimplifyDedupRule.DEDUP_SIMPLIFY_RULE via addRuleCollection. When a user where precedes dedup, the user's filter sits adjacent to the bucket-non-null filter; FilterMergeRule can merge them into AND(IS NOT NULL(field), <user predicate>). The previous mayBeFilterFromBucketNonNull operand predicate required every conjunct of the AND to be IS NOT NULL($ref) (allMatch), so the merged shape failed the match. PPLSimplifyDedupRule never fired, no LogicalDedup was produced, and DedupPushdownRule had nothing to match — dedup fell through to the in-memory ROW_NUMBER window form on the coordinator, which exceeds timeouts on large indices.

Fix

Loosen the operand predicate at the source rather than reordering HEP rules.

• PlanUtils.mayBeFilterFromBucketNonNull now accepts the un-merged IS NOT NULL($ref) shape or any AND whose conjuncts contain at least oneIS NOT NULL($ref) (anyMatch instead of allMatch). The predicate is order-independent — the simplify rule fires whether or not FilterMergeRule ran first.
• PPLSimplifyDedupRule.apply splits the matched filter's conjuncts into (a) the IS NOT NULL on a partition key (absorbed by LogicalDedup) and (b) everything else (emitted as a separate filter below LogicalDedup), so any user predicate folded into the AND is preserved verbatim. A defensive bail-out: if no IS NOT NULL on a partition key is present in the matched filter, the rule returns without transforming.
• PlanUtils.isNotNullOnRef is promoted to public so PPLSimplifyDedupRule.isNotNullOnPartitionKey can reuse it.

Verification

• CalcitePPLDedupTest.testDedupAfterWhereProducesLogicalDedup runs the rule sequence (PPLSimplifyDedupRule, FilterMergeRule) on a where + dedup plan and asserts that LogicalDedup is produced regardless of rule order. The user predicate appears as a separate filter below LogicalDedup, ready for downstream pushdown:

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

• CalcitePPLDedupTest.testDedupAfterWhereProducesLogicalDedupRegardlessOfRuleOrder exercises the swapped order (FilterMergeRule first) and asserts the same result, pinning the user-visible contract rather than any rule-ordering invariant.

• CalciteExplainIT.testDedupAfterWherePushDown (new) — end-to-end against a live cluster: runs source=opensearch-sql_test_index_account | where age > 25 | dedup gender and asserts LogicalDedup is present in the explain output and EnumerableWindow is absent (i.e., dedup is actually pushed down and the in-memory window fallback the bug caused is gone).

• All existing CalcitePPLDedupTest cases continue to pass.

Related Issues

Resolves#5482

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.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 38fc6c3)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review Possible Issue When the filter is only partially pushable, the code bails out without transforming. However, the original dedup pattern (with the filter still present) remains in the plan. If the residual filter is later removed or merged by another rule pass, a subsequent optimizer iteration might re-match this same dedup pattern, potentially causing an infinite loop or redundant rule applications. The code should either mark the node as transformed or ensure the pattern cannot re-match. if (call.rels[1] instanceofLogicalFilterfilter) { // Push the filter into the scan, then synthesize an identity project so the standard// apply() can run on the resulting Dedup → Project → Scan shape. If the filter is only// partially pushable, pushDownFilter returns a Filter wrapping the residual condition over// the new scan; we can't strip a residual filter without breaking semantics, so bail.finalCalciteLogicalIndexScanscan = call.rel(2); if (!(scan.pushDownFilter(filter) instanceofCalciteLogicalIndexScannewScan)) { return; } RelBuilderrelBuilder = call.builder(); Possible Issue If dedupColumns contains a non-RexInputRef element (e.g., a literal or expression), partitionKeyIndices will not include it, but the dedup pattern may still be valid. The defensive check at line 148 will bail out even though a valid IS_NOT_NULL conjunct on an actual partition key might exist. This could silently skip legitimate dedup simplifications when partition keys include expressions. Set<Integer> partitionKeyIndices = newHashSet<>(); for (RexNodekey : dedupColumns) { if (keyinstanceofRexInputRefref) { partitionKeyIndices.add(ref.getIndex()); } } List<RexNode> bucketNonNullConjuncts = newArrayList<>(); List<RexNode> remainingConjuncts = newArrayList<>(); for (RexNodeconjunct : RelOptUtil.conjunctions(bucketNonNullFilter.getCondition())) { if (isNotNullOnPartitionKey(conjunct, partitionKeyIndices)) { bucketNonNullConjuncts.add(conjunct); } else { remainingConjuncts.add(conjunct); } } // Defensive: if no IS_NOT_NULL conjunct on a partition key is present, this filter is not// actually a bucket-non-null filter — bail out without transforming. The loose operand// predicate may have matched on an unrelated AND that happens to contain an IS_NOT_NULL on// some other ref.if (bucketNonNullConjuncts.isEmpty()) { return; }

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 38fc6c3

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Add type check before casting The code assumes call.rel(2) is always a CalciteLogicalIndexScan when call.rels[1] is a LogicalFilter, but this assumption isn't validated. If the operand structure changes or the rule matches unexpectedly, a ClassCastException could occur. Add a type check before casting. opensearch/src/main/java/org/opensearch/sql/opensearch/planner/rules/DedupPushdownRule.java [51-59] if (call.rels[1] instanceof LogicalFilter filter) { // Push the filter into the scan, then synthesize an identity project so the standard // apply() can run on the resulting Dedup → Project → Scan shape. If the filter is only // partially pushable, pushDownFilter returns a Filter wrapping the residual condition over // the new scan; we can't strip a residual filter without breaking semantics, so bail. - final CalciteLogicalIndexScan scan = call.rel(2);+ if (!(call.rel(2) instanceof CalciteLogicalIndexScan scan)) {+ return;+ } if (!(scan.pushDownFilter(filter) instanceof CalciteLogicalIndexScan newScan)) { return; } Suggestion importance[1-10]: 7 __ Why: Valid safety improvement. The operand structure is defined by the rule configuration, but adding an explicit type check before casting call.rel(2) to CalciteLogicalIndexScan prevents potential ClassCastException if the rule configuration changes or matches unexpectedly, improving robustness.	Medium
Possible issue	Validate no predicates are lost When no bucket-non-null conjuncts are found, the method returns early without transforming. However, the extracted remainingConjuncts are discarded, potentially losing user predicates. Consider logging a warning or validating that remainingConjuncts is also empty to ensure no predicates are silently dropped. core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.java [148-150] if (bucketNonNullConjuncts.isEmpty()) { + // Defensive: if no IS_NOT_NULL conjunct on a partition key is present, this filter is not+ // actually a bucket-non-null filter — bail out without transforming.+ if (!remainingConjuncts.isEmpty()) {+ // Log or handle unexpected case where user predicates exist but no bucket guards found+ throw new IllegalStateException("Expected bucket-non-null filter but found only user predicates");+ } return; } Suggestion importance[1-10]: 3 __ Why: The suggestion raises a valid concern about potentially discarding remainingConjuncts, but the current behavior is intentional: if no bucket-non-null conjuncts exist, the filter isn't a dedup pattern and the rule shouldn't transform. The remainingConjuncts remain in the original filter. Throwing an exception would be too aggressive for a defensive check.	Low

---

Previous suggestions

Suggestions up to commit 95e2aeb

Category	Suggestion	Impact
General	Handle partial filter pushdown explicitly The pattern match checks if pushDownFilter returns a CalciteLogicalIndexScan, but doesn't verify whether the filter was fully pushed down or if a residual filter remains. If pushDownFilter returns a LogicalFilter wrapping a scan (indicating partial pushdown), the cast fails silently and the method returns. Consider explicitly checking the return type and handling partial pushdown scenarios. opensearch/src/main/java/org/opensearch/sql/opensearch/planner/rules/DedupPushdownRule.java [57-59] -if (!(scan.pushDownFilter(filter) instanceof CalciteLogicalIndexScan newScan)) {+var pushResult = scan.pushDownFilter(filter);+if (!(pushResult instanceof CalciteLogicalIndexScan newScan)) {+ // Partial pushdown or unsupported filter - bail out return; } Suggestion importance[1-10]: 4 __ Why: The suggestion correctly identifies that the code checks for partial pushdown via the instanceof pattern match and bails out if a residual filter remains (as stated in the comment on lines 53-55). Extracting to a variable improves readability slightly, but the logic is already correct and the comment explains the behavior. The improvement is marginal.	Low
	Validate all conjuncts are classified The loop iterates over all conjunctions and classifies them into two lists. If the filter condition is complex or deeply nested, RelOptUtil.conjunctions may not flatten all levels of nested ANDs. Consider adding validation to ensure all conjuncts are properly classified, or document the assumption that only top-level ANDs are expected. core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.java [137-143] -for (RexNode conjunct : RelOptUtil.conjunctions(bucketNonNullFilter.getCondition())) {+List<RexNode> allConjuncts = RelOptUtil.conjunctions(bucketNonNullFilter.getCondition());+for (RexNode conjunct : allConjuncts) { if (isNotNullOnPartitionKey(conjunct, partitionKeyIndices)) { bucketNonNullConjuncts.add(conjunct); } else { remainingConjuncts.add(conjunct); } } +// Validate that all conjuncts were classified+assert bucketNonNullConjuncts.size() + remainingConjuncts.size() == allConjuncts.size(); Suggestion importance[1-10]: 2 __ Why: The suggestion adds an assertion to validate that all conjuncts are classified, but RelOptUtil.conjunctions is a standard Calcite utility that flattens top-level ANDs reliably. The assertion would always pass in normal operation and only catch programming errors. The concern about deeply nested ANDs is unfounded since conjunctions handles this. The added validation has minimal practical value.	Low
Possible issue	Validate remaining conjuncts before early return When no bucket-non-null conjuncts are found, the method returns early without transforming. However, the extracted remainingConjuncts list is not checked for emptiness. If remainingConjuncts is non-empty but bucketNonNullConjuncts is empty, user predicates could be silently dropped. Consider logging a warning or validating that remainingConjuncts is also empty before returning. core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.java [148-150] if (bucketNonNullConjuncts.isEmpty()) { + if (!remainingConjuncts.isEmpty()) {+ // Log or handle unexpected case: filter has user predicates but no bucket-non-null guards+ } return; } Suggestion importance[1-10]: 3 __ Why: The suggestion raises a valid edge case where remainingConjuncts could be non-empty while bucketNonNullConjuncts is empty. However, the early return is intentional per the defensive comment (lines 144-147), and silently dropping user predicates is unlikely since the operand predicate should prevent matching unrelated filters. The suggestion to log or validate is reasonable but has low impact.	Low

Suggestions up to commit a66ad5b

Category	Suggestion	Impact
General	Handle non-RexInputRef partition keys The code only handles RexInputRef instances when building partitionKeyIndices, silently ignoring other node types. If dedupColumns contains non-RexInputRef nodes (e.g., expressions), those partition keys won't be tracked, potentially causing incorrect classification of conjuncts. Consider handling or explicitly documenting this limitation. core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.java [130-134] for (RexNode key : dedupColumns) { if (key instanceof RexInputRef ref) { partitionKeyIndices.add(ref.getIndex()); + } else {+ // Log or handle non-RexInputRef partition keys if they're expected+ call.getPlanner().getContext().unwrap(org.slf4j.Logger.class)+ .warn("Unexpected partition key type: {}", key.getClass().getSimpleName()); } } Suggestion importance[1-10]: 4 __ Why: The suggestion correctly identifies that non-RexInputRef nodes in dedupColumns are silently ignored. However, based on the PR context and the dedup pattern, partition keys are expected to be simple column references (RexInputRef). Adding logging for unexpected types could help catch edge cases, but the current behavior appears intentional for the dedup use case. The suggestion provides moderate value for defensive programming.	Low
	Add logging for defensive check When no bucket-non-null conjuncts are found, the method returns early without transforming the plan. However, this leaves the original plan unchanged, which may cause downstream rules to fail or produce incorrect results. Consider logging a warning or throwing an exception to make this defensive check more explicit and aid debugging. core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.java [148-150] if (bucketNonNullConjuncts.isEmpty()) { + // Log or handle the case where the filter doesn't match the expected pattern+ call.getPlanner().getContext().unwrap(org.slf4j.Logger.class)+ .warn("Expected bucket-non-null filter pattern not found; skipping transformation"); return; } Suggestion importance[1-10]: 3 __ Why: While adding logging for the defensive check could aid debugging, the early return is already well-documented with a comment explaining why the transformation is skipped. The suggestion to add logging is a minor improvement that would help debugging but doesn't address a critical issue. The improved code also assumes a specific logging framework is available in the context, which may not be the case.	Low
	Log filter pushdown failures The pushDownFilter method may return null if the filter cannot be pushed down, but the code silently returns without any indication of why the transformation failed. This makes debugging difficult when pushdown unexpectedly fails. Consider logging the failure reason or the filter condition that couldn't be pushed. opensearch/src/main/java/org/opensearch/sql/opensearch/planner/rules/DedupPushdownRule.java [56-59] AbstractRelNode scanWithFilter = scan.pushDownFilter(filter); if (scanWithFilter == null) { + // Log when filter pushdown fails to aid debugging+ call.getPlanner().getContext().unwrap(org.slf4j.Logger.class)+ .debug("Filter pushdown failed for condition: {}", filter.getCondition()); return; } Suggestion importance[1-10]: 3 __ Why: Adding debug logging when pushDownFilter returns null could help with debugging, but this is a normal control flow path where the rule simply doesn't match. The null return indicates the filter cannot be pushed down, which is expected behavior in some cases. The suggestion provides marginal debugging value but doesn't fix any bug or improve correctness.	Low

Suggestions up to commit f67458f

Category	Suggestion	Impact
General	Add fallback for failed filter pushdown When pushDownFilter returns null, the method returns early without any transformation. This silently fails the optimization. Consider logging this case or falling back to the standard path to ensure the query still executes correctly, even if the filter cannot be pushed down. opensearch/src/main/java/org/opensearch/sql/opensearch/planner/rules/DedupPushdownRule.java [56-59] AbstractRelNode scanWithFilter = scan.pushDownFilter(filter); if (scanWithFilter == null) { + // Filter cannot be pushed down; fall back to standard dedup path+ // or log the failure for debugging return; } Suggestion importance[1-10]: 4 __ Why: The suggestion identifies a valid concern about the early return when pushDownFilter returns null. However, the current behavior is intentional - when filter pushdown fails, the rule simply doesn't match and other rules can handle the query. Adding a log for debugging could be helpful, but the suggestion to "fall back to standard dedup path" is vague since the early return already allows the standard path (without the filter optimization) to proceed via the else branch at line 70. The comment addition would provide minimal value.	Low
	Handle edge case for missing bucket guards When bucketNonNullConjuncts is empty, the method returns early without transforming. However, remainingConjuncts may contain valid user predicates that should still be processed. Consider logging a warning or handling the case where only non-partition-key predicates exist to avoid silently dropping valid filters. core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.java [148-150] if (bucketNonNullConjuncts.isEmpty()) { + // Log or handle case where filter doesn't match expected dedup pattern+ // but may contain valid user predicates return; } Suggestion importance[1-10]: 3 __ Why: The suggestion correctly identifies the early return when bucketNonNullConjuncts is empty, but the concern about "silently dropping valid filters" is misplaced. The code comment at lines 144-147 explicitly states this is a defensive check to bail out when the filter doesn't match the expected bucket-non-null pattern. The remainingConjuncts are only relevant when there are actual bucket guards present. Adding a log might be marginally useful for debugging, but doesn't address a real correctness issue.	Low

Suggestions up to commit 0bf8f7c

Category	Suggestion	Impact
General	Remove redundant operand check The !rexCall.getOperands().isEmpty() check is redundant because IS_NOT_NULL always has exactly one operand by definition. Accessing getOperands().get(0) without this check is safe and simplifies the logic. core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.java [174-180] private static boolean isNotNullOnPartitionKey(RexNode rex, Set<Integer> partitionKeyIndices) { return rex instanceof RexCall rexCall && rexCall.isA(SqlKind.IS_NOT_NULL) - && !rexCall.getOperands().isEmpty() && rexCall.getOperands().get(0) instanceof RexInputRef ref && partitionKeyIndices.contains(ref.getIndex()); } Suggestion importance[1-10]: 4 __ Why: While IS_NOT_NULL typically has exactly one operand, the check !rexCall.getOperands().isEmpty() provides defensive programming against malformed expressions. Removing it offers minimal benefit and slightly reduces code robustness. The same pattern is used consistently in PlanUtils.isNotNullOnRef at line 826, suggesting it's an intentional defensive practice.	Low
	Handle non-reference partition keys Handle non-RexInputRef partition keys gracefully. If dedupColumns contains complex expressions that aren't simple column references, they won't be added to partitionKeyIndices, potentially causing valid bucket-non-null filters to be incorrectly rejected. Consider logging a warning or adding a comment explaining this limitation. core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.java [130-134] for (RexNode key : dedupColumns) { if (key instanceof RexInputRef ref) { partitionKeyIndices.add(ref.getIndex()); } + // Note: Complex expressions in dedupColumns are not tracked as partition keys.+ // This may cause the rule to bail out if bucket-non-null filters reference them. } Suggestion importance[1-10]: 3 __ Why: The suggestion correctly identifies that non-RexInputRef partition keys won't be tracked, but this appears to be intentional design rather than a bug. Adding a comment could improve code clarity, but the impact is minimal since the defensive check at line 148 already handles this case by bailing out when no matching filters are found.	Low

Suggestions up to commit 19f506d

Category	Suggestion	Impact
General	Make filter assertion more specific The assertion checking for "1000" is too generic and may match unintended occurrences in the plan. Consider checking for a more specific pattern like ">($7, 1000)" or "SAL > 1000" to ensure the user's WHERE clause filter is actually present in the expected location. ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLDedupTest.java [380-386] String rawExplain = raw.explain(); Assert.assertTrue( "Raw plan should contain the bucket-non-null filter:\n" + rawExplain, rawExplain.contains("IS NOT NULL")); Assert.assertTrue( "Raw plan should contain the user where filter:\n" + rawExplain, - rawExplain.contains("1000"));+ rawExplain.contains(">") && rawExplain.contains("1000")); Suggestion importance[1-10]: 4 __ Why: The suggestion correctly identifies that checking for "1000" alone could match unintended occurrences. However, the improved code rawExplain.contains(">") && rawExplain.contains("1000") is still too generic and doesn't guarantee the filter is in the expected location. A more specific pattern would be better, but the suggestion does improve robustness slightly.	Low

[RyanL1997]

Hi @ryan-gh-bot , thanks for the change, left some reviews.

[github-actions]

Persistent review updated to latest commit 19f506d

[RyanL1997]

@ryan-gh-bot the rule-reordering fix works but treats a symptom rather than the root cause. The actual brittleness is in PlanUtils.mayBeFilterFromBucketNonNull — its allMatch(isNotNullOnRef) requirement means any future code path that produces a merged conjunction (a new rule, a different optimization order, a SQL-side caller) will silently re-disable dedup pushdown the same way. The HEP ordering is now a load-bearing invariant with nothing enforcing it; if anyone later adds a third rule to HEP_PROGRAM or refactors it, the bug comes back silently. The new testDedupAfterWhereWithFilterMergeFirstFailsToSimplify test pins the implementation strategy rather than the user-visible behavior.

Please revise to take option 1 from the issue instead:

1. Extend mayBeFilterFromBucketNonNull (core/.../utils/PlanUtils.java) to accept AND(IS_NOT_NULL(partition_col), <rest>) — i.e., recognize an AND whose operands contain an IS_NOT_NULL on the partition column rather than requiring every conjunct to be one.
2. In PPLSimplifyDedupRule.apply, split the non-IS_NOT_NULL conjuncts off and emit them as a separate filter below the new LogicalDedup, so the user predicate is preserved and pushdown still works.
3. Revert the addRuleCollection → addRuleInstance change in CalciteToolsHelper.HEP_PROGRAM — once the predicate is order-independent, rule order no longer matters.
4. Replace testDedupAfterWhereWithFilterMergeFirstFailsToSimplify (which pins buggy behavior) with a test that asserts the user-visible contract: with where upstream, dedup still produces a LogicalDedup regardless of rule order. Keep testDedupAfterWhereProducesLogicalDedup.

Please sanity-check that the loosened operand can't accidentally match user-written IS_NOT_NULL(x) AND ... filters outside the dedup pattern — the four-level operand chain (project → filter with row_number <= N → project containing ROW_NUMBER-dedup → filter) should be specific enough, but worth confirming.

[ryan-gh-bot]

Working on /@triage (invoked by @RyanL1997) — posting result when done.

[ryan-gh-bot]

Done — pushed 0bf8f7c73.

[github-actions]

Persistent review updated to latest commit 0bf8f7c

[dai-chen]

@ryan-gh-bot the rule-reordering fix works but treats a symptom rather than the root cause. The actual brittleness is in PlanUtils.mayBeFilterFromBucketNonNull — its allMatch(isNotNullOnRef) requirement means any future code path that produces a merged conjunction (a new rule, a different optimization order, a SQL-side caller) will silently re-disable dedup pushdown the same way. The HEP ordering is now a load-bearing invariant with nothing enforcing it; if anyone later adds a third rule to HEP_PROGRAM or refactors it, the bug comes back silently. The new testDedupAfterWhereWithFilterMergeFirstFailsToSimplify test pins the implementation strategy rather than the user-visible behavior.

Please revise to take option 1 from the issue instead:

1. Extend mayBeFilterFromBucketNonNull (core/.../utils/PlanUtils.java) to accept AND(IS_NOT_NULL(partition_col), <rest>) — i.e., recognize an AND whose operands contain an IS_NOT_NULL on the partition column rather than requiring every conjunct to be one.
2. In PPLSimplifyDedupRule.apply, split the non-IS_NOT_NULL conjuncts off and emit them as a separate filter below the new LogicalDedup, so the user predicate is preserved and pushdown still works.
3. Revert the addRuleCollection → addRuleInstance change in CalciteToolsHelper.HEP_PROGRAM — once the predicate is order-independent, rule order no longer matters.
4. Replace testDedupAfterWhereWithFilterMergeFirstFailsToSimplify (which pins buggy behavior) with a test that asserts the user-visible contract: with where upstream, dedup still produces a LogicalDedup regardless of rule order. Keep testDedupAfterWhereProducesLogicalDedup.

Please sanity-check that the loosened operand can't accidentally match user-written IS_NOT_NULL(x) AND ... filters outside the dedup pattern — the four-level operand chain (project → filter with row_number <= N → project containing ROW_NUMBER-dedup → filter) should be specific enough, but worth confirming.

I'm weighing the pros and cons of these two options:

• Option 1 — make PPLSimplifyDedupRule run first: If this rule runs before FilterMergeRule, it matches the original unmerged pattern and fixes the problem directly. The concern is that some other rule could run ahead of it and reintroduce the mismatch.
• Option 2 - current PR approach: Does this cover all the plan shapes visitDedupe can produce, or does it only fix the plain where + dedup case? e.g. dedup N, multiple dedup fields, and the sort/collation cases. Or these cases are not supported by DSL query anyway?

[penghuo]

1. Add end-to-end test asserts dedup is actually pushed down when where precedes dedup.
2. Refactor PR description to match the implementation.

[github-actions]

Persistent review updated to latest commit ae86ece

[RyanL1997]

Hi @penghuo, thanks for the feedback:

Add end-to-end test asserts dedup is actually pushed down when where precedes dedup.

added CalciteExplainIT.testDedupAfterWherePushDown

Refactor PR description to match the implementation.

PR description refactored — now describes the actual implementation (loosen predicate at source, not reorder HEP rules) instead of the bot's earlier reorder approach.

[RyanL1997]

Walked the axes against buildDedupNotNull / buildDedupOrNull and reasoned about each. The fix lives in two places — the mayBeFilterFromBucketNonNull operand predicate, and the conjunct split inside PPLSimplifyDedupRule#apply. As long as the bottom filter of the rule's 4-level operand chain still includes an IS NOT NULL($pk) somewhere (alone or in an AND), the rule fires and apply cleanly separates the partition-key guards from the user predicate.

Axis	Effect on bucket-non-null filter shape	Covered?	Why
dedup N (N ≥ 2)	N lives in the upper _row_number_ <= N filter, not the bucket-non-null filter. Bottom-filter shape is identical regardless of N.	Yes	Predicate match path is unchanged; apply reads N from the upper filter as before.
Multiple dedup fields (dedup a, b)	Bucket-non-null becomes AND(IS NOT NULL(a), IS NOT NULL(b)). With a user where, FilterMergeRule produces AND(IS NOT NULL(a), IS NOT NULL(b), <user predicate>).	Yes	The anyMatch predicate matches (multiple IS NOT NULL conjuncts present). apply absorbs all IS NOT NULL-on-partition-key conjuncts into LogicalDedup and emits the user predicate as a separate filter below it.
Sort/collation (| sort x | dedup a / | where p | sort x | dedup a)	stripInputSort pulls the prior sort out and threads it into ROW_NUMBER's ORDER BY. Bucket-non-null filter shape is unchanged.	Yes	apply extracts collation from the window. The user where can sit below or above the sort; either way the bottom filter is the same bucket-non-null shape (possibly merged with the user predicate).
keepempty=true (with or without where)	buildDedupOrNull does not emit a bucket-non-null filter at all. The 4-level operand chain has no matching bottom filter.	No, by design	The rule's operand chain doesn't match, so the rule never fires. Documented in code: PPLSimplifyDedupRule.java:101 (Since we cannot push down dedup with keepEmpty=true, we don't simplify that pattern) and DedupPushdownRule.java:218 (Cannot push dedup operator if keepEmpty=true). Unchanged by this PR.

Net: this PR covers the keepempty=false cases (the default and only pushable shape) across all four where + dedup <variant> combinations — simple, dedup N, multi-field, and sort/collation. The keepempty=true case is an existing, intentional non-pushable shape, unchanged here.

[RyanL1997]

Checking on the CI failures

[github-actions]

Persistent review updated to latest commit 7725e0b

[github-actions]

Persistent review updated to latest commit f67458f

[github-actions]

Persistent review updated to latest commit a66ad5b

[github-actions]

Persistent review updated to latest commit 95e2aeb

[RyanL1997]

The current CI failure is unrelated.

The failure is:

 * What went wrong:
Execution failed for task ':doctest:startPrometheus'.
> java.net.SocketTimeoutException: Read timed out

[github-actions]

Persistent review updated to latest commit 38fc6c3

[ryan-gh-bot]

Description

PPL dedup is initially compiled into a ROW_NUMBER() OVER (PARTITION BY field) window pattern with an IS NOT NULL(field) bucket-non-null filter directly above the scan. PPLSimplifyDedupRule folds that pattern into a LogicalDedup which DedupPushdownRule then rewrites into a composite + top_hits aggregation against the shard.

The HEP program registers FilterMergeRule.DEFAULT and PPLSimplifyDedupRule.DEDUP_SIMPLIFY_RULE via addRuleCollection. When a user where precedes dedup, the user's filter sits adjacent to the bucket-non-null filter; FilterMergeRule can merge them into AND(IS NOT NULL(field), <user predicate>). The previous mayBeFilterFromBucketNonNull operand predicate required every conjunct of the AND to be IS NOT NULL($ref) (allMatch), so the merged shape failed the match. PPLSimplifyDedupRule never fired, no LogicalDedup was produced, and DedupPushdownRule had nothing to match — dedup fell through to the in-memory ROW_NUMBER window form on the coordinator, which exceeds timeouts on large indices.

Fix

Loosen the operand predicate at the source rather than reordering HEP rules.

• PlanUtils.mayBeFilterFromBucketNonNull now accepts the un-merged IS NOT NULL($ref) shape or any AND whose conjuncts contain at least oneIS NOT NULL($ref) (anyMatch instead of allMatch). The predicate is order-independent — the simplify rule fires whether or not FilterMergeRule ran first.
• PPLSimplifyDedupRule.apply splits the matched filter's conjuncts into (a) the IS NOT NULL on a partition key (absorbed by LogicalDedup) and (b) everything else (emitted as a separate filter below LogicalDedup), so any user predicate folded into the AND is preserved verbatim. A defensive bail-out: if no IS NOT NULL on a partition key is present in the matched filter, the rule returns without transforming.
• PlanUtils.isNotNullOnRef is promoted to public so PPLSimplifyDedupRule.isNotNullOnPartitionKey can reuse it.

Verification

• CalcitePPLDedupTest.testDedupAfterWhereProducesLogicalDedup runs the rule sequence (PPLSimplifyDedupRule, FilterMergeRule) on a where + dedup plan and asserts that LogicalDedup is produced regardless of rule order. The user predicate appears as a separate filter below LogicalDedup, ready for downstream pushdown:

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

• CalcitePPLDedupTest.testDedupAfterWhereProducesLogicalDedupRegardlessOfRuleOrder exercises the swapped order (FilterMergeRule first) and asserts the same result, pinning the user-visible contract rather than any rule-ordering invariant.

• CalciteExplainIT.testDedupAfterWherePushDown (new) — end-to-end against a live cluster: runs source=opensearch-sql_test_index_account | where age > 25 | dedup gender and asserts LogicalDedup is present in the explain output and EnumerableWindow is absent (i.e., dedup is actually pushed down and the in-memory window fallback the bug caused is gone).

• All existing CalcitePPLDedupTest cases continue to pass.

Related Issues

Resolves#5482

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.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 38fc6c3)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review Possible Issue When the filter is only partially pushable, the code bails out without transforming. However, the original dedup pattern (with the filter still present) remains in the plan. If the residual filter is later removed or merged by another rule pass, a subsequent optimizer iteration might re-match this same dedup pattern, potentially causing an infinite loop or redundant rule applications. The code should either mark the node as transformed or ensure the pattern cannot re-match. if (call.rels[1] instanceofLogicalFilterfilter) { // Push the filter into the scan, then synthesize an identity project so the standard// apply() can run on the resulting Dedup → Project → Scan shape. If the filter is only// partially pushable, pushDownFilter returns a Filter wrapping the residual condition over// the new scan; we can't strip a residual filter without breaking semantics, so bail.finalCalciteLogicalIndexScanscan = call.rel(2); if (!(scan.pushDownFilter(filter) instanceofCalciteLogicalIndexScannewScan)) { return; } RelBuilderrelBuilder = call.builder(); Possible Issue If dedupColumns contains a non-RexInputRef element (e.g., a literal or expression), partitionKeyIndices will not include it, but the dedup pattern may still be valid. The defensive check at line 148 will bail out even though a valid IS_NOT_NULL conjunct on an actual partition key might exist. This could silently skip legitimate dedup simplifications when partition keys include expressions. Set<Integer> partitionKeyIndices = newHashSet<>(); for (RexNodekey : dedupColumns) { if (keyinstanceofRexInputRefref) { partitionKeyIndices.add(ref.getIndex()); } } List<RexNode> bucketNonNullConjuncts = newArrayList<>(); List<RexNode> remainingConjuncts = newArrayList<>(); for (RexNodeconjunct : RelOptUtil.conjunctions(bucketNonNullFilter.getCondition())) { if (isNotNullOnPartitionKey(conjunct, partitionKeyIndices)) { bucketNonNullConjuncts.add(conjunct); } else { remainingConjuncts.add(conjunct); } } // Defensive: if no IS_NOT_NULL conjunct on a partition key is present, this filter is not// actually a bucket-non-null filter — bail out without transforming. The loose operand// predicate may have matched on an unrelated AND that happens to contain an IS_NOT_NULL on// some other ref.if (bucketNonNullConjuncts.isEmpty()) { return; }

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 38fc6c3

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Add type check before casting The code assumes call.rel(2) is always a CalciteLogicalIndexScan when call.rels[1] is a LogicalFilter, but this assumption isn't validated. If the operand structure changes or the rule matches unexpectedly, a ClassCastException could occur. Add a type check before casting. opensearch/src/main/java/org/opensearch/sql/opensearch/planner/rules/DedupPushdownRule.java [51-59] if (call.rels[1] instanceof LogicalFilter filter) { // Push the filter into the scan, then synthesize an identity project so the standard // apply() can run on the resulting Dedup → Project → Scan shape. If the filter is only // partially pushable, pushDownFilter returns a Filter wrapping the residual condition over // the new scan; we can't strip a residual filter without breaking semantics, so bail. - final CalciteLogicalIndexScan scan = call.rel(2);+ if (!(call.rel(2) instanceof CalciteLogicalIndexScan scan)) {+ return;+ } if (!(scan.pushDownFilter(filter) instanceof CalciteLogicalIndexScan newScan)) { return; } Suggestion importance[1-10]: 7 __ Why: Valid safety improvement. The operand structure is defined by the rule configuration, but adding an explicit type check before casting call.rel(2) to CalciteLogicalIndexScan prevents potential ClassCastException if the rule configuration changes or matches unexpectedly, improving robustness.	Medium
Possible issue	Validate no predicates are lost When no bucket-non-null conjuncts are found, the method returns early without transforming. However, the extracted remainingConjuncts are discarded, potentially losing user predicates. Consider logging a warning or validating that remainingConjuncts is also empty to ensure no predicates are silently dropped. core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.java [148-150] if (bucketNonNullConjuncts.isEmpty()) { + // Defensive: if no IS_NOT_NULL conjunct on a partition key is present, this filter is not+ // actually a bucket-non-null filter — bail out without transforming.+ if (!remainingConjuncts.isEmpty()) {+ // Log or handle unexpected case where user predicates exist but no bucket guards found+ throw new IllegalStateException("Expected bucket-non-null filter but found only user predicates");+ } return; } Suggestion importance[1-10]: 3 __ Why: The suggestion raises a valid concern about potentially discarding remainingConjuncts, but the current behavior is intentional: if no bucket-non-null conjuncts exist, the filter isn't a dedup pattern and the rule shouldn't transform. The remainingConjuncts remain in the original filter. Throwing an exception would be too aggressive for a defensive check.	Low

---

Previous suggestions

Suggestions up to commit 95e2aeb

Category	Suggestion	Impact
General	Handle partial filter pushdown explicitly The pattern match checks if pushDownFilter returns a CalciteLogicalIndexScan, but doesn't verify whether the filter was fully pushed down or if a residual filter remains. If pushDownFilter returns a LogicalFilter wrapping a scan (indicating partial pushdown), the cast fails silently and the method returns. Consider explicitly checking the return type and handling partial pushdown scenarios. opensearch/src/main/java/org/opensearch/sql/opensearch/planner/rules/DedupPushdownRule.java [57-59] -if (!(scan.pushDownFilter(filter) instanceof CalciteLogicalIndexScan newScan)) {+var pushResult = scan.pushDownFilter(filter);+if (!(pushResult instanceof CalciteLogicalIndexScan newScan)) {+ // Partial pushdown or unsupported filter - bail out return; } Suggestion importance[1-10]: 4 __ Why: The suggestion correctly identifies that the code checks for partial pushdown via the instanceof pattern match and bails out if a residual filter remains (as stated in the comment on lines 53-55). Extracting to a variable improves readability slightly, but the logic is already correct and the comment explains the behavior. The improvement is marginal.	Low
	Validate all conjuncts are classified The loop iterates over all conjunctions and classifies them into two lists. If the filter condition is complex or deeply nested, RelOptUtil.conjunctions may not flatten all levels of nested ANDs. Consider adding validation to ensure all conjuncts are properly classified, or document the assumption that only top-level ANDs are expected. core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.java [137-143] -for (RexNode conjunct : RelOptUtil.conjunctions(bucketNonNullFilter.getCondition())) {+List<RexNode> allConjuncts = RelOptUtil.conjunctions(bucketNonNullFilter.getCondition());+for (RexNode conjunct : allConjuncts) { if (isNotNullOnPartitionKey(conjunct, partitionKeyIndices)) { bucketNonNullConjuncts.add(conjunct); } else { remainingConjuncts.add(conjunct); } } +// Validate that all conjuncts were classified+assert bucketNonNullConjuncts.size() + remainingConjuncts.size() == allConjuncts.size(); Suggestion importance[1-10]: 2 __ Why: The suggestion adds an assertion to validate that all conjuncts are classified, but RelOptUtil.conjunctions is a standard Calcite utility that flattens top-level ANDs reliably. The assertion would always pass in normal operation and only catch programming errors. The concern about deeply nested ANDs is unfounded since conjunctions handles this. The added validation has minimal practical value.	Low
Possible issue	Validate remaining conjuncts before early return When no bucket-non-null conjuncts are found, the method returns early without transforming. However, the extracted remainingConjuncts list is not checked for emptiness. If remainingConjuncts is non-empty but bucketNonNullConjuncts is empty, user predicates could be silently dropped. Consider logging a warning or validating that remainingConjuncts is also empty before returning. core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.java [148-150] if (bucketNonNullConjuncts.isEmpty()) { + if (!remainingConjuncts.isEmpty()) {+ // Log or handle unexpected case: filter has user predicates but no bucket-non-null guards+ } return; } Suggestion importance[1-10]: 3 __ Why: The suggestion raises a valid edge case where remainingConjuncts could be non-empty while bucketNonNullConjuncts is empty. However, the early return is intentional per the defensive comment (lines 144-147), and silently dropping user predicates is unlikely since the operand predicate should prevent matching unrelated filters. The suggestion to log or validate is reasonable but has low impact.	Low

Suggestions up to commit a66ad5b

Category	Suggestion	Impact
General	Handle non-RexInputRef partition keys The code only handles RexInputRef instances when building partitionKeyIndices, silently ignoring other node types. If dedupColumns contains non-RexInputRef nodes (e.g., expressions), those partition keys won't be tracked, potentially causing incorrect classification of conjuncts. Consider handling or explicitly documenting this limitation. core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.java [130-134] for (RexNode key : dedupColumns) { if (key instanceof RexInputRef ref) { partitionKeyIndices.add(ref.getIndex()); + } else {+ // Log or handle non-RexInputRef partition keys if they're expected+ call.getPlanner().getContext().unwrap(org.slf4j.Logger.class)+ .warn("Unexpected partition key type: {}", key.getClass().getSimpleName()); } } Suggestion importance[1-10]: 4 __ Why: The suggestion correctly identifies that non-RexInputRef nodes in dedupColumns are silently ignored. However, based on the PR context and the dedup pattern, partition keys are expected to be simple column references (RexInputRef). Adding logging for unexpected types could help catch edge cases, but the current behavior appears intentional for the dedup use case. The suggestion provides moderate value for defensive programming.	Low
	Add logging for defensive check When no bucket-non-null conjuncts are found, the method returns early without transforming the plan. However, this leaves the original plan unchanged, which may cause downstream rules to fail or produce incorrect results. Consider logging a warning or throwing an exception to make this defensive check more explicit and aid debugging. core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.java [148-150] if (bucketNonNullConjuncts.isEmpty()) { + // Log or handle the case where the filter doesn't match the expected pattern+ call.getPlanner().getContext().unwrap(org.slf4j.Logger.class)+ .warn("Expected bucket-non-null filter pattern not found; skipping transformation"); return; } Suggestion importance[1-10]: 3 __ Why: While adding logging for the defensive check could aid debugging, the early return is already well-documented with a comment explaining why the transformation is skipped. The suggestion to add logging is a minor improvement that would help debugging but doesn't address a critical issue. The improved code also assumes a specific logging framework is available in the context, which may not be the case.	Low
	Log filter pushdown failures The pushDownFilter method may return null if the filter cannot be pushed down, but the code silently returns without any indication of why the transformation failed. This makes debugging difficult when pushdown unexpectedly fails. Consider logging the failure reason or the filter condition that couldn't be pushed. opensearch/src/main/java/org/opensearch/sql/opensearch/planner/rules/DedupPushdownRule.java [56-59] AbstractRelNode scanWithFilter = scan.pushDownFilter(filter); if (scanWithFilter == null) { + // Log when filter pushdown fails to aid debugging+ call.getPlanner().getContext().unwrap(org.slf4j.Logger.class)+ .debug("Filter pushdown failed for condition: {}", filter.getCondition()); return; } Suggestion importance[1-10]: 3 __ Why: Adding debug logging when pushDownFilter returns null could help with debugging, but this is a normal control flow path where the rule simply doesn't match. The null return indicates the filter cannot be pushed down, which is expected behavior in some cases. The suggestion provides marginal debugging value but doesn't fix any bug or improve correctness.	Low

Suggestions up to commit f67458f

Category	Suggestion	Impact
General	Add fallback for failed filter pushdown When pushDownFilter returns null, the method returns early without any transformation. This silently fails the optimization. Consider logging this case or falling back to the standard path to ensure the query still executes correctly, even if the filter cannot be pushed down. opensearch/src/main/java/org/opensearch/sql/opensearch/planner/rules/DedupPushdownRule.java [56-59] AbstractRelNode scanWithFilter = scan.pushDownFilter(filter); if (scanWithFilter == null) { + // Filter cannot be pushed down; fall back to standard dedup path+ // or log the failure for debugging return; } Suggestion importance[1-10]: 4 __ Why: The suggestion identifies a valid concern about the early return when pushDownFilter returns null. However, the current behavior is intentional - when filter pushdown fails, the rule simply doesn't match and other rules can handle the query. Adding a log for debugging could be helpful, but the suggestion to "fall back to standard dedup path" is vague since the early return already allows the standard path (without the filter optimization) to proceed via the else branch at line 70. The comment addition would provide minimal value.	Low
	Handle edge case for missing bucket guards When bucketNonNullConjuncts is empty, the method returns early without transforming. However, remainingConjuncts may contain valid user predicates that should still be processed. Consider logging a warning or handling the case where only non-partition-key predicates exist to avoid silently dropping valid filters. core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.java [148-150] if (bucketNonNullConjuncts.isEmpty()) { + // Log or handle case where filter doesn't match expected dedup pattern+ // but may contain valid user predicates return; } Suggestion importance[1-10]: 3 __ Why: The suggestion correctly identifies the early return when bucketNonNullConjuncts is empty, but the concern about "silently dropping valid filters" is misplaced. The code comment at lines 144-147 explicitly states this is a defensive check to bail out when the filter doesn't match the expected bucket-non-null pattern. The remainingConjuncts are only relevant when there are actual bucket guards present. Adding a log might be marginally useful for debugging, but doesn't address a real correctness issue.	Low

Suggestions up to commit 0bf8f7c

Category	Suggestion	Impact
General	Remove redundant operand check The !rexCall.getOperands().isEmpty() check is redundant because IS_NOT_NULL always has exactly one operand by definition. Accessing getOperands().get(0) without this check is safe and simplifies the logic. core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.java [174-180] private static boolean isNotNullOnPartitionKey(RexNode rex, Set<Integer> partitionKeyIndices) { return rex instanceof RexCall rexCall && rexCall.isA(SqlKind.IS_NOT_NULL) - && !rexCall.getOperands().isEmpty() && rexCall.getOperands().get(0) instanceof RexInputRef ref && partitionKeyIndices.contains(ref.getIndex()); } Suggestion importance[1-10]: 4 __ Why: While IS_NOT_NULL typically has exactly one operand, the check !rexCall.getOperands().isEmpty() provides defensive programming against malformed expressions. Removing it offers minimal benefit and slightly reduces code robustness. The same pattern is used consistently in PlanUtils.isNotNullOnRef at line 826, suggesting it's an intentional defensive practice.	Low
	Handle non-reference partition keys Handle non-RexInputRef partition keys gracefully. If dedupColumns contains complex expressions that aren't simple column references, they won't be added to partitionKeyIndices, potentially causing valid bucket-non-null filters to be incorrectly rejected. Consider logging a warning or adding a comment explaining this limitation. core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.java [130-134] for (RexNode key : dedupColumns) { if (key instanceof RexInputRef ref) { partitionKeyIndices.add(ref.getIndex()); } + // Note: Complex expressions in dedupColumns are not tracked as partition keys.+ // This may cause the rule to bail out if bucket-non-null filters reference them. } Suggestion importance[1-10]: 3 __ Why: The suggestion correctly identifies that non-RexInputRef partition keys won't be tracked, but this appears to be intentional design rather than a bug. Adding a comment could improve code clarity, but the impact is minimal since the defensive check at line 148 already handles this case by bailing out when no matching filters are found.	Low

Suggestions up to commit 19f506d

Category	Suggestion	Impact
General	Make filter assertion more specific The assertion checking for "1000" is too generic and may match unintended occurrences in the plan. Consider checking for a more specific pattern like ">($7, 1000)" or "SAL > 1000" to ensure the user's WHERE clause filter is actually present in the expected location. ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLDedupTest.java [380-386] String rawExplain = raw.explain(); Assert.assertTrue( "Raw plan should contain the bucket-non-null filter:\n" + rawExplain, rawExplain.contains("IS NOT NULL")); Assert.assertTrue( "Raw plan should contain the user where filter:\n" + rawExplain, - rawExplain.contains("1000"));+ rawExplain.contains(">") && rawExplain.contains("1000")); Suggestion importance[1-10]: 4 __ Why: The suggestion correctly identifies that checking for "1000" alone could match unintended occurrences. However, the improved code rawExplain.contains(">") && rawExplain.contains("1000") is still too generic and doesn't guarantee the filter is in the expected location. A more specific pattern would be better, but the suggestion does improve robustness slightly.	Low

[RyanL1997]

Hi @ryan-gh-bot , thanks for the change, left some reviews.

[github-actions]

Persistent review updated to latest commit 19f506d

[RyanL1997]

@ryan-gh-bot the rule-reordering fix works but treats a symptom rather than the root cause. The actual brittleness is in PlanUtils.mayBeFilterFromBucketNonNull — its allMatch(isNotNullOnRef) requirement means any future code path that produces a merged conjunction (a new rule, a different optimization order, a SQL-side caller) will silently re-disable dedup pushdown the same way. The HEP ordering is now a load-bearing invariant with nothing enforcing it; if anyone later adds a third rule to HEP_PROGRAM or refactors it, the bug comes back silently. The new testDedupAfterWhereWithFilterMergeFirstFailsToSimplify test pins the implementation strategy rather than the user-visible behavior.

Please revise to take option 1 from the issue instead:

1. Extend mayBeFilterFromBucketNonNull (core/.../utils/PlanUtils.java) to accept AND(IS_NOT_NULL(partition_col), <rest>) — i.e., recognize an AND whose operands contain an IS_NOT_NULL on the partition column rather than requiring every conjunct to be one.
2. In PPLSimplifyDedupRule.apply, split the non-IS_NOT_NULL conjuncts off and emit them as a separate filter below the new LogicalDedup, so the user predicate is preserved and pushdown still works.
3. Revert the addRuleCollection → addRuleInstance change in CalciteToolsHelper.HEP_PROGRAM — once the predicate is order-independent, rule order no longer matters.
4. Replace testDedupAfterWhereWithFilterMergeFirstFailsToSimplify (which pins buggy behavior) with a test that asserts the user-visible contract: with where upstream, dedup still produces a LogicalDedup regardless of rule order. Keep testDedupAfterWhereProducesLogicalDedup.

Please sanity-check that the loosened operand can't accidentally match user-written IS_NOT_NULL(x) AND ... filters outside the dedup pattern — the four-level operand chain (project → filter with row_number <= N → project containing ROW_NUMBER-dedup → filter) should be specific enough, but worth confirming.

[ryan-gh-bot]

Working on /@triage (invoked by @RyanL1997) — posting result when done.

[ryan-gh-bot]

Done — pushed 0bf8f7c73.

[github-actions]

Persistent review updated to latest commit 0bf8f7c

[dai-chen]

@ryan-gh-bot the rule-reordering fix works but treats a symptom rather than the root cause. The actual brittleness is in PlanUtils.mayBeFilterFromBucketNonNull — its allMatch(isNotNullOnRef) requirement means any future code path that produces a merged conjunction (a new rule, a different optimization order, a SQL-side caller) will silently re-disable dedup pushdown the same way. The HEP ordering is now a load-bearing invariant with nothing enforcing it; if anyone later adds a third rule to HEP_PROGRAM or refactors it, the bug comes back silently. The new testDedupAfterWhereWithFilterMergeFirstFailsToSimplify test pins the implementation strategy rather than the user-visible behavior.

Please revise to take option 1 from the issue instead:

1. Extend mayBeFilterFromBucketNonNull (core/.../utils/PlanUtils.java) to accept AND(IS_NOT_NULL(partition_col), <rest>) — i.e., recognize an AND whose operands contain an IS_NOT_NULL on the partition column rather than requiring every conjunct to be one.
2. In PPLSimplifyDedupRule.apply, split the non-IS_NOT_NULL conjuncts off and emit them as a separate filter below the new LogicalDedup, so the user predicate is preserved and pushdown still works.
3. Revert the addRuleCollection → addRuleInstance change in CalciteToolsHelper.HEP_PROGRAM — once the predicate is order-independent, rule order no longer matters.
4. Replace testDedupAfterWhereWithFilterMergeFirstFailsToSimplify (which pins buggy behavior) with a test that asserts the user-visible contract: with where upstream, dedup still produces a LogicalDedup regardless of rule order. Keep testDedupAfterWhereProducesLogicalDedup.

Please sanity-check that the loosened operand can't accidentally match user-written IS_NOT_NULL(x) AND ... filters outside the dedup pattern — the four-level operand chain (project → filter with row_number <= N → project containing ROW_NUMBER-dedup → filter) should be specific enough, but worth confirming.

I'm weighing the pros and cons of these two options:

• Option 1 — make PPLSimplifyDedupRule run first: If this rule runs before FilterMergeRule, it matches the original unmerged pattern and fixes the problem directly. The concern is that some other rule could run ahead of it and reintroduce the mismatch.
• Option 2 - current PR approach: Does this cover all the plan shapes visitDedupe can produce, or does it only fix the plain where + dedup case? e.g. dedup N, multiple dedup fields, and the sort/collation cases. Or these cases are not supported by DSL query anyway?

[penghuo]

1. Add end-to-end test asserts dedup is actually pushed down when where precedes dedup.
2. Refactor PR description to match the implementation.

[github-actions]

Persistent review updated to latest commit ae86ece

[RyanL1997]

Hi @penghuo, thanks for the feedback:

Add end-to-end test asserts dedup is actually pushed down when where precedes dedup.

added CalciteExplainIT.testDedupAfterWherePushDown

Refactor PR description to match the implementation.

PR description refactored — now describes the actual implementation (loosen predicate at source, not reorder HEP rules) instead of the bot's earlier reorder approach.

[RyanL1997]

Walked the axes against buildDedupNotNull / buildDedupOrNull and reasoned about each. The fix lives in two places — the mayBeFilterFromBucketNonNull operand predicate, and the conjunct split inside PPLSimplifyDedupRule#apply. As long as the bottom filter of the rule's 4-level operand chain still includes an IS NOT NULL($pk) somewhere (alone or in an AND), the rule fires and apply cleanly separates the partition-key guards from the user predicate.

Axis	Effect on bucket-non-null filter shape	Covered?	Why
dedup N (N ≥ 2)	N lives in the upper _row_number_ <= N filter, not the bucket-non-null filter. Bottom-filter shape is identical regardless of N.	Yes	Predicate match path is unchanged; apply reads N from the upper filter as before.
Multiple dedup fields (dedup a, b)	Bucket-non-null becomes AND(IS NOT NULL(a), IS NOT NULL(b)). With a user where, FilterMergeRule produces AND(IS NOT NULL(a), IS NOT NULL(b), <user predicate>).	Yes	The anyMatch predicate matches (multiple IS NOT NULL conjuncts present). apply absorbs all IS NOT NULL-on-partition-key conjuncts into LogicalDedup and emits the user predicate as a separate filter below it.
Sort/collation (| sort x | dedup a / | where p | sort x | dedup a)	stripInputSort pulls the prior sort out and threads it into ROW_NUMBER's ORDER BY. Bucket-non-null filter shape is unchanged.	Yes	apply extracts collation from the window. The user where can sit below or above the sort; either way the bottom filter is the same bucket-non-null shape (possibly merged with the user predicate).
keepempty=true (with or without where)	buildDedupOrNull does not emit a bucket-non-null filter at all. The 4-level operand chain has no matching bottom filter.	No, by design	The rule's operand chain doesn't match, so the rule never fires. Documented in code: PPLSimplifyDedupRule.java:101 (Since we cannot push down dedup with keepEmpty=true, we don't simplify that pattern) and DedupPushdownRule.java:218 (Cannot push dedup operator if keepEmpty=true). Unchanged by this PR.

Net: this PR covers the keepempty=false cases (the default and only pushable shape) across all four where + dedup <variant> combinations — simple, dedup N, multi-field, and sort/collation. The keepempty=true case is an existing, intentional non-pushable shape, unchanged here.

[RyanL1997]

Checking on the CI failures

[github-actions]

Persistent review updated to latest commit 7725e0b

[github-actions]

Persistent review updated to latest commit f67458f

[github-actions]

Persistent review updated to latest commit a66ad5b

[github-actions]

Persistent review updated to latest commit 95e2aeb

[RyanL1997]

The current CI failure is unrelated.

The failure is:

 * What went wrong:
Execution failed for task ':doctest:startPrometheus'.
> java.net.SocketTimeoutException: Read timed out

[github-actions]

Persistent review updated to latest commit 38fc6c3

[ryan-gh-bot]

Description

PPL dedup is initially compiled into a ROW_NUMBER() OVER (PARTITION BY field) window pattern with an IS NOT NULL(field) bucket-non-null filter directly above the scan. PPLSimplifyDedupRule folds that pattern into a LogicalDedup which DedupPushdownRule then rewrites into a composite + top_hits aggregation against the shard.

The HEP program registers FilterMergeRule.DEFAULT and PPLSimplifyDedupRule.DEDUP_SIMPLIFY_RULE via addRuleCollection. When a user where precedes dedup, the user's filter sits adjacent to the bucket-non-null filter; FilterMergeRule can merge them into AND(IS NOT NULL(field), <user predicate>). The previous mayBeFilterFromBucketNonNull operand predicate required every conjunct of the AND to be IS NOT NULL($ref) (allMatch), so the merged shape failed the match. PPLSimplifyDedupRule never fired, no LogicalDedup was produced, and DedupPushdownRule had nothing to match — dedup fell through to the in-memory ROW_NUMBER window form on the coordinator, which exceeds timeouts on large indices.

Fix

Loosen the operand predicate at the source rather than reordering HEP rules.

• PlanUtils.mayBeFilterFromBucketNonNull now accepts the un-merged IS NOT NULL($ref) shape or any AND whose conjuncts contain at least oneIS NOT NULL($ref) (anyMatch instead of allMatch). The predicate is order-independent — the simplify rule fires whether or not FilterMergeRule ran first.
• PPLSimplifyDedupRule.apply splits the matched filter's conjuncts into (a) the IS NOT NULL on a partition key (absorbed by LogicalDedup) and (b) everything else (emitted as a separate filter below LogicalDedup), so any user predicate folded into the AND is preserved verbatim. A defensive bail-out: if no IS NOT NULL on a partition key is present in the matched filter, the rule returns without transforming.
• PlanUtils.isNotNullOnRef is promoted to public so PPLSimplifyDedupRule.isNotNullOnPartitionKey can reuse it.

Verification

• CalcitePPLDedupTest.testDedupAfterWhereProducesLogicalDedup runs the rule sequence (PPLSimplifyDedupRule, FilterMergeRule) on a where + dedup plan and asserts that LogicalDedup is produced regardless of rule order. The user predicate appears as a separate filter below LogicalDedup, ready for downstream pushdown:

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

• CalcitePPLDedupTest.testDedupAfterWhereProducesLogicalDedupRegardlessOfRuleOrder exercises the swapped order (FilterMergeRule first) and asserts the same result, pinning the user-visible contract rather than any rule-ordering invariant.

• CalciteExplainIT.testDedupAfterWherePushDown (new) — end-to-end against a live cluster: runs source=opensearch-sql_test_index_account | where age > 25 | dedup gender and asserts LogicalDedup is present in the explain output and EnumerableWindow is absent (i.e., dedup is actually pushed down and the in-memory window fallback the bug caused is gone).

• All existing CalcitePPLDedupTest cases continue to pass.

Related Issues

Resolves#5482

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.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 38fc6c3)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review Possible Issue When the filter is only partially pushable, the code bails out without transforming. However, the original dedup pattern (with the filter still present) remains in the plan. If the residual filter is later removed or merged by another rule pass, a subsequent optimizer iteration might re-match this same dedup pattern, potentially causing an infinite loop or redundant rule applications. The code should either mark the node as transformed or ensure the pattern cannot re-match. if (call.rels[1] instanceofLogicalFilterfilter) { // Push the filter into the scan, then synthesize an identity project so the standard// apply() can run on the resulting Dedup → Project → Scan shape. If the filter is only// partially pushable, pushDownFilter returns a Filter wrapping the residual condition over// the new scan; we can't strip a residual filter without breaking semantics, so bail.finalCalciteLogicalIndexScanscan = call.rel(2); if (!(scan.pushDownFilter(filter) instanceofCalciteLogicalIndexScannewScan)) { return; } RelBuilderrelBuilder = call.builder(); Possible Issue If dedupColumns contains a non-RexInputRef element (e.g., a literal or expression), partitionKeyIndices will not include it, but the dedup pattern may still be valid. The defensive check at line 148 will bail out even though a valid IS_NOT_NULL conjunct on an actual partition key might exist. This could silently skip legitimate dedup simplifications when partition keys include expressions. Set<Integer> partitionKeyIndices = newHashSet<>(); for (RexNodekey : dedupColumns) { if (keyinstanceofRexInputRefref) { partitionKeyIndices.add(ref.getIndex()); } } List<RexNode> bucketNonNullConjuncts = newArrayList<>(); List<RexNode> remainingConjuncts = newArrayList<>(); for (RexNodeconjunct : RelOptUtil.conjunctions(bucketNonNullFilter.getCondition())) { if (isNotNullOnPartitionKey(conjunct, partitionKeyIndices)) { bucketNonNullConjuncts.add(conjunct); } else { remainingConjuncts.add(conjunct); } } // Defensive: if no IS_NOT_NULL conjunct on a partition key is present, this filter is not// actually a bucket-non-null filter — bail out without transforming. The loose operand// predicate may have matched on an unrelated AND that happens to contain an IS_NOT_NULL on// some other ref.if (bucketNonNullConjuncts.isEmpty()) { return; }

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 38fc6c3

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Add type check before casting The code assumes call.rel(2) is always a CalciteLogicalIndexScan when call.rels[1] is a LogicalFilter, but this assumption isn't validated. If the operand structure changes or the rule matches unexpectedly, a ClassCastException could occur. Add a type check before casting. opensearch/src/main/java/org/opensearch/sql/opensearch/planner/rules/DedupPushdownRule.java [51-59] if (call.rels[1] instanceof LogicalFilter filter) { // Push the filter into the scan, then synthesize an identity project so the standard // apply() can run on the resulting Dedup → Project → Scan shape. If the filter is only // partially pushable, pushDownFilter returns a Filter wrapping the residual condition over // the new scan; we can't strip a residual filter without breaking semantics, so bail. - final CalciteLogicalIndexScan scan = call.rel(2);+ if (!(call.rel(2) instanceof CalciteLogicalIndexScan scan)) {+ return;+ } if (!(scan.pushDownFilter(filter) instanceof CalciteLogicalIndexScan newScan)) { return; } Suggestion importance[1-10]: 7 __ Why: Valid safety improvement. The operand structure is defined by the rule configuration, but adding an explicit type check before casting call.rel(2) to CalciteLogicalIndexScan prevents potential ClassCastException if the rule configuration changes or matches unexpectedly, improving robustness.	Medium
Possible issue	Validate no predicates are lost When no bucket-non-null conjuncts are found, the method returns early without transforming. However, the extracted remainingConjuncts are discarded, potentially losing user predicates. Consider logging a warning or validating that remainingConjuncts is also empty to ensure no predicates are silently dropped. core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.java [148-150] if (bucketNonNullConjuncts.isEmpty()) { + // Defensive: if no IS_NOT_NULL conjunct on a partition key is present, this filter is not+ // actually a bucket-non-null filter — bail out without transforming.+ if (!remainingConjuncts.isEmpty()) {+ // Log or handle unexpected case where user predicates exist but no bucket guards found+ throw new IllegalStateException("Expected bucket-non-null filter but found only user predicates");+ } return; } Suggestion importance[1-10]: 3 __ Why: The suggestion raises a valid concern about potentially discarding remainingConjuncts, but the current behavior is intentional: if no bucket-non-null conjuncts exist, the filter isn't a dedup pattern and the rule shouldn't transform. The remainingConjuncts remain in the original filter. Throwing an exception would be too aggressive for a defensive check.	Low

---

Previous suggestions

Suggestions up to commit 95e2aeb

Category	Suggestion	Impact
General	Handle partial filter pushdown explicitly The pattern match checks if pushDownFilter returns a CalciteLogicalIndexScan, but doesn't verify whether the filter was fully pushed down or if a residual filter remains. If pushDownFilter returns a LogicalFilter wrapping a scan (indicating partial pushdown), the cast fails silently and the method returns. Consider explicitly checking the return type and handling partial pushdown scenarios. opensearch/src/main/java/org/opensearch/sql/opensearch/planner/rules/DedupPushdownRule.java [57-59] -if (!(scan.pushDownFilter(filter) instanceof CalciteLogicalIndexScan newScan)) {+var pushResult = scan.pushDownFilter(filter);+if (!(pushResult instanceof CalciteLogicalIndexScan newScan)) {+ // Partial pushdown or unsupported filter - bail out return; } Suggestion importance[1-10]: 4 __ Why: The suggestion correctly identifies that the code checks for partial pushdown via the instanceof pattern match and bails out if a residual filter remains (as stated in the comment on lines 53-55). Extracting to a variable improves readability slightly, but the logic is already correct and the comment explains the behavior. The improvement is marginal.	Low
	Validate all conjuncts are classified The loop iterates over all conjunctions and classifies them into two lists. If the filter condition is complex or deeply nested, RelOptUtil.conjunctions may not flatten all levels of nested ANDs. Consider adding validation to ensure all conjuncts are properly classified, or document the assumption that only top-level ANDs are expected. core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.java [137-143] -for (RexNode conjunct : RelOptUtil.conjunctions(bucketNonNullFilter.getCondition())) {+List<RexNode> allConjuncts = RelOptUtil.conjunctions(bucketNonNullFilter.getCondition());+for (RexNode conjunct : allConjuncts) { if (isNotNullOnPartitionKey(conjunct, partitionKeyIndices)) { bucketNonNullConjuncts.add(conjunct); } else { remainingConjuncts.add(conjunct); } } +// Validate that all conjuncts were classified+assert bucketNonNullConjuncts.size() + remainingConjuncts.size() == allConjuncts.size(); Suggestion importance[1-10]: 2 __ Why: The suggestion adds an assertion to validate that all conjuncts are classified, but RelOptUtil.conjunctions is a standard Calcite utility that flattens top-level ANDs reliably. The assertion would always pass in normal operation and only catch programming errors. The concern about deeply nested ANDs is unfounded since conjunctions handles this. The added validation has minimal practical value.	Low
Possible issue	Validate remaining conjuncts before early return When no bucket-non-null conjuncts are found, the method returns early without transforming. However, the extracted remainingConjuncts list is not checked for emptiness. If remainingConjuncts is non-empty but bucketNonNullConjuncts is empty, user predicates could be silently dropped. Consider logging a warning or validating that remainingConjuncts is also empty before returning. core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.java [148-150] if (bucketNonNullConjuncts.isEmpty()) { + if (!remainingConjuncts.isEmpty()) {+ // Log or handle unexpected case: filter has user predicates but no bucket-non-null guards+ } return; } Suggestion importance[1-10]: 3 __ Why: The suggestion raises a valid edge case where remainingConjuncts could be non-empty while bucketNonNullConjuncts is empty. However, the early return is intentional per the defensive comment (lines 144-147), and silently dropping user predicates is unlikely since the operand predicate should prevent matching unrelated filters. The suggestion to log or validate is reasonable but has low impact.	Low

Suggestions up to commit a66ad5b

Category	Suggestion	Impact
General	Handle non-RexInputRef partition keys The code only handles RexInputRef instances when building partitionKeyIndices, silently ignoring other node types. If dedupColumns contains non-RexInputRef nodes (e.g., expressions), those partition keys won't be tracked, potentially causing incorrect classification of conjuncts. Consider handling or explicitly documenting this limitation. core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.java [130-134] for (RexNode key : dedupColumns) { if (key instanceof RexInputRef ref) { partitionKeyIndices.add(ref.getIndex()); + } else {+ // Log or handle non-RexInputRef partition keys if they're expected+ call.getPlanner().getContext().unwrap(org.slf4j.Logger.class)+ .warn("Unexpected partition key type: {}", key.getClass().getSimpleName()); } } Suggestion importance[1-10]: 4 __ Why: The suggestion correctly identifies that non-RexInputRef nodes in dedupColumns are silently ignored. However, based on the PR context and the dedup pattern, partition keys are expected to be simple column references (RexInputRef). Adding logging for unexpected types could help catch edge cases, but the current behavior appears intentional for the dedup use case. The suggestion provides moderate value for defensive programming.	Low
	Add logging for defensive check When no bucket-non-null conjuncts are found, the method returns early without transforming the plan. However, this leaves the original plan unchanged, which may cause downstream rules to fail or produce incorrect results. Consider logging a warning or throwing an exception to make this defensive check more explicit and aid debugging. core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.java [148-150] if (bucketNonNullConjuncts.isEmpty()) { + // Log or handle the case where the filter doesn't match the expected pattern+ call.getPlanner().getContext().unwrap(org.slf4j.Logger.class)+ .warn("Expected bucket-non-null filter pattern not found; skipping transformation"); return; } Suggestion importance[1-10]: 3 __ Why: While adding logging for the defensive check could aid debugging, the early return is already well-documented with a comment explaining why the transformation is skipped. The suggestion to add logging is a minor improvement that would help debugging but doesn't address a critical issue. The improved code also assumes a specific logging framework is available in the context, which may not be the case.	Low
	Log filter pushdown failures The pushDownFilter method may return null if the filter cannot be pushed down, but the code silently returns without any indication of why the transformation failed. This makes debugging difficult when pushdown unexpectedly fails. Consider logging the failure reason or the filter condition that couldn't be pushed. opensearch/src/main/java/org/opensearch/sql/opensearch/planner/rules/DedupPushdownRule.java [56-59] AbstractRelNode scanWithFilter = scan.pushDownFilter(filter); if (scanWithFilter == null) { + // Log when filter pushdown fails to aid debugging+ call.getPlanner().getContext().unwrap(org.slf4j.Logger.class)+ .debug("Filter pushdown failed for condition: {}", filter.getCondition()); return; } Suggestion importance[1-10]: 3 __ Why: Adding debug logging when pushDownFilter returns null could help with debugging, but this is a normal control flow path where the rule simply doesn't match. The null return indicates the filter cannot be pushed down, which is expected behavior in some cases. The suggestion provides marginal debugging value but doesn't fix any bug or improve correctness.	Low

Suggestions up to commit f67458f

Category	Suggestion	Impact
General	Add fallback for failed filter pushdown When pushDownFilter returns null, the method returns early without any transformation. This silently fails the optimization. Consider logging this case or falling back to the standard path to ensure the query still executes correctly, even if the filter cannot be pushed down. opensearch/src/main/java/org/opensearch/sql/opensearch/planner/rules/DedupPushdownRule.java [56-59] AbstractRelNode scanWithFilter = scan.pushDownFilter(filter); if (scanWithFilter == null) { + // Filter cannot be pushed down; fall back to standard dedup path+ // or log the failure for debugging return; } Suggestion importance[1-10]: 4 __ Why: The suggestion identifies a valid concern about the early return when pushDownFilter returns null. However, the current behavior is intentional - when filter pushdown fails, the rule simply doesn't match and other rules can handle the query. Adding a log for debugging could be helpful, but the suggestion to "fall back to standard dedup path" is vague since the early return already allows the standard path (without the filter optimization) to proceed via the else branch at line 70. The comment addition would provide minimal value.	Low
	Handle edge case for missing bucket guards When bucketNonNullConjuncts is empty, the method returns early without transforming. However, remainingConjuncts may contain valid user predicates that should still be processed. Consider logging a warning or handling the case where only non-partition-key predicates exist to avoid silently dropping valid filters. core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.java [148-150] if (bucketNonNullConjuncts.isEmpty()) { + // Log or handle case where filter doesn't match expected dedup pattern+ // but may contain valid user predicates return; } Suggestion importance[1-10]: 3 __ Why: The suggestion correctly identifies the early return when bucketNonNullConjuncts is empty, but the concern about "silently dropping valid filters" is misplaced. The code comment at lines 144-147 explicitly states this is a defensive check to bail out when the filter doesn't match the expected bucket-non-null pattern. The remainingConjuncts are only relevant when there are actual bucket guards present. Adding a log might be marginally useful for debugging, but doesn't address a real correctness issue.	Low

Suggestions up to commit 0bf8f7c

Category	Suggestion	Impact
General	Remove redundant operand check The !rexCall.getOperands().isEmpty() check is redundant because IS_NOT_NULL always has exactly one operand by definition. Accessing getOperands().get(0) without this check is safe and simplifies the logic. core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.java [174-180] private static boolean isNotNullOnPartitionKey(RexNode rex, Set<Integer> partitionKeyIndices) { return rex instanceof RexCall rexCall && rexCall.isA(SqlKind.IS_NOT_NULL) - && !rexCall.getOperands().isEmpty() && rexCall.getOperands().get(0) instanceof RexInputRef ref && partitionKeyIndices.contains(ref.getIndex()); } Suggestion importance[1-10]: 4 __ Why: While IS_NOT_NULL typically has exactly one operand, the check !rexCall.getOperands().isEmpty() provides defensive programming against malformed expressions. Removing it offers minimal benefit and slightly reduces code robustness. The same pattern is used consistently in PlanUtils.isNotNullOnRef at line 826, suggesting it's an intentional defensive practice.	Low
	Handle non-reference partition keys Handle non-RexInputRef partition keys gracefully. If dedupColumns contains complex expressions that aren't simple column references, they won't be added to partitionKeyIndices, potentially causing valid bucket-non-null filters to be incorrectly rejected. Consider logging a warning or adding a comment explaining this limitation. core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.java [130-134] for (RexNode key : dedupColumns) { if (key instanceof RexInputRef ref) { partitionKeyIndices.add(ref.getIndex()); } + // Note: Complex expressions in dedupColumns are not tracked as partition keys.+ // This may cause the rule to bail out if bucket-non-null filters reference them. } Suggestion importance[1-10]: 3 __ Why: The suggestion correctly identifies that non-RexInputRef partition keys won't be tracked, but this appears to be intentional design rather than a bug. Adding a comment could improve code clarity, but the impact is minimal since the defensive check at line 148 already handles this case by bailing out when no matching filters are found.	Low

Suggestions up to commit 19f506d

Category	Suggestion	Impact
General	Make filter assertion more specific The assertion checking for "1000" is too generic and may match unintended occurrences in the plan. Consider checking for a more specific pattern like ">($7, 1000)" or "SAL > 1000" to ensure the user's WHERE clause filter is actually present in the expected location. ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLDedupTest.java [380-386] String rawExplain = raw.explain(); Assert.assertTrue( "Raw plan should contain the bucket-non-null filter:\n" + rawExplain, rawExplain.contains("IS NOT NULL")); Assert.assertTrue( "Raw plan should contain the user where filter:\n" + rawExplain, - rawExplain.contains("1000"));+ rawExplain.contains(">") && rawExplain.contains("1000")); Suggestion importance[1-10]: 4 __ Why: The suggestion correctly identifies that checking for "1000" alone could match unintended occurrences. However, the improved code rawExplain.contains(">") && rawExplain.contains("1000") is still too generic and doesn't guarantee the filter is in the expected location. A more specific pattern would be better, but the suggestion does improve robustness slightly.	Low

[RyanL1997]

Hi @ryan-gh-bot , thanks for the change, left some reviews.

[github-actions]

Persistent review updated to latest commit 19f506d

[RyanL1997]

@ryan-gh-bot the rule-reordering fix works but treats a symptom rather than the root cause. The actual brittleness is in PlanUtils.mayBeFilterFromBucketNonNull — its allMatch(isNotNullOnRef) requirement means any future code path that produces a merged conjunction (a new rule, a different optimization order, a SQL-side caller) will silently re-disable dedup pushdown the same way. The HEP ordering is now a load-bearing invariant with nothing enforcing it; if anyone later adds a third rule to HEP_PROGRAM or refactors it, the bug comes back silently. The new testDedupAfterWhereWithFilterMergeFirstFailsToSimplify test pins the implementation strategy rather than the user-visible behavior.

Please revise to take option 1 from the issue instead:

1. Extend mayBeFilterFromBucketNonNull (core/.../utils/PlanUtils.java) to accept AND(IS_NOT_NULL(partition_col), <rest>) — i.e., recognize an AND whose operands contain an IS_NOT_NULL on the partition column rather than requiring every conjunct to be one.
2. In PPLSimplifyDedupRule.apply, split the non-IS_NOT_NULL conjuncts off and emit them as a separate filter below the new LogicalDedup, so the user predicate is preserved and pushdown still works.
3. Revert the addRuleCollection → addRuleInstance change in CalciteToolsHelper.HEP_PROGRAM — once the predicate is order-independent, rule order no longer matters.
4. Replace testDedupAfterWhereWithFilterMergeFirstFailsToSimplify (which pins buggy behavior) with a test that asserts the user-visible contract: with where upstream, dedup still produces a LogicalDedup regardless of rule order. Keep testDedupAfterWhereProducesLogicalDedup.

Please sanity-check that the loosened operand can't accidentally match user-written IS_NOT_NULL(x) AND ... filters outside the dedup pattern — the four-level operand chain (project → filter with row_number <= N → project containing ROW_NUMBER-dedup → filter) should be specific enough, but worth confirming.

[ryan-gh-bot]

Working on /@triage (invoked by @RyanL1997) — posting result when done.

[ryan-gh-bot]

Done — pushed 0bf8f7c73.

[github-actions]

Persistent review updated to latest commit 0bf8f7c

[dai-chen]

@ryan-gh-bot the rule-reordering fix works but treats a symptom rather than the root cause. The actual brittleness is in PlanUtils.mayBeFilterFromBucketNonNull — its allMatch(isNotNullOnRef) requirement means any future code path that produces a merged conjunction (a new rule, a different optimization order, a SQL-side caller) will silently re-disable dedup pushdown the same way. The HEP ordering is now a load-bearing invariant with nothing enforcing it; if anyone later adds a third rule to HEP_PROGRAM or refactors it, the bug comes back silently. The new testDedupAfterWhereWithFilterMergeFirstFailsToSimplify test pins the implementation strategy rather than the user-visible behavior.

Please revise to take option 1 from the issue instead:

1. Extend mayBeFilterFromBucketNonNull (core/.../utils/PlanUtils.java) to accept AND(IS_NOT_NULL(partition_col), <rest>) — i.e., recognize an AND whose operands contain an IS_NOT_NULL on the partition column rather than requiring every conjunct to be one.
2. In PPLSimplifyDedupRule.apply, split the non-IS_NOT_NULL conjuncts off and emit them as a separate filter below the new LogicalDedup, so the user predicate is preserved and pushdown still works.
3. Revert the addRuleCollection → addRuleInstance change in CalciteToolsHelper.HEP_PROGRAM — once the predicate is order-independent, rule order no longer matters.
4. Replace testDedupAfterWhereWithFilterMergeFirstFailsToSimplify (which pins buggy behavior) with a test that asserts the user-visible contract: with where upstream, dedup still produces a LogicalDedup regardless of rule order. Keep testDedupAfterWhereProducesLogicalDedup.

Please sanity-check that the loosened operand can't accidentally match user-written IS_NOT_NULL(x) AND ... filters outside the dedup pattern — the four-level operand chain (project → filter with row_number <= N → project containing ROW_NUMBER-dedup → filter) should be specific enough, but worth confirming.

I'm weighing the pros and cons of these two options:

• Option 1 — make PPLSimplifyDedupRule run first: If this rule runs before FilterMergeRule, it matches the original unmerged pattern and fixes the problem directly. The concern is that some other rule could run ahead of it and reintroduce the mismatch.
• Option 2 - current PR approach: Does this cover all the plan shapes visitDedupe can produce, or does it only fix the plain where + dedup case? e.g. dedup N, multiple dedup fields, and the sort/collation cases. Or these cases are not supported by DSL query anyway?

[penghuo]

1. Add end-to-end test asserts dedup is actually pushed down when where precedes dedup.
2. Refactor PR description to match the implementation.

[github-actions]

Persistent review updated to latest commit ae86ece

[RyanL1997]

Hi @penghuo, thanks for the feedback:

Add end-to-end test asserts dedup is actually pushed down when where precedes dedup.

added CalciteExplainIT.testDedupAfterWherePushDown

Refactor PR description to match the implementation.

PR description refactored — now describes the actual implementation (loosen predicate at source, not reorder HEP rules) instead of the bot's earlier reorder approach.

[RyanL1997]

Walked the axes against buildDedupNotNull / buildDedupOrNull and reasoned about each. The fix lives in two places — the mayBeFilterFromBucketNonNull operand predicate, and the conjunct split inside PPLSimplifyDedupRule#apply. As long as the bottom filter of the rule's 4-level operand chain still includes an IS NOT NULL($pk) somewhere (alone or in an AND), the rule fires and apply cleanly separates the partition-key guards from the user predicate.

Axis	Effect on bucket-non-null filter shape	Covered?	Why
dedup N (N ≥ 2)	N lives in the upper _row_number_ <= N filter, not the bucket-non-null filter. Bottom-filter shape is identical regardless of N.	Yes	Predicate match path is unchanged; apply reads N from the upper filter as before.
Multiple dedup fields (dedup a, b)	Bucket-non-null becomes AND(IS NOT NULL(a), IS NOT NULL(b)). With a user where, FilterMergeRule produces AND(IS NOT NULL(a), IS NOT NULL(b), <user predicate>).	Yes	The anyMatch predicate matches (multiple IS NOT NULL conjuncts present). apply absorbs all IS NOT NULL-on-partition-key conjuncts into LogicalDedup and emits the user predicate as a separate filter below it.
Sort/collation (| sort x | dedup a / | where p | sort x | dedup a)	stripInputSort pulls the prior sort out and threads it into ROW_NUMBER's ORDER BY. Bucket-non-null filter shape is unchanged.	Yes	apply extracts collation from the window. The user where can sit below or above the sort; either way the bottom filter is the same bucket-non-null shape (possibly merged with the user predicate).
keepempty=true (with or without where)	buildDedupOrNull does not emit a bucket-non-null filter at all. The 4-level operand chain has no matching bottom filter.	No, by design	The rule's operand chain doesn't match, so the rule never fires. Documented in code: PPLSimplifyDedupRule.java:101 (Since we cannot push down dedup with keepEmpty=true, we don't simplify that pattern) and DedupPushdownRule.java:218 (Cannot push dedup operator if keepEmpty=true). Unchanged by this PR.

Net: this PR covers the keepempty=false cases (the default and only pushable shape) across all four where + dedup <variant> combinations — simple, dedup N, multi-field, and sort/collation. The keepempty=true case is an existing, intentional non-pushable shape, unchanged here.

[RyanL1997]

Checking on the CI failures

[github-actions]

Persistent review updated to latest commit 7725e0b

[github-actions]

Persistent review updated to latest commit f67458f

[github-actions]

Persistent review updated to latest commit a66ad5b

[github-actions]

Persistent review updated to latest commit 95e2aeb

[RyanL1997]

The current CI failure is unrelated.

The failure is:

 * What went wrong:
Execution failed for task ':doctest:startPrometheus'.
> java.net.SocketTimeoutException: Read timed out

[github-actions]

Persistent review updated to latest commit 38fc6c3

[ryan-gh-bot]

Description

PPL dedup is initially compiled into a ROW_NUMBER() OVER (PARTITION BY field) window pattern with an IS NOT NULL(field) bucket-non-null filter directly above the scan. PPLSimplifyDedupRule folds that pattern into a LogicalDedup which DedupPushdownRule then rewrites into a composite + top_hits aggregation against the shard.

The HEP program registers FilterMergeRule.DEFAULT and PPLSimplifyDedupRule.DEDUP_SIMPLIFY_RULE via addRuleCollection. When a user where precedes dedup, the user's filter sits adjacent to the bucket-non-null filter; FilterMergeRule can merge them into AND(IS NOT NULL(field), <user predicate>). The previous mayBeFilterFromBucketNonNull operand predicate required every conjunct of the AND to be IS NOT NULL($ref) (allMatch), so the merged shape failed the match. PPLSimplifyDedupRule never fired, no LogicalDedup was produced, and DedupPushdownRule had nothing to match — dedup fell through to the in-memory ROW_NUMBER window form on the coordinator, which exceeds timeouts on large indices.

Fix

Loosen the operand predicate at the source rather than reordering HEP rules.

• PlanUtils.mayBeFilterFromBucketNonNull now accepts the un-merged IS NOT NULL($ref) shape or any AND whose conjuncts contain at least oneIS NOT NULL($ref) (anyMatch instead of allMatch). The predicate is order-independent — the simplify rule fires whether or not FilterMergeRule ran first.
• PPLSimplifyDedupRule.apply splits the matched filter's conjuncts into (a) the IS NOT NULL on a partition key (absorbed by LogicalDedup) and (b) everything else (emitted as a separate filter below LogicalDedup), so any user predicate folded into the AND is preserved verbatim. A defensive bail-out: if no IS NOT NULL on a partition key is present in the matched filter, the rule returns without transforming.
• PlanUtils.isNotNullOnRef is promoted to public so PPLSimplifyDedupRule.isNotNullOnPartitionKey can reuse it.

Verification

• CalcitePPLDedupTest.testDedupAfterWhereProducesLogicalDedup runs the rule sequence (PPLSimplifyDedupRule, FilterMergeRule) on a where + dedup plan and asserts that LogicalDedup is produced regardless of rule order. The user predicate appears as a separate filter below LogicalDedup, ready for downstream pushdown:

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

• CalcitePPLDedupTest.testDedupAfterWhereProducesLogicalDedupRegardlessOfRuleOrder exercises the swapped order (FilterMergeRule first) and asserts the same result, pinning the user-visible contract rather than any rule-ordering invariant.

• CalciteExplainIT.testDedupAfterWherePushDown (new) — end-to-end against a live cluster: runs source=opensearch-sql_test_index_account | where age > 25 | dedup gender and asserts LogicalDedup is present in the explain output and EnumerableWindow is absent (i.e., dedup is actually pushed down and the in-memory window fallback the bug caused is gone).

• All existing CalcitePPLDedupTest cases continue to pass.

Related Issues

Resolves#5482

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.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 38fc6c3)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review Possible Issue When the filter is only partially pushable, the code bails out without transforming. However, the original dedup pattern (with the filter still present) remains in the plan. If the residual filter is later removed or merged by another rule pass, a subsequent optimizer iteration might re-match this same dedup pattern, potentially causing an infinite loop or redundant rule applications. The code should either mark the node as transformed or ensure the pattern cannot re-match. if (call.rels[1] instanceofLogicalFilterfilter) { // Push the filter into the scan, then synthesize an identity project so the standard// apply() can run on the resulting Dedup → Project → Scan shape. If the filter is only// partially pushable, pushDownFilter returns a Filter wrapping the residual condition over// the new scan; we can't strip a residual filter without breaking semantics, so bail.finalCalciteLogicalIndexScanscan = call.rel(2); if (!(scan.pushDownFilter(filter) instanceofCalciteLogicalIndexScannewScan)) { return; } RelBuilderrelBuilder = call.builder(); Possible Issue If dedupColumns contains a non-RexInputRef element (e.g., a literal or expression), partitionKeyIndices will not include it, but the dedup pattern may still be valid. The defensive check at line 148 will bail out even though a valid IS_NOT_NULL conjunct on an actual partition key might exist. This could silently skip legitimate dedup simplifications when partition keys include expressions. Set<Integer> partitionKeyIndices = newHashSet<>(); for (RexNodekey : dedupColumns) { if (keyinstanceofRexInputRefref) { partitionKeyIndices.add(ref.getIndex()); } } List<RexNode> bucketNonNullConjuncts = newArrayList<>(); List<RexNode> remainingConjuncts = newArrayList<>(); for (RexNodeconjunct : RelOptUtil.conjunctions(bucketNonNullFilter.getCondition())) { if (isNotNullOnPartitionKey(conjunct, partitionKeyIndices)) { bucketNonNullConjuncts.add(conjunct); } else { remainingConjuncts.add(conjunct); } } // Defensive: if no IS_NOT_NULL conjunct on a partition key is present, this filter is not// actually a bucket-non-null filter — bail out without transforming. The loose operand// predicate may have matched on an unrelated AND that happens to contain an IS_NOT_NULL on// some other ref.if (bucketNonNullConjuncts.isEmpty()) { return; }

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 38fc6c3

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Add type check before casting The code assumes call.rel(2) is always a CalciteLogicalIndexScan when call.rels[1] is a LogicalFilter, but this assumption isn't validated. If the operand structure changes or the rule matches unexpectedly, a ClassCastException could occur. Add a type check before casting. opensearch/src/main/java/org/opensearch/sql/opensearch/planner/rules/DedupPushdownRule.java [51-59] if (call.rels[1] instanceof LogicalFilter filter) { // Push the filter into the scan, then synthesize an identity project so the standard // apply() can run on the resulting Dedup → Project → Scan shape. If the filter is only // partially pushable, pushDownFilter returns a Filter wrapping the residual condition over // the new scan; we can't strip a residual filter without breaking semantics, so bail. - final CalciteLogicalIndexScan scan = call.rel(2);+ if (!(call.rel(2) instanceof CalciteLogicalIndexScan scan)) {+ return;+ } if (!(scan.pushDownFilter(filter) instanceof CalciteLogicalIndexScan newScan)) { return; } Suggestion importance[1-10]: 7 __ Why: Valid safety improvement. The operand structure is defined by the rule configuration, but adding an explicit type check before casting call.rel(2) to CalciteLogicalIndexScan prevents potential ClassCastException if the rule configuration changes or matches unexpectedly, improving robustness.	Medium
Possible issue	Validate no predicates are lost When no bucket-non-null conjuncts are found, the method returns early without transforming. However, the extracted remainingConjuncts are discarded, potentially losing user predicates. Consider logging a warning or validating that remainingConjuncts is also empty to ensure no predicates are silently dropped. core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.java [148-150] if (bucketNonNullConjuncts.isEmpty()) { + // Defensive: if no IS_NOT_NULL conjunct on a partition key is present, this filter is not+ // actually a bucket-non-null filter — bail out without transforming.+ if (!remainingConjuncts.isEmpty()) {+ // Log or handle unexpected case where user predicates exist but no bucket guards found+ throw new IllegalStateException("Expected bucket-non-null filter but found only user predicates");+ } return; } Suggestion importance[1-10]: 3 __ Why: The suggestion raises a valid concern about potentially discarding remainingConjuncts, but the current behavior is intentional: if no bucket-non-null conjuncts exist, the filter isn't a dedup pattern and the rule shouldn't transform. The remainingConjuncts remain in the original filter. Throwing an exception would be too aggressive for a defensive check.	Low

---

Previous suggestions

Suggestions up to commit 95e2aeb

Category	Suggestion	Impact
General	Handle partial filter pushdown explicitly The pattern match checks if pushDownFilter returns a CalciteLogicalIndexScan, but doesn't verify whether the filter was fully pushed down or if a residual filter remains. If pushDownFilter returns a LogicalFilter wrapping a scan (indicating partial pushdown), the cast fails silently and the method returns. Consider explicitly checking the return type and handling partial pushdown scenarios. opensearch/src/main/java/org/opensearch/sql/opensearch/planner/rules/DedupPushdownRule.java [57-59] -if (!(scan.pushDownFilter(filter) instanceof CalciteLogicalIndexScan newScan)) {+var pushResult = scan.pushDownFilter(filter);+if (!(pushResult instanceof CalciteLogicalIndexScan newScan)) {+ // Partial pushdown or unsupported filter - bail out return; } Suggestion importance[1-10]: 4 __ Why: The suggestion correctly identifies that the code checks for partial pushdown via the instanceof pattern match and bails out if a residual filter remains (as stated in the comment on lines 53-55). Extracting to a variable improves readability slightly, but the logic is already correct and the comment explains the behavior. The improvement is marginal.	Low
	Validate all conjuncts are classified The loop iterates over all conjunctions and classifies them into two lists. If the filter condition is complex or deeply nested, RelOptUtil.conjunctions may not flatten all levels of nested ANDs. Consider adding validation to ensure all conjuncts are properly classified, or document the assumption that only top-level ANDs are expected. core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.java [137-143] -for (RexNode conjunct : RelOptUtil.conjunctions(bucketNonNullFilter.getCondition())) {+List<RexNode> allConjuncts = RelOptUtil.conjunctions(bucketNonNullFilter.getCondition());+for (RexNode conjunct : allConjuncts) { if (isNotNullOnPartitionKey(conjunct, partitionKeyIndices)) { bucketNonNullConjuncts.add(conjunct); } else { remainingConjuncts.add(conjunct); } } +// Validate that all conjuncts were classified+assert bucketNonNullConjuncts.size() + remainingConjuncts.size() == allConjuncts.size(); Suggestion importance[1-10]: 2 __ Why: The suggestion adds an assertion to validate that all conjuncts are classified, but RelOptUtil.conjunctions is a standard Calcite utility that flattens top-level ANDs reliably. The assertion would always pass in normal operation and only catch programming errors. The concern about deeply nested ANDs is unfounded since conjunctions handles this. The added validation has minimal practical value.	Low
Possible issue	Validate remaining conjuncts before early return When no bucket-non-null conjuncts are found, the method returns early without transforming. However, the extracted remainingConjuncts list is not checked for emptiness. If remainingConjuncts is non-empty but bucketNonNullConjuncts is empty, user predicates could be silently dropped. Consider logging a warning or validating that remainingConjuncts is also empty before returning. core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.java [148-150] if (bucketNonNullConjuncts.isEmpty()) { + if (!remainingConjuncts.isEmpty()) {+ // Log or handle unexpected case: filter has user predicates but no bucket-non-null guards+ } return; } Suggestion importance[1-10]: 3 __ Why: The suggestion raises a valid edge case where remainingConjuncts could be non-empty while bucketNonNullConjuncts is empty. However, the early return is intentional per the defensive comment (lines 144-147), and silently dropping user predicates is unlikely since the operand predicate should prevent matching unrelated filters. The suggestion to log or validate is reasonable but has low impact.	Low

Suggestions up to commit a66ad5b

Category	Suggestion	Impact
General	Handle non-RexInputRef partition keys The code only handles RexInputRef instances when building partitionKeyIndices, silently ignoring other node types. If dedupColumns contains non-RexInputRef nodes (e.g., expressions), those partition keys won't be tracked, potentially causing incorrect classification of conjuncts. Consider handling or explicitly documenting this limitation. core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.java [130-134] for (RexNode key : dedupColumns) { if (key instanceof RexInputRef ref) { partitionKeyIndices.add(ref.getIndex()); + } else {+ // Log or handle non-RexInputRef partition keys if they're expected+ call.getPlanner().getContext().unwrap(org.slf4j.Logger.class)+ .warn("Unexpected partition key type: {}", key.getClass().getSimpleName()); } } Suggestion importance[1-10]: 4 __ Why: The suggestion correctly identifies that non-RexInputRef nodes in dedupColumns are silently ignored. However, based on the PR context and the dedup pattern, partition keys are expected to be simple column references (RexInputRef). Adding logging for unexpected types could help catch edge cases, but the current behavior appears intentional for the dedup use case. The suggestion provides moderate value for defensive programming.	Low
	Add logging for defensive check When no bucket-non-null conjuncts are found, the method returns early without transforming the plan. However, this leaves the original plan unchanged, which may cause downstream rules to fail or produce incorrect results. Consider logging a warning or throwing an exception to make this defensive check more explicit and aid debugging. core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.java [148-150] if (bucketNonNullConjuncts.isEmpty()) { + // Log or handle the case where the filter doesn't match the expected pattern+ call.getPlanner().getContext().unwrap(org.slf4j.Logger.class)+ .warn("Expected bucket-non-null filter pattern not found; skipping transformation"); return; } Suggestion importance[1-10]: 3 __ Why: While adding logging for the defensive check could aid debugging, the early return is already well-documented with a comment explaining why the transformation is skipped. The suggestion to add logging is a minor improvement that would help debugging but doesn't address a critical issue. The improved code also assumes a specific logging framework is available in the context, which may not be the case.	Low
	Log filter pushdown failures The pushDownFilter method may return null if the filter cannot be pushed down, but the code silently returns without any indication of why the transformation failed. This makes debugging difficult when pushdown unexpectedly fails. Consider logging the failure reason or the filter condition that couldn't be pushed. opensearch/src/main/java/org/opensearch/sql/opensearch/planner/rules/DedupPushdownRule.java [56-59] AbstractRelNode scanWithFilter = scan.pushDownFilter(filter); if (scanWithFilter == null) { + // Log when filter pushdown fails to aid debugging+ call.getPlanner().getContext().unwrap(org.slf4j.Logger.class)+ .debug("Filter pushdown failed for condition: {}", filter.getCondition()); return; } Suggestion importance[1-10]: 3 __ Why: Adding debug logging when pushDownFilter returns null could help with debugging, but this is a normal control flow path where the rule simply doesn't match. The null return indicates the filter cannot be pushed down, which is expected behavior in some cases. The suggestion provides marginal debugging value but doesn't fix any bug or improve correctness.	Low

Suggestions up to commit f67458f

Category	Suggestion	Impact
General	Add fallback for failed filter pushdown When pushDownFilter returns null, the method returns early without any transformation. This silently fails the optimization. Consider logging this case or falling back to the standard path to ensure the query still executes correctly, even if the filter cannot be pushed down. opensearch/src/main/java/org/opensearch/sql/opensearch/planner/rules/DedupPushdownRule.java [56-59] AbstractRelNode scanWithFilter = scan.pushDownFilter(filter); if (scanWithFilter == null) { + // Filter cannot be pushed down; fall back to standard dedup path+ // or log the failure for debugging return; } Suggestion importance[1-10]: 4 __ Why: The suggestion identifies a valid concern about the early return when pushDownFilter returns null. However, the current behavior is intentional - when filter pushdown fails, the rule simply doesn't match and other rules can handle the query. Adding a log for debugging could be helpful, but the suggestion to "fall back to standard dedup path" is vague since the early return already allows the standard path (without the filter optimization) to proceed via the else branch at line 70. The comment addition would provide minimal value.	Low
	Handle edge case for missing bucket guards When bucketNonNullConjuncts is empty, the method returns early without transforming. However, remainingConjuncts may contain valid user predicates that should still be processed. Consider logging a warning or handling the case where only non-partition-key predicates exist to avoid silently dropping valid filters. core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.java [148-150] if (bucketNonNullConjuncts.isEmpty()) { + // Log or handle case where filter doesn't match expected dedup pattern+ // but may contain valid user predicates return; } Suggestion importance[1-10]: 3 __ Why: The suggestion correctly identifies the early return when bucketNonNullConjuncts is empty, but the concern about "silently dropping valid filters" is misplaced. The code comment at lines 144-147 explicitly states this is a defensive check to bail out when the filter doesn't match the expected bucket-non-null pattern. The remainingConjuncts are only relevant when there are actual bucket guards present. Adding a log might be marginally useful for debugging, but doesn't address a real correctness issue.	Low

Suggestions up to commit 0bf8f7c

Category	Suggestion	Impact
General	Remove redundant operand check The !rexCall.getOperands().isEmpty() check is redundant because IS_NOT_NULL always has exactly one operand by definition. Accessing getOperands().get(0) without this check is safe and simplifies the logic. core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.java [174-180] private static boolean isNotNullOnPartitionKey(RexNode rex, Set<Integer> partitionKeyIndices) { return rex instanceof RexCall rexCall && rexCall.isA(SqlKind.IS_NOT_NULL) - && !rexCall.getOperands().isEmpty() && rexCall.getOperands().get(0) instanceof RexInputRef ref && partitionKeyIndices.contains(ref.getIndex()); } Suggestion importance[1-10]: 4 __ Why: While IS_NOT_NULL typically has exactly one operand, the check !rexCall.getOperands().isEmpty() provides defensive programming against malformed expressions. Removing it offers minimal benefit and slightly reduces code robustness. The same pattern is used consistently in PlanUtils.isNotNullOnRef at line 826, suggesting it's an intentional defensive practice.	Low
	Handle non-reference partition keys Handle non-RexInputRef partition keys gracefully. If dedupColumns contains complex expressions that aren't simple column references, they won't be added to partitionKeyIndices, potentially causing valid bucket-non-null filters to be incorrectly rejected. Consider logging a warning or adding a comment explaining this limitation. core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.java [130-134] for (RexNode key : dedupColumns) { if (key instanceof RexInputRef ref) { partitionKeyIndices.add(ref.getIndex()); } + // Note: Complex expressions in dedupColumns are not tracked as partition keys.+ // This may cause the rule to bail out if bucket-non-null filters reference them. } Suggestion importance[1-10]: 3 __ Why: The suggestion correctly identifies that non-RexInputRef partition keys won't be tracked, but this appears to be intentional design rather than a bug. Adding a comment could improve code clarity, but the impact is minimal since the defensive check at line 148 already handles this case by bailing out when no matching filters are found.	Low

Suggestions up to commit 19f506d

Category	Suggestion	Impact
General	Make filter assertion more specific The assertion checking for "1000" is too generic and may match unintended occurrences in the plan. Consider checking for a more specific pattern like ">($7, 1000)" or "SAL > 1000" to ensure the user's WHERE clause filter is actually present in the expected location. ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLDedupTest.java [380-386] String rawExplain = raw.explain(); Assert.assertTrue( "Raw plan should contain the bucket-non-null filter:\n" + rawExplain, rawExplain.contains("IS NOT NULL")); Assert.assertTrue( "Raw plan should contain the user where filter:\n" + rawExplain, - rawExplain.contains("1000"));+ rawExplain.contains(">") && rawExplain.contains("1000")); Suggestion importance[1-10]: 4 __ Why: The suggestion correctly identifies that checking for "1000" alone could match unintended occurrences. However, the improved code rawExplain.contains(">") && rawExplain.contains("1000") is still too generic and doesn't guarantee the filter is in the expected location. A more specific pattern would be better, but the suggestion does improve robustness slightly.	Low

[RyanL1997]

Hi @ryan-gh-bot , thanks for the change, left some reviews.

[github-actions]

Persistent review updated to latest commit 19f506d

[RyanL1997]

@ryan-gh-bot the rule-reordering fix works but treats a symptom rather than the root cause. The actual brittleness is in PlanUtils.mayBeFilterFromBucketNonNull — its allMatch(isNotNullOnRef) requirement means any future code path that produces a merged conjunction (a new rule, a different optimization order, a SQL-side caller) will silently re-disable dedup pushdown the same way. The HEP ordering is now a load-bearing invariant with nothing enforcing it; if anyone later adds a third rule to HEP_PROGRAM or refactors it, the bug comes back silently. The new testDedupAfterWhereWithFilterMergeFirstFailsToSimplify test pins the implementation strategy rather than the user-visible behavior.

Please revise to take option 1 from the issue instead:

1. Extend mayBeFilterFromBucketNonNull (core/.../utils/PlanUtils.java) to accept AND(IS_NOT_NULL(partition_col), <rest>) — i.e., recognize an AND whose operands contain an IS_NOT_NULL on the partition column rather than requiring every conjunct to be one.
2. In PPLSimplifyDedupRule.apply, split the non-IS_NOT_NULL conjuncts off and emit them as a separate filter below the new LogicalDedup, so the user predicate is preserved and pushdown still works.
3. Revert the addRuleCollection → addRuleInstance change in CalciteToolsHelper.HEP_PROGRAM — once the predicate is order-independent, rule order no longer matters.
4. Replace testDedupAfterWhereWithFilterMergeFirstFailsToSimplify (which pins buggy behavior) with a test that asserts the user-visible contract: with where upstream, dedup still produces a LogicalDedup regardless of rule order. Keep testDedupAfterWhereProducesLogicalDedup.

Please sanity-check that the loosened operand can't accidentally match user-written IS_NOT_NULL(x) AND ... filters outside the dedup pattern — the four-level operand chain (project → filter with row_number <= N → project containing ROW_NUMBER-dedup → filter) should be specific enough, but worth confirming.

[ryan-gh-bot]

Working on /@triage (invoked by @RyanL1997) — posting result when done.

[ryan-gh-bot]

Done — pushed 0bf8f7c73.

[github-actions]

Persistent review updated to latest commit 0bf8f7c

[dai-chen]

@ryan-gh-bot the rule-reordering fix works but treats a symptom rather than the root cause. The actual brittleness is in PlanUtils.mayBeFilterFromBucketNonNull — its allMatch(isNotNullOnRef) requirement means any future code path that produces a merged conjunction (a new rule, a different optimization order, a SQL-side caller) will silently re-disable dedup pushdown the same way. The HEP ordering is now a load-bearing invariant with nothing enforcing it; if anyone later adds a third rule to HEP_PROGRAM or refactors it, the bug comes back silently. The new testDedupAfterWhereWithFilterMergeFirstFailsToSimplify test pins the implementation strategy rather than the user-visible behavior.

Please revise to take option 1 from the issue instead:

1. Extend mayBeFilterFromBucketNonNull (core/.../utils/PlanUtils.java) to accept AND(IS_NOT_NULL(partition_col), <rest>) — i.e., recognize an AND whose operands contain an IS_NOT_NULL on the partition column rather than requiring every conjunct to be one.
2. In PPLSimplifyDedupRule.apply, split the non-IS_NOT_NULL conjuncts off and emit them as a separate filter below the new LogicalDedup, so the user predicate is preserved and pushdown still works.
3. Revert the addRuleCollection → addRuleInstance change in CalciteToolsHelper.HEP_PROGRAM — once the predicate is order-independent, rule order no longer matters.
4. Replace testDedupAfterWhereWithFilterMergeFirstFailsToSimplify (which pins buggy behavior) with a test that asserts the user-visible contract: with where upstream, dedup still produces a LogicalDedup regardless of rule order. Keep testDedupAfterWhereProducesLogicalDedup.

Please sanity-check that the loosened operand can't accidentally match user-written IS_NOT_NULL(x) AND ... filters outside the dedup pattern — the four-level operand chain (project → filter with row_number <= N → project containing ROW_NUMBER-dedup → filter) should be specific enough, but worth confirming.

I'm weighing the pros and cons of these two options:

• Option 1 — make PPLSimplifyDedupRule run first: If this rule runs before FilterMergeRule, it matches the original unmerged pattern and fixes the problem directly. The concern is that some other rule could run ahead of it and reintroduce the mismatch.
• Option 2 - current PR approach: Does this cover all the plan shapes visitDedupe can produce, or does it only fix the plain where + dedup case? e.g. dedup N, multiple dedup fields, and the sort/collation cases. Or these cases are not supported by DSL query anyway?

[penghuo]

1. Add end-to-end test asserts dedup is actually pushed down when where precedes dedup.
2. Refactor PR description to match the implementation.

[github-actions]

Persistent review updated to latest commit ae86ece

[RyanL1997]

Hi @penghuo, thanks for the feedback:

Add end-to-end test asserts dedup is actually pushed down when where precedes dedup.

added CalciteExplainIT.testDedupAfterWherePushDown

Refactor PR description to match the implementation.

PR description refactored — now describes the actual implementation (loosen predicate at source, not reorder HEP rules) instead of the bot's earlier reorder approach.

[RyanL1997]

Walked the axes against buildDedupNotNull / buildDedupOrNull and reasoned about each. The fix lives in two places — the mayBeFilterFromBucketNonNull operand predicate, and the conjunct split inside PPLSimplifyDedupRule#apply. As long as the bottom filter of the rule's 4-level operand chain still includes an IS NOT NULL($pk) somewhere (alone or in an AND), the rule fires and apply cleanly separates the partition-key guards from the user predicate.

Axis	Effect on bucket-non-null filter shape	Covered?	Why
dedup N (N ≥ 2)	N lives in the upper _row_number_ <= N filter, not the bucket-non-null filter. Bottom-filter shape is identical regardless of N.	Yes	Predicate match path is unchanged; apply reads N from the upper filter as before.
Multiple dedup fields (dedup a, b)	Bucket-non-null becomes AND(IS NOT NULL(a), IS NOT NULL(b)). With a user where, FilterMergeRule produces AND(IS NOT NULL(a), IS NOT NULL(b), <user predicate>).	Yes	The anyMatch predicate matches (multiple IS NOT NULL conjuncts present). apply absorbs all IS NOT NULL-on-partition-key conjuncts into LogicalDedup and emits the user predicate as a separate filter below it.
Sort/collation (| sort x | dedup a / | where p | sort x | dedup a)	stripInputSort pulls the prior sort out and threads it into ROW_NUMBER's ORDER BY. Bucket-non-null filter shape is unchanged.	Yes	apply extracts collation from the window. The user where can sit below or above the sort; either way the bottom filter is the same bucket-non-null shape (possibly merged with the user predicate).
keepempty=true (with or without where)	buildDedupOrNull does not emit a bucket-non-null filter at all. The 4-level operand chain has no matching bottom filter.	No, by design	The rule's operand chain doesn't match, so the rule never fires. Documented in code: PPLSimplifyDedupRule.java:101 (Since we cannot push down dedup with keepEmpty=true, we don't simplify that pattern) and DedupPushdownRule.java:218 (Cannot push dedup operator if keepEmpty=true). Unchanged by this PR.

Net: this PR covers the keepempty=false cases (the default and only pushable shape) across all four where + dedup <variant> combinations — simple, dedup N, multi-field, and sort/collation. The keepempty=true case is an existing, intentional non-pushable shape, unchanged here.

[RyanL1997]

Checking on the CI failures

[github-actions]

Persistent review updated to latest commit 7725e0b

[github-actions]

Persistent review updated to latest commit f67458f

[github-actions]

Persistent review updated to latest commit a66ad5b

[github-actions]

Persistent review updated to latest commit 95e2aeb

[RyanL1997]

The current CI failure is unrelated.

The failure is:

 * What went wrong:
Execution failed for task ':doctest:startPrometheus'.
> java.net.SocketTimeoutException: Read timed out

[github-actions]

Persistent review updated to latest commit 38fc6c3

[ryan-gh-bot]

Description

PPL dedup is initially compiled into a ROW_NUMBER() OVER (PARTITION BY field) window pattern with an IS NOT NULL(field) bucket-non-null filter directly above the scan. PPLSimplifyDedupRule folds that pattern into a LogicalDedup which DedupPushdownRule then rewrites into a composite + top_hits aggregation against the shard.

The HEP program registers FilterMergeRule.DEFAULT and PPLSimplifyDedupRule.DEDUP_SIMPLIFY_RULE via addRuleCollection. When a user where precedes dedup, the user's filter sits adjacent to the bucket-non-null filter; FilterMergeRule can merge them into AND(IS NOT NULL(field), <user predicate>). The previous mayBeFilterFromBucketNonNull operand predicate required every conjunct of the AND to be IS NOT NULL($ref) (allMatch), so the merged shape failed the match. PPLSimplifyDedupRule never fired, no LogicalDedup was produced, and DedupPushdownRule had nothing to match — dedup fell through to the in-memory ROW_NUMBER window form on the coordinator, which exceeds timeouts on large indices.

Fix

Loosen the operand predicate at the source rather than reordering HEP rules.

• PlanUtils.mayBeFilterFromBucketNonNull now accepts the un-merged IS NOT NULL($ref) shape or any AND whose conjuncts contain at least oneIS NOT NULL($ref) (anyMatch instead of allMatch). The predicate is order-independent — the simplify rule fires whether or not FilterMergeRule ran first.
• PPLSimplifyDedupRule.apply splits the matched filter's conjuncts into (a) the IS NOT NULL on a partition key (absorbed by LogicalDedup) and (b) everything else (emitted as a separate filter below LogicalDedup), so any user predicate folded into the AND is preserved verbatim. A defensive bail-out: if no IS NOT NULL on a partition key is present in the matched filter, the rule returns without transforming.
• PlanUtils.isNotNullOnRef is promoted to public so PPLSimplifyDedupRule.isNotNullOnPartitionKey can reuse it.

Verification

• CalcitePPLDedupTest.testDedupAfterWhereProducesLogicalDedup runs the rule sequence (PPLSimplifyDedupRule, FilterMergeRule) on a where + dedup plan and asserts that LogicalDedup is produced regardless of rule order. The user predicate appears as a separate filter below LogicalDedup, ready for downstream pushdown:

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

• CalcitePPLDedupTest.testDedupAfterWhereProducesLogicalDedupRegardlessOfRuleOrder exercises the swapped order (FilterMergeRule first) and asserts the same result, pinning the user-visible contract rather than any rule-ordering invariant.

• CalciteExplainIT.testDedupAfterWherePushDown (new) — end-to-end against a live cluster: runs source=opensearch-sql_test_index_account | where age > 25 | dedup gender and asserts LogicalDedup is present in the explain output and EnumerableWindow is absent (i.e., dedup is actually pushed down and the in-memory window fallback the bug caused is gone).

• All existing CalcitePPLDedupTest cases continue to pass.

Related Issues

Resolves#5482

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.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 38fc6c3)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review Possible Issue When the filter is only partially pushable, the code bails out without transforming. However, the original dedup pattern (with the filter still present) remains in the plan. If the residual filter is later removed or merged by another rule pass, a subsequent optimizer iteration might re-match this same dedup pattern, potentially causing an infinite loop or redundant rule applications. The code should either mark the node as transformed or ensure the pattern cannot re-match. if (call.rels[1] instanceofLogicalFilterfilter) { // Push the filter into the scan, then synthesize an identity project so the standard// apply() can run on the resulting Dedup → Project → Scan shape. If the filter is only// partially pushable, pushDownFilter returns a Filter wrapping the residual condition over// the new scan; we can't strip a residual filter without breaking semantics, so bail.finalCalciteLogicalIndexScanscan = call.rel(2); if (!(scan.pushDownFilter(filter) instanceofCalciteLogicalIndexScannewScan)) { return; } RelBuilderrelBuilder = call.builder(); Possible Issue If dedupColumns contains a non-RexInputRef element (e.g., a literal or expression), partitionKeyIndices will not include it, but the dedup pattern may still be valid. The defensive check at line 148 will bail out even though a valid IS_NOT_NULL conjunct on an actual partition key might exist. This could silently skip legitimate dedup simplifications when partition keys include expressions. Set<Integer> partitionKeyIndices = newHashSet<>(); for (RexNodekey : dedupColumns) { if (keyinstanceofRexInputRefref) { partitionKeyIndices.add(ref.getIndex()); } } List<RexNode> bucketNonNullConjuncts = newArrayList<>(); List<RexNode> remainingConjuncts = newArrayList<>(); for (RexNodeconjunct : RelOptUtil.conjunctions(bucketNonNullFilter.getCondition())) { if (isNotNullOnPartitionKey(conjunct, partitionKeyIndices)) { bucketNonNullConjuncts.add(conjunct); } else { remainingConjuncts.add(conjunct); } } // Defensive: if no IS_NOT_NULL conjunct on a partition key is present, this filter is not// actually a bucket-non-null filter — bail out without transforming. The loose operand// predicate may have matched on an unrelated AND that happens to contain an IS_NOT_NULL on// some other ref.if (bucketNonNullConjuncts.isEmpty()) { return; }

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 38fc6c3

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Add type check before casting The code assumes call.rel(2) is always a CalciteLogicalIndexScan when call.rels[1] is a LogicalFilter, but this assumption isn't validated. If the operand structure changes or the rule matches unexpectedly, a ClassCastException could occur. Add a type check before casting. opensearch/src/main/java/org/opensearch/sql/opensearch/planner/rules/DedupPushdownRule.java [51-59] if (call.rels[1] instanceof LogicalFilter filter) { // Push the filter into the scan, then synthesize an identity project so the standard // apply() can run on the resulting Dedup → Project → Scan shape. If the filter is only // partially pushable, pushDownFilter returns a Filter wrapping the residual condition over // the new scan; we can't strip a residual filter without breaking semantics, so bail. - final CalciteLogicalIndexScan scan = call.rel(2);+ if (!(call.rel(2) instanceof CalciteLogicalIndexScan scan)) {+ return;+ } if (!(scan.pushDownFilter(filter) instanceof CalciteLogicalIndexScan newScan)) { return; } Suggestion importance[1-10]: 7 __ Why: Valid safety improvement. The operand structure is defined by the rule configuration, but adding an explicit type check before casting call.rel(2) to CalciteLogicalIndexScan prevents potential ClassCastException if the rule configuration changes or matches unexpectedly, improving robustness.	Medium
Possible issue	Validate no predicates are lost When no bucket-non-null conjuncts are found, the method returns early without transforming. However, the extracted remainingConjuncts are discarded, potentially losing user predicates. Consider logging a warning or validating that remainingConjuncts is also empty to ensure no predicates are silently dropped. core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.java [148-150] if (bucketNonNullConjuncts.isEmpty()) { + // Defensive: if no IS_NOT_NULL conjunct on a partition key is present, this filter is not+ // actually a bucket-non-null filter — bail out without transforming.+ if (!remainingConjuncts.isEmpty()) {+ // Log or handle unexpected case where user predicates exist but no bucket guards found+ throw new IllegalStateException("Expected bucket-non-null filter but found only user predicates");+ } return; } Suggestion importance[1-10]: 3 __ Why: The suggestion raises a valid concern about potentially discarding remainingConjuncts, but the current behavior is intentional: if no bucket-non-null conjuncts exist, the filter isn't a dedup pattern and the rule shouldn't transform. The remainingConjuncts remain in the original filter. Throwing an exception would be too aggressive for a defensive check.	Low

---

Previous suggestions

Suggestions up to commit 95e2aeb

Category	Suggestion	Impact
General	Handle partial filter pushdown explicitly The pattern match checks if pushDownFilter returns a CalciteLogicalIndexScan, but doesn't verify whether the filter was fully pushed down or if a residual filter remains. If pushDownFilter returns a LogicalFilter wrapping a scan (indicating partial pushdown), the cast fails silently and the method returns. Consider explicitly checking the return type and handling partial pushdown scenarios. opensearch/src/main/java/org/opensearch/sql/opensearch/planner/rules/DedupPushdownRule.java [57-59] -if (!(scan.pushDownFilter(filter) instanceof CalciteLogicalIndexScan newScan)) {+var pushResult = scan.pushDownFilter(filter);+if (!(pushResult instanceof CalciteLogicalIndexScan newScan)) {+ // Partial pushdown or unsupported filter - bail out return; } Suggestion importance[1-10]: 4 __ Why: The suggestion correctly identifies that the code checks for partial pushdown via the instanceof pattern match and bails out if a residual filter remains (as stated in the comment on lines 53-55). Extracting to a variable improves readability slightly, but the logic is already correct and the comment explains the behavior. The improvement is marginal.	Low
	Validate all conjuncts are classified The loop iterates over all conjunctions and classifies them into two lists. If the filter condition is complex or deeply nested, RelOptUtil.conjunctions may not flatten all levels of nested ANDs. Consider adding validation to ensure all conjuncts are properly classified, or document the assumption that only top-level ANDs are expected. core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.java [137-143] -for (RexNode conjunct : RelOptUtil.conjunctions(bucketNonNullFilter.getCondition())) {+List<RexNode> allConjuncts = RelOptUtil.conjunctions(bucketNonNullFilter.getCondition());+for (RexNode conjunct : allConjuncts) { if (isNotNullOnPartitionKey(conjunct, partitionKeyIndices)) { bucketNonNullConjuncts.add(conjunct); } else { remainingConjuncts.add(conjunct); } } +// Validate that all conjuncts were classified+assert bucketNonNullConjuncts.size() + remainingConjuncts.size() == allConjuncts.size(); Suggestion importance[1-10]: 2 __ Why: The suggestion adds an assertion to validate that all conjuncts are classified, but RelOptUtil.conjunctions is a standard Calcite utility that flattens top-level ANDs reliably. The assertion would always pass in normal operation and only catch programming errors. The concern about deeply nested ANDs is unfounded since conjunctions handles this. The added validation has minimal practical value.	Low
Possible issue	Validate remaining conjuncts before early return When no bucket-non-null conjuncts are found, the method returns early without transforming. However, the extracted remainingConjuncts list is not checked for emptiness. If remainingConjuncts is non-empty but bucketNonNullConjuncts is empty, user predicates could be silently dropped. Consider logging a warning or validating that remainingConjuncts is also empty before returning. core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.java [148-150] if (bucketNonNullConjuncts.isEmpty()) { + if (!remainingConjuncts.isEmpty()) {+ // Log or handle unexpected case: filter has user predicates but no bucket-non-null guards+ } return; } Suggestion importance[1-10]: 3 __ Why: The suggestion raises a valid edge case where remainingConjuncts could be non-empty while bucketNonNullConjuncts is empty. However, the early return is intentional per the defensive comment (lines 144-147), and silently dropping user predicates is unlikely since the operand predicate should prevent matching unrelated filters. The suggestion to log or validate is reasonable but has low impact.	Low

Suggestions up to commit a66ad5b

Category	Suggestion	Impact
General	Handle non-RexInputRef partition keys The code only handles RexInputRef instances when building partitionKeyIndices, silently ignoring other node types. If dedupColumns contains non-RexInputRef nodes (e.g., expressions), those partition keys won't be tracked, potentially causing incorrect classification of conjuncts. Consider handling or explicitly documenting this limitation. core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.java [130-134] for (RexNode key : dedupColumns) { if (key instanceof RexInputRef ref) { partitionKeyIndices.add(ref.getIndex()); + } else {+ // Log or handle non-RexInputRef partition keys if they're expected+ call.getPlanner().getContext().unwrap(org.slf4j.Logger.class)+ .warn("Unexpected partition key type: {}", key.getClass().getSimpleName()); } } Suggestion importance[1-10]: 4 __ Why: The suggestion correctly identifies that non-RexInputRef nodes in dedupColumns are silently ignored. However, based on the PR context and the dedup pattern, partition keys are expected to be simple column references (RexInputRef). Adding logging for unexpected types could help catch edge cases, but the current behavior appears intentional for the dedup use case. The suggestion provides moderate value for defensive programming.	Low
	Add logging for defensive check When no bucket-non-null conjuncts are found, the method returns early without transforming the plan. However, this leaves the original plan unchanged, which may cause downstream rules to fail or produce incorrect results. Consider logging a warning or throwing an exception to make this defensive check more explicit and aid debugging. core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.java [148-150] if (bucketNonNullConjuncts.isEmpty()) { + // Log or handle the case where the filter doesn't match the expected pattern+ call.getPlanner().getContext().unwrap(org.slf4j.Logger.class)+ .warn("Expected bucket-non-null filter pattern not found; skipping transformation"); return; } Suggestion importance[1-10]: 3 __ Why: While adding logging for the defensive check could aid debugging, the early return is already well-documented with a comment explaining why the transformation is skipped. The suggestion to add logging is a minor improvement that would help debugging but doesn't address a critical issue. The improved code also assumes a specific logging framework is available in the context, which may not be the case.	Low
	Log filter pushdown failures The pushDownFilter method may return null if the filter cannot be pushed down, but the code silently returns without any indication of why the transformation failed. This makes debugging difficult when pushdown unexpectedly fails. Consider logging the failure reason or the filter condition that couldn't be pushed. opensearch/src/main/java/org/opensearch/sql/opensearch/planner/rules/DedupPushdownRule.java [56-59] AbstractRelNode scanWithFilter = scan.pushDownFilter(filter); if (scanWithFilter == null) { + // Log when filter pushdown fails to aid debugging+ call.getPlanner().getContext().unwrap(org.slf4j.Logger.class)+ .debug("Filter pushdown failed for condition: {}", filter.getCondition()); return; } Suggestion importance[1-10]: 3 __ Why: Adding debug logging when pushDownFilter returns null could help with debugging, but this is a normal control flow path where the rule simply doesn't match. The null return indicates the filter cannot be pushed down, which is expected behavior in some cases. The suggestion provides marginal debugging value but doesn't fix any bug or improve correctness.	Low

Suggestions up to commit f67458f

Category	Suggestion	Impact
General	Add fallback for failed filter pushdown When pushDownFilter returns null, the method returns early without any transformation. This silently fails the optimization. Consider logging this case or falling back to the standard path to ensure the query still executes correctly, even if the filter cannot be pushed down. opensearch/src/main/java/org/opensearch/sql/opensearch/planner/rules/DedupPushdownRule.java [56-59] AbstractRelNode scanWithFilter = scan.pushDownFilter(filter); if (scanWithFilter == null) { + // Filter cannot be pushed down; fall back to standard dedup path+ // or log the failure for debugging return; } Suggestion importance[1-10]: 4 __ Why: The suggestion identifies a valid concern about the early return when pushDownFilter returns null. However, the current behavior is intentional - when filter pushdown fails, the rule simply doesn't match and other rules can handle the query. Adding a log for debugging could be helpful, but the suggestion to "fall back to standard dedup path" is vague since the early return already allows the standard path (without the filter optimization) to proceed via the else branch at line 70. The comment addition would provide minimal value.	Low
	Handle edge case for missing bucket guards When bucketNonNullConjuncts is empty, the method returns early without transforming. However, remainingConjuncts may contain valid user predicates that should still be processed. Consider logging a warning or handling the case where only non-partition-key predicates exist to avoid silently dropping valid filters. core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.java [148-150] if (bucketNonNullConjuncts.isEmpty()) { + // Log or handle case where filter doesn't match expected dedup pattern+ // but may contain valid user predicates return; } Suggestion importance[1-10]: 3 __ Why: The suggestion correctly identifies the early return when bucketNonNullConjuncts is empty, but the concern about "silently dropping valid filters" is misplaced. The code comment at lines 144-147 explicitly states this is a defensive check to bail out when the filter doesn't match the expected bucket-non-null pattern. The remainingConjuncts are only relevant when there are actual bucket guards present. Adding a log might be marginally useful for debugging, but doesn't address a real correctness issue.	Low

Suggestions up to commit 0bf8f7c

Category	Suggestion	Impact
General	Remove redundant operand check The !rexCall.getOperands().isEmpty() check is redundant because IS_NOT_NULL always has exactly one operand by definition. Accessing getOperands().get(0) without this check is safe and simplifies the logic. core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.java [174-180] private static boolean isNotNullOnPartitionKey(RexNode rex, Set<Integer> partitionKeyIndices) { return rex instanceof RexCall rexCall && rexCall.isA(SqlKind.IS_NOT_NULL) - && !rexCall.getOperands().isEmpty() && rexCall.getOperands().get(0) instanceof RexInputRef ref && partitionKeyIndices.contains(ref.getIndex()); } Suggestion importance[1-10]: 4 __ Why: While IS_NOT_NULL typically has exactly one operand, the check !rexCall.getOperands().isEmpty() provides defensive programming against malformed expressions. Removing it offers minimal benefit and slightly reduces code robustness. The same pattern is used consistently in PlanUtils.isNotNullOnRef at line 826, suggesting it's an intentional defensive practice.	Low
	Handle non-reference partition keys Handle non-RexInputRef partition keys gracefully. If dedupColumns contains complex expressions that aren't simple column references, they won't be added to partitionKeyIndices, potentially causing valid bucket-non-null filters to be incorrectly rejected. Consider logging a warning or adding a comment explaining this limitation. core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.java [130-134] for (RexNode key : dedupColumns) { if (key instanceof RexInputRef ref) { partitionKeyIndices.add(ref.getIndex()); } + // Note: Complex expressions in dedupColumns are not tracked as partition keys.+ // This may cause the rule to bail out if bucket-non-null filters reference them. } Suggestion importance[1-10]: 3 __ Why: The suggestion correctly identifies that non-RexInputRef partition keys won't be tracked, but this appears to be intentional design rather than a bug. Adding a comment could improve code clarity, but the impact is minimal since the defensive check at line 148 already handles this case by bailing out when no matching filters are found.	Low

Suggestions up to commit 19f506d

Category	Suggestion	Impact
General	Make filter assertion more specific The assertion checking for "1000" is too generic and may match unintended occurrences in the plan. Consider checking for a more specific pattern like ">($7, 1000)" or "SAL > 1000" to ensure the user's WHERE clause filter is actually present in the expected location. ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLDedupTest.java [380-386] String rawExplain = raw.explain(); Assert.assertTrue( "Raw plan should contain the bucket-non-null filter:\n" + rawExplain, rawExplain.contains("IS NOT NULL")); Assert.assertTrue( "Raw plan should contain the user where filter:\n" + rawExplain, - rawExplain.contains("1000"));+ rawExplain.contains(">") && rawExplain.contains("1000")); Suggestion importance[1-10]: 4 __ Why: The suggestion correctly identifies that checking for "1000" alone could match unintended occurrences. However, the improved code rawExplain.contains(">") && rawExplain.contains("1000") is still too generic and doesn't guarantee the filter is in the expected location. A more specific pattern would be better, but the suggestion does improve robustness slightly.	Low

[RyanL1997]

Hi @ryan-gh-bot , thanks for the change, left some reviews.

[github-actions]

Persistent review updated to latest commit 19f506d

[RyanL1997]

@ryan-gh-bot the rule-reordering fix works but treats a symptom rather than the root cause. The actual brittleness is in PlanUtils.mayBeFilterFromBucketNonNull — its allMatch(isNotNullOnRef) requirement means any future code path that produces a merged conjunction (a new rule, a different optimization order, a SQL-side caller) will silently re-disable dedup pushdown the same way. The HEP ordering is now a load-bearing invariant with nothing enforcing it; if anyone later adds a third rule to HEP_PROGRAM or refactors it, the bug comes back silently. The new testDedupAfterWhereWithFilterMergeFirstFailsToSimplify test pins the implementation strategy rather than the user-visible behavior.

Please revise to take option 1 from the issue instead:

1. Extend mayBeFilterFromBucketNonNull (core/.../utils/PlanUtils.java) to accept AND(IS_NOT_NULL(partition_col), <rest>) — i.e., recognize an AND whose operands contain an IS_NOT_NULL on the partition column rather than requiring every conjunct to be one.
2. In PPLSimplifyDedupRule.apply, split the non-IS_NOT_NULL conjuncts off and emit them as a separate filter below the new LogicalDedup, so the user predicate is preserved and pushdown still works.
3. Revert the addRuleCollection → addRuleInstance change in CalciteToolsHelper.HEP_PROGRAM — once the predicate is order-independent, rule order no longer matters.
4. Replace testDedupAfterWhereWithFilterMergeFirstFailsToSimplify (which pins buggy behavior) with a test that asserts the user-visible contract: with where upstream, dedup still produces a LogicalDedup regardless of rule order. Keep testDedupAfterWhereProducesLogicalDedup.

Please sanity-check that the loosened operand can't accidentally match user-written IS_NOT_NULL(x) AND ... filters outside the dedup pattern — the four-level operand chain (project → filter with row_number <= N → project containing ROW_NUMBER-dedup → filter) should be specific enough, but worth confirming.

[ryan-gh-bot]

Working on /@triage (invoked by @RyanL1997) — posting result when done.

[ryan-gh-bot]

Done — pushed 0bf8f7c73.

[github-actions]

Persistent review updated to latest commit 0bf8f7c

[dai-chen]

@ryan-gh-bot the rule-reordering fix works but treats a symptom rather than the root cause. The actual brittleness is in PlanUtils.mayBeFilterFromBucketNonNull — its allMatch(isNotNullOnRef) requirement means any future code path that produces a merged conjunction (a new rule, a different optimization order, a SQL-side caller) will silently re-disable dedup pushdown the same way. The HEP ordering is now a load-bearing invariant with nothing enforcing it; if anyone later adds a third rule to HEP_PROGRAM or refactors it, the bug comes back silently. The new testDedupAfterWhereWithFilterMergeFirstFailsToSimplify test pins the implementation strategy rather than the user-visible behavior.

Please revise to take option 1 from the issue instead:

1. Extend mayBeFilterFromBucketNonNull (core/.../utils/PlanUtils.java) to accept AND(IS_NOT_NULL(partition_col), <rest>) — i.e., recognize an AND whose operands contain an IS_NOT_NULL on the partition column rather than requiring every conjunct to be one.
2. In PPLSimplifyDedupRule.apply, split the non-IS_NOT_NULL conjuncts off and emit them as a separate filter below the new LogicalDedup, so the user predicate is preserved and pushdown still works.
3. Revert the addRuleCollection → addRuleInstance change in CalciteToolsHelper.HEP_PROGRAM — once the predicate is order-independent, rule order no longer matters.
4. Replace testDedupAfterWhereWithFilterMergeFirstFailsToSimplify (which pins buggy behavior) with a test that asserts the user-visible contract: with where upstream, dedup still produces a LogicalDedup regardless of rule order. Keep testDedupAfterWhereProducesLogicalDedup.

Please sanity-check that the loosened operand can't accidentally match user-written IS_NOT_NULL(x) AND ... filters outside the dedup pattern — the four-level operand chain (project → filter with row_number <= N → project containing ROW_NUMBER-dedup → filter) should be specific enough, but worth confirming.

I'm weighing the pros and cons of these two options:

• Option 1 — make PPLSimplifyDedupRule run first: If this rule runs before FilterMergeRule, it matches the original unmerged pattern and fixes the problem directly. The concern is that some other rule could run ahead of it and reintroduce the mismatch.
• Option 2 - current PR approach: Does this cover all the plan shapes visitDedupe can produce, or does it only fix the plain where + dedup case? e.g. dedup N, multiple dedup fields, and the sort/collation cases. Or these cases are not supported by DSL query anyway?

[penghuo]

1. Add end-to-end test asserts dedup is actually pushed down when where precedes dedup.
2. Refactor PR description to match the implementation.

[github-actions]

Persistent review updated to latest commit ae86ece

[RyanL1997]

Hi @penghuo, thanks for the feedback:

Add end-to-end test asserts dedup is actually pushed down when where precedes dedup.

added CalciteExplainIT.testDedupAfterWherePushDown

Refactor PR description to match the implementation.

PR description refactored — now describes the actual implementation (loosen predicate at source, not reorder HEP rules) instead of the bot's earlier reorder approach.

[RyanL1997]

Walked the axes against buildDedupNotNull / buildDedupOrNull and reasoned about each. The fix lives in two places — the mayBeFilterFromBucketNonNull operand predicate, and the conjunct split inside PPLSimplifyDedupRule#apply. As long as the bottom filter of the rule's 4-level operand chain still includes an IS NOT NULL($pk) somewhere (alone or in an AND), the rule fires and apply cleanly separates the partition-key guards from the user predicate.

Axis	Effect on bucket-non-null filter shape	Covered?	Why
dedup N (N ≥ 2)	N lives in the upper _row_number_ <= N filter, not the bucket-non-null filter. Bottom-filter shape is identical regardless of N.	Yes	Predicate match path is unchanged; apply reads N from the upper filter as before.
Multiple dedup fields (dedup a, b)	Bucket-non-null becomes AND(IS NOT NULL(a), IS NOT NULL(b)). With a user where, FilterMergeRule produces AND(IS NOT NULL(a), IS NOT NULL(b), <user predicate>).	Yes	The anyMatch predicate matches (multiple IS NOT NULL conjuncts present). apply absorbs all IS NOT NULL-on-partition-key conjuncts into LogicalDedup and emits the user predicate as a separate filter below it.
Sort/collation (| sort x | dedup a / | where p | sort x | dedup a)	stripInputSort pulls the prior sort out and threads it into ROW_NUMBER's ORDER BY. Bucket-non-null filter shape is unchanged.	Yes	apply extracts collation from the window. The user where can sit below or above the sort; either way the bottom filter is the same bucket-non-null shape (possibly merged with the user predicate).
keepempty=true (with or without where)	buildDedupOrNull does not emit a bucket-non-null filter at all. The 4-level operand chain has no matching bottom filter.	No, by design	The rule's operand chain doesn't match, so the rule never fires. Documented in code: PPLSimplifyDedupRule.java:101 (Since we cannot push down dedup with keepEmpty=true, we don't simplify that pattern) and DedupPushdownRule.java:218 (Cannot push dedup operator if keepEmpty=true). Unchanged by this PR.

Net: this PR covers the keepempty=false cases (the default and only pushable shape) across all four where + dedup <variant> combinations — simple, dedup N, multi-field, and sort/collation. The keepempty=true case is an existing, intentional non-pushable shape, unchanged here.

[RyanL1997]

Checking on the CI failures

[github-actions]

Persistent review updated to latest commit 7725e0b

[github-actions]

Persistent review updated to latest commit f67458f

[github-actions]

Persistent review updated to latest commit a66ad5b

[github-actions]

Persistent review updated to latest commit 95e2aeb

[RyanL1997]

The current CI failure is unrelated.

The failure is:

 * What went wrong:
Execution failed for task ':doctest:startPrometheus'.
> java.net.SocketTimeoutException: Read timed out

[github-actions]

Persistent review updated to latest commit 38fc6c3

[ryan-gh-bot]

Description

PPL dedup is initially compiled into a ROW_NUMBER() OVER (PARTITION BY field) window pattern with an IS NOT NULL(field) bucket-non-null filter directly above the scan. PPLSimplifyDedupRule folds that pattern into a LogicalDedup which DedupPushdownRule then rewrites into a composite + top_hits aggregation against the shard.

The HEP program registers FilterMergeRule.DEFAULT and PPLSimplifyDedupRule.DEDUP_SIMPLIFY_RULE via addRuleCollection. When a user where precedes dedup, the user's filter sits adjacent to the bucket-non-null filter; FilterMergeRule can merge them into AND(IS NOT NULL(field), <user predicate>). The previous mayBeFilterFromBucketNonNull operand predicate required every conjunct of the AND to be IS NOT NULL($ref) (allMatch), so the merged shape failed the match. PPLSimplifyDedupRule never fired, no LogicalDedup was produced, and DedupPushdownRule had nothing to match — dedup fell through to the in-memory ROW_NUMBER window form on the coordinator, which exceeds timeouts on large indices.

Fix

Loosen the operand predicate at the source rather than reordering HEP rules.

• PlanUtils.mayBeFilterFromBucketNonNull now accepts the un-merged IS NOT NULL($ref) shape or any AND whose conjuncts contain at least oneIS NOT NULL($ref) (anyMatch instead of allMatch). The predicate is order-independent — the simplify rule fires whether or not FilterMergeRule ran first.
• PPLSimplifyDedupRule.apply splits the matched filter's conjuncts into (a) the IS NOT NULL on a partition key (absorbed by LogicalDedup) and (b) everything else (emitted as a separate filter below LogicalDedup), so any user predicate folded into the AND is preserved verbatim. A defensive bail-out: if no IS NOT NULL on a partition key is present in the matched filter, the rule returns without transforming.
• PlanUtils.isNotNullOnRef is promoted to public so PPLSimplifyDedupRule.isNotNullOnPartitionKey can reuse it.

Verification

• CalcitePPLDedupTest.testDedupAfterWhereProducesLogicalDedup runs the rule sequence (PPLSimplifyDedupRule, FilterMergeRule) on a where + dedup plan and asserts that LogicalDedup is produced regardless of rule order. The user predicate appears as a separate filter below LogicalDedup, ready for downstream pushdown:

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

• CalcitePPLDedupTest.testDedupAfterWhereProducesLogicalDedupRegardlessOfRuleOrder exercises the swapped order (FilterMergeRule first) and asserts the same result, pinning the user-visible contract rather than any rule-ordering invariant.

• CalciteExplainIT.testDedupAfterWherePushDown (new) — end-to-end against a live cluster: runs source=opensearch-sql_test_index_account | where age > 25 | dedup gender and asserts LogicalDedup is present in the explain output and EnumerableWindow is absent (i.e., dedup is actually pushed down and the in-memory window fallback the bug caused is gone).

• All existing CalcitePPLDedupTest cases continue to pass.

Related Issues

Resolves#5482

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.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 38fc6c3)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review Possible Issue When the filter is only partially pushable, the code bails out without transforming. However, the original dedup pattern (with the filter still present) remains in the plan. If the residual filter is later removed or merged by another rule pass, a subsequent optimizer iteration might re-match this same dedup pattern, potentially causing an infinite loop or redundant rule applications. The code should either mark the node as transformed or ensure the pattern cannot re-match. if (call.rels[1] instanceofLogicalFilterfilter) { // Push the filter into the scan, then synthesize an identity project so the standard// apply() can run on the resulting Dedup → Project → Scan shape. If the filter is only// partially pushable, pushDownFilter returns a Filter wrapping the residual condition over// the new scan; we can't strip a residual filter without breaking semantics, so bail.finalCalciteLogicalIndexScanscan = call.rel(2); if (!(scan.pushDownFilter(filter) instanceofCalciteLogicalIndexScannewScan)) { return; } RelBuilderrelBuilder = call.builder(); Possible Issue If dedupColumns contains a non-RexInputRef element (e.g., a literal or expression), partitionKeyIndices will not include it, but the dedup pattern may still be valid. The defensive check at line 148 will bail out even though a valid IS_NOT_NULL conjunct on an actual partition key might exist. This could silently skip legitimate dedup simplifications when partition keys include expressions. Set<Integer> partitionKeyIndices = newHashSet<>(); for (RexNodekey : dedupColumns) { if (keyinstanceofRexInputRefref) { partitionKeyIndices.add(ref.getIndex()); } } List<RexNode> bucketNonNullConjuncts = newArrayList<>(); List<RexNode> remainingConjuncts = newArrayList<>(); for (RexNodeconjunct : RelOptUtil.conjunctions(bucketNonNullFilter.getCondition())) { if (isNotNullOnPartitionKey(conjunct, partitionKeyIndices)) { bucketNonNullConjuncts.add(conjunct); } else { remainingConjuncts.add(conjunct); } } // Defensive: if no IS_NOT_NULL conjunct on a partition key is present, this filter is not// actually a bucket-non-null filter — bail out without transforming. The loose operand// predicate may have matched on an unrelated AND that happens to contain an IS_NOT_NULL on// some other ref.if (bucketNonNullConjuncts.isEmpty()) { return; }

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 38fc6c3

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Add type check before casting The code assumes call.rel(2) is always a CalciteLogicalIndexScan when call.rels[1] is a LogicalFilter, but this assumption isn't validated. If the operand structure changes or the rule matches unexpectedly, a ClassCastException could occur. Add a type check before casting. opensearch/src/main/java/org/opensearch/sql/opensearch/planner/rules/DedupPushdownRule.java [51-59] if (call.rels[1] instanceof LogicalFilter filter) { // Push the filter into the scan, then synthesize an identity project so the standard // apply() can run on the resulting Dedup → Project → Scan shape. If the filter is only // partially pushable, pushDownFilter returns a Filter wrapping the residual condition over // the new scan; we can't strip a residual filter without breaking semantics, so bail. - final CalciteLogicalIndexScan scan = call.rel(2);+ if (!(call.rel(2) instanceof CalciteLogicalIndexScan scan)) {+ return;+ } if (!(scan.pushDownFilter(filter) instanceof CalciteLogicalIndexScan newScan)) { return; } Suggestion importance[1-10]: 7 __ Why: Valid safety improvement. The operand structure is defined by the rule configuration, but adding an explicit type check before casting call.rel(2) to CalciteLogicalIndexScan prevents potential ClassCastException if the rule configuration changes or matches unexpectedly, improving robustness.	Medium
Possible issue	Validate no predicates are lost When no bucket-non-null conjuncts are found, the method returns early without transforming. However, the extracted remainingConjuncts are discarded, potentially losing user predicates. Consider logging a warning or validating that remainingConjuncts is also empty to ensure no predicates are silently dropped. core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.java [148-150] if (bucketNonNullConjuncts.isEmpty()) { + // Defensive: if no IS_NOT_NULL conjunct on a partition key is present, this filter is not+ // actually a bucket-non-null filter — bail out without transforming.+ if (!remainingConjuncts.isEmpty()) {+ // Log or handle unexpected case where user predicates exist but no bucket guards found+ throw new IllegalStateException("Expected bucket-non-null filter but found only user predicates");+ } return; } Suggestion importance[1-10]: 3 __ Why: The suggestion raises a valid concern about potentially discarding remainingConjuncts, but the current behavior is intentional: if no bucket-non-null conjuncts exist, the filter isn't a dedup pattern and the rule shouldn't transform. The remainingConjuncts remain in the original filter. Throwing an exception would be too aggressive for a defensive check.	Low

---

Previous suggestions

Suggestions up to commit 95e2aeb

Category	Suggestion	Impact
General	Handle partial filter pushdown explicitly The pattern match checks if pushDownFilter returns a CalciteLogicalIndexScan, but doesn't verify whether the filter was fully pushed down or if a residual filter remains. If pushDownFilter returns a LogicalFilter wrapping a scan (indicating partial pushdown), the cast fails silently and the method returns. Consider explicitly checking the return type and handling partial pushdown scenarios. opensearch/src/main/java/org/opensearch/sql/opensearch/planner/rules/DedupPushdownRule.java [57-59] -if (!(scan.pushDownFilter(filter) instanceof CalciteLogicalIndexScan newScan)) {+var pushResult = scan.pushDownFilter(filter);+if (!(pushResult instanceof CalciteLogicalIndexScan newScan)) {+ // Partial pushdown or unsupported filter - bail out return; } Suggestion importance[1-10]: 4 __ Why: The suggestion correctly identifies that the code checks for partial pushdown via the instanceof pattern match and bails out if a residual filter remains (as stated in the comment on lines 53-55). Extracting to a variable improves readability slightly, but the logic is already correct and the comment explains the behavior. The improvement is marginal.	Low
	Validate all conjuncts are classified The loop iterates over all conjunctions and classifies them into two lists. If the filter condition is complex or deeply nested, RelOptUtil.conjunctions may not flatten all levels of nested ANDs. Consider adding validation to ensure all conjuncts are properly classified, or document the assumption that only top-level ANDs are expected. core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.java [137-143] -for (RexNode conjunct : RelOptUtil.conjunctions(bucketNonNullFilter.getCondition())) {+List<RexNode> allConjuncts = RelOptUtil.conjunctions(bucketNonNullFilter.getCondition());+for (RexNode conjunct : allConjuncts) { if (isNotNullOnPartitionKey(conjunct, partitionKeyIndices)) { bucketNonNullConjuncts.add(conjunct); } else { remainingConjuncts.add(conjunct); } } +// Validate that all conjuncts were classified+assert bucketNonNullConjuncts.size() + remainingConjuncts.size() == allConjuncts.size(); Suggestion importance[1-10]: 2 __ Why: The suggestion adds an assertion to validate that all conjuncts are classified, but RelOptUtil.conjunctions is a standard Calcite utility that flattens top-level ANDs reliably. The assertion would always pass in normal operation and only catch programming errors. The concern about deeply nested ANDs is unfounded since conjunctions handles this. The added validation has minimal practical value.	Low
Possible issue	Validate remaining conjuncts before early return When no bucket-non-null conjuncts are found, the method returns early without transforming. However, the extracted remainingConjuncts list is not checked for emptiness. If remainingConjuncts is non-empty but bucketNonNullConjuncts is empty, user predicates could be silently dropped. Consider logging a warning or validating that remainingConjuncts is also empty before returning. core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.java [148-150] if (bucketNonNullConjuncts.isEmpty()) { + if (!remainingConjuncts.isEmpty()) {+ // Log or handle unexpected case: filter has user predicates but no bucket-non-null guards+ } return; } Suggestion importance[1-10]: 3 __ Why: The suggestion raises a valid edge case where remainingConjuncts could be non-empty while bucketNonNullConjuncts is empty. However, the early return is intentional per the defensive comment (lines 144-147), and silently dropping user predicates is unlikely since the operand predicate should prevent matching unrelated filters. The suggestion to log or validate is reasonable but has low impact.	Low

Suggestions up to commit a66ad5b

Category	Suggestion	Impact
General	Handle non-RexInputRef partition keys The code only handles RexInputRef instances when building partitionKeyIndices, silently ignoring other node types. If dedupColumns contains non-RexInputRef nodes (e.g., expressions), those partition keys won't be tracked, potentially causing incorrect classification of conjuncts. Consider handling or explicitly documenting this limitation. core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.java [130-134] for (RexNode key : dedupColumns) { if (key instanceof RexInputRef ref) { partitionKeyIndices.add(ref.getIndex()); + } else {+ // Log or handle non-RexInputRef partition keys if they're expected+ call.getPlanner().getContext().unwrap(org.slf4j.Logger.class)+ .warn("Unexpected partition key type: {}", key.getClass().getSimpleName()); } } Suggestion importance[1-10]: 4 __ Why: The suggestion correctly identifies that non-RexInputRef nodes in dedupColumns are silently ignored. However, based on the PR context and the dedup pattern, partition keys are expected to be simple column references (RexInputRef). Adding logging for unexpected types could help catch edge cases, but the current behavior appears intentional for the dedup use case. The suggestion provides moderate value for defensive programming.	Low
	Add logging for defensive check When no bucket-non-null conjuncts are found, the method returns early without transforming the plan. However, this leaves the original plan unchanged, which may cause downstream rules to fail or produce incorrect results. Consider logging a warning or throwing an exception to make this defensive check more explicit and aid debugging. core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.java [148-150] if (bucketNonNullConjuncts.isEmpty()) { + // Log or handle the case where the filter doesn't match the expected pattern+ call.getPlanner().getContext().unwrap(org.slf4j.Logger.class)+ .warn("Expected bucket-non-null filter pattern not found; skipping transformation"); return; } Suggestion importance[1-10]: 3 __ Why: While adding logging for the defensive check could aid debugging, the early return is already well-documented with a comment explaining why the transformation is skipped. The suggestion to add logging is a minor improvement that would help debugging but doesn't address a critical issue. The improved code also assumes a specific logging framework is available in the context, which may not be the case.	Low
	Log filter pushdown failures The pushDownFilter method may return null if the filter cannot be pushed down, but the code silently returns without any indication of why the transformation failed. This makes debugging difficult when pushdown unexpectedly fails. Consider logging the failure reason or the filter condition that couldn't be pushed. opensearch/src/main/java/org/opensearch/sql/opensearch/planner/rules/DedupPushdownRule.java [56-59] AbstractRelNode scanWithFilter = scan.pushDownFilter(filter); if (scanWithFilter == null) { + // Log when filter pushdown fails to aid debugging+ call.getPlanner().getContext().unwrap(org.slf4j.Logger.class)+ .debug("Filter pushdown failed for condition: {}", filter.getCondition()); return; } Suggestion importance[1-10]: 3 __ Why: Adding debug logging when pushDownFilter returns null could help with debugging, but this is a normal control flow path where the rule simply doesn't match. The null return indicates the filter cannot be pushed down, which is expected behavior in some cases. The suggestion provides marginal debugging value but doesn't fix any bug or improve correctness.	Low

Suggestions up to commit f67458f

Category	Suggestion	Impact
General	Add fallback for failed filter pushdown When pushDownFilter returns null, the method returns early without any transformation. This silently fails the optimization. Consider logging this case or falling back to the standard path to ensure the query still executes correctly, even if the filter cannot be pushed down. opensearch/src/main/java/org/opensearch/sql/opensearch/planner/rules/DedupPushdownRule.java [56-59] AbstractRelNode scanWithFilter = scan.pushDownFilter(filter); if (scanWithFilter == null) { + // Filter cannot be pushed down; fall back to standard dedup path+ // or log the failure for debugging return; } Suggestion importance[1-10]: 4 __ Why: The suggestion identifies a valid concern about the early return when pushDownFilter returns null. However, the current behavior is intentional - when filter pushdown fails, the rule simply doesn't match and other rules can handle the query. Adding a log for debugging could be helpful, but the suggestion to "fall back to standard dedup path" is vague since the early return already allows the standard path (without the filter optimization) to proceed via the else branch at line 70. The comment addition would provide minimal value.	Low
	Handle edge case for missing bucket guards When bucketNonNullConjuncts is empty, the method returns early without transforming. However, remainingConjuncts may contain valid user predicates that should still be processed. Consider logging a warning or handling the case where only non-partition-key predicates exist to avoid silently dropping valid filters. core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.java [148-150] if (bucketNonNullConjuncts.isEmpty()) { + // Log or handle case where filter doesn't match expected dedup pattern+ // but may contain valid user predicates return; } Suggestion importance[1-10]: 3 __ Why: The suggestion correctly identifies the early return when bucketNonNullConjuncts is empty, but the concern about "silently dropping valid filters" is misplaced. The code comment at lines 144-147 explicitly states this is a defensive check to bail out when the filter doesn't match the expected bucket-non-null pattern. The remainingConjuncts are only relevant when there are actual bucket guards present. Adding a log might be marginally useful for debugging, but doesn't address a real correctness issue.	Low

Suggestions up to commit 0bf8f7c

Category	Suggestion	Impact
General	Remove redundant operand check The !rexCall.getOperands().isEmpty() check is redundant because IS_NOT_NULL always has exactly one operand by definition. Accessing getOperands().get(0) without this check is safe and simplifies the logic. core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.java [174-180] private static boolean isNotNullOnPartitionKey(RexNode rex, Set<Integer> partitionKeyIndices) { return rex instanceof RexCall rexCall && rexCall.isA(SqlKind.IS_NOT_NULL) - && !rexCall.getOperands().isEmpty() && rexCall.getOperands().get(0) instanceof RexInputRef ref && partitionKeyIndices.contains(ref.getIndex()); } Suggestion importance[1-10]: 4 __ Why: While IS_NOT_NULL typically has exactly one operand, the check !rexCall.getOperands().isEmpty() provides defensive programming against malformed expressions. Removing it offers minimal benefit and slightly reduces code robustness. The same pattern is used consistently in PlanUtils.isNotNullOnRef at line 826, suggesting it's an intentional defensive practice.	Low
	Handle non-reference partition keys Handle non-RexInputRef partition keys gracefully. If dedupColumns contains complex expressions that aren't simple column references, they won't be added to partitionKeyIndices, potentially causing valid bucket-non-null filters to be incorrectly rejected. Consider logging a warning or adding a comment explaining this limitation. core/src/main/java/org/opensearch/sql/calcite/plan/rule/PPLSimplifyDedupRule.java [130-134] for (RexNode key : dedupColumns) { if (key instanceof RexInputRef ref) { partitionKeyIndices.add(ref.getIndex()); } + // Note: Complex expressions in dedupColumns are not tracked as partition keys.+ // This may cause the rule to bail out if bucket-non-null filters reference them. } Suggestion importance[1-10]: 3 __ Why: The suggestion correctly identifies that non-RexInputRef partition keys won't be tracked, but this appears to be intentional design rather than a bug. Adding a comment could improve code clarity, but the impact is minimal since the defensive check at line 148 already handles this case by bailing out when no matching filters are found.	Low

Suggestions up to commit 19f506d

Category	Suggestion	Impact
General	Make filter assertion more specific The assertion checking for "1000" is too generic and may match unintended occurrences in the plan. Consider checking for a more specific pattern like ">($7, 1000)" or "SAL > 1000" to ensure the user's WHERE clause filter is actually present in the expected location. ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLDedupTest.java [380-386] String rawExplain = raw.explain(); Assert.assertTrue( "Raw plan should contain the bucket-non-null filter:\n" + rawExplain, rawExplain.contains("IS NOT NULL")); Assert.assertTrue( "Raw plan should contain the user where filter:\n" + rawExplain, - rawExplain.contains("1000"));+ rawExplain.contains(">") && rawExplain.contains("1000")); Suggestion importance[1-10]: 4 __ Why: The suggestion correctly identifies that checking for "1000" alone could match unintended occurrences. However, the improved code rawExplain.contains(">") && rawExplain.contains("1000") is still too generic and doesn't guarantee the filter is in the expected location. A more specific pattern would be better, but the suggestion does improve robustness slightly.	Low

[RyanL1997]

Hi @ryan-gh-bot , thanks for the change, left some reviews.

[github-actions]

Persistent review updated to latest commit 19f506d

[RyanL1997]

@ryan-gh-bot the rule-reordering fix works but treats a symptom rather than the root cause. The actual brittleness is in PlanUtils.mayBeFilterFromBucketNonNull — its allMatch(isNotNullOnRef) requirement means any future code path that produces a merged conjunction (a new rule, a different optimization order, a SQL-side caller) will silently re-disable dedup pushdown the same way. The HEP ordering is now a load-bearing invariant with nothing enforcing it; if anyone later adds a third rule to HEP_PROGRAM or refactors it, the bug comes back silently. The new testDedupAfterWhereWithFilterMergeFirstFailsToSimplify test pins the implementation strategy rather than the user-visible behavior.

Please revise to take option 1 from the issue instead:

1. Extend mayBeFilterFromBucketNonNull (core/.../utils/PlanUtils.java) to accept AND(IS_NOT_NULL(partition_col), <rest>) — i.e., recognize an AND whose operands contain an IS_NOT_NULL on the partition column rather than requiring every conjunct to be one.
2. In PPLSimplifyDedupRule.apply, split the non-IS_NOT_NULL conjuncts off and emit them as a separate filter below the new LogicalDedup, so the user predicate is preserved and pushdown still works.
3. Revert the addRuleCollection → addRuleInstance change in CalciteToolsHelper.HEP_PROGRAM — once the predicate is order-independent, rule order no longer matters.
4. Replace testDedupAfterWhereWithFilterMergeFirstFailsToSimplify (which pins buggy behavior) with a test that asserts the user-visible contract: with where upstream, dedup still produces a LogicalDedup regardless of rule order. Keep testDedupAfterWhereProducesLogicalDedup.

Please sanity-check that the loosened operand can't accidentally match user-written IS_NOT_NULL(x) AND ... filters outside the dedup pattern — the four-level operand chain (project → filter with row_number <= N → project containing ROW_NUMBER-dedup → filter) should be specific enough, but worth confirming.

[ryan-gh-bot]

Working on /@triage (invoked by @RyanL1997) — posting result when done.

[ryan-gh-bot]

Done — pushed 0bf8f7c73.

[github-actions]

Persistent review updated to latest commit 0bf8f7c

[dai-chen]

@ryan-gh-bot the rule-reordering fix works but treats a symptom rather than the root cause. The actual brittleness is in PlanUtils.mayBeFilterFromBucketNonNull — its allMatch(isNotNullOnRef) requirement means any future code path that produces a merged conjunction (a new rule, a different optimization order, a SQL-side caller) will silently re-disable dedup pushdown the same way. The HEP ordering is now a load-bearing invariant with nothing enforcing it; if anyone later adds a third rule to HEP_PROGRAM or refactors it, the bug comes back silently. The new testDedupAfterWhereWithFilterMergeFirstFailsToSimplify test pins the implementation strategy rather than the user-visible behavior.

Please revise to take option 1 from the issue instead:

1. Extend mayBeFilterFromBucketNonNull (core/.../utils/PlanUtils.java) to accept AND(IS_NOT_NULL(partition_col), <rest>) — i.e., recognize an AND whose operands contain an IS_NOT_NULL on the partition column rather than requiring every conjunct to be one.
2. In PPLSimplifyDedupRule.apply, split the non-IS_NOT_NULL conjuncts off and emit them as a separate filter below the new LogicalDedup, so the user predicate is preserved and pushdown still works.
3. Revert the addRuleCollection → addRuleInstance change in CalciteToolsHelper.HEP_PROGRAM — once the predicate is order-independent, rule order no longer matters.
4. Replace testDedupAfterWhereWithFilterMergeFirstFailsToSimplify (which pins buggy behavior) with a test that asserts the user-visible contract: with where upstream, dedup still produces a LogicalDedup regardless of rule order. Keep testDedupAfterWhereProducesLogicalDedup.

Please sanity-check that the loosened operand can't accidentally match user-written IS_NOT_NULL(x) AND ... filters outside the dedup pattern — the four-level operand chain (project → filter with row_number <= N → project containing ROW_NUMBER-dedup → filter) should be specific enough, but worth confirming.

I'm weighing the pros and cons of these two options:

• Option 1 — make PPLSimplifyDedupRule run first: If this rule runs before FilterMergeRule, it matches the original unmerged pattern and fixes the problem directly. The concern is that some other rule could run ahead of it and reintroduce the mismatch.
• Option 2 - current PR approach: Does this cover all the plan shapes visitDedupe can produce, or does it only fix the plain where + dedup case? e.g. dedup N, multiple dedup fields, and the sort/collation cases. Or these cases are not supported by DSL query anyway?

[penghuo]

1. Add end-to-end test asserts dedup is actually pushed down when where precedes dedup.
2. Refactor PR description to match the implementation.

[github-actions]

Persistent review updated to latest commit ae86ece

[RyanL1997]

Hi @penghuo, thanks for the feedback:

Add end-to-end test asserts dedup is actually pushed down when where precedes dedup.

added CalciteExplainIT.testDedupAfterWherePushDown

Refactor PR description to match the implementation.

PR description refactored — now describes the actual implementation (loosen predicate at source, not reorder HEP rules) instead of the bot's earlier reorder approach.

[RyanL1997]

Walked the axes against buildDedupNotNull / buildDedupOrNull and reasoned about each. The fix lives in two places — the mayBeFilterFromBucketNonNull operand predicate, and the conjunct split inside PPLSimplifyDedupRule#apply. As long as the bottom filter of the rule's 4-level operand chain still includes an IS NOT NULL($pk) somewhere (alone or in an AND), the rule fires and apply cleanly separates the partition-key guards from the user predicate.

Axis	Effect on bucket-non-null filter shape	Covered?	Why
dedup N (N ≥ 2)	N lives in the upper _row_number_ <= N filter, not the bucket-non-null filter. Bottom-filter shape is identical regardless of N.	Yes	Predicate match path is unchanged; apply reads N from the upper filter as before.
Multiple dedup fields (dedup a, b)	Bucket-non-null becomes AND(IS NOT NULL(a), IS NOT NULL(b)). With a user where, FilterMergeRule produces AND(IS NOT NULL(a), IS NOT NULL(b), <user predicate>).	Yes	The anyMatch predicate matches (multiple IS NOT NULL conjuncts present). apply absorbs all IS NOT NULL-on-partition-key conjuncts into LogicalDedup and emits the user predicate as a separate filter below it.
Sort/collation (| sort x | dedup a / | where p | sort x | dedup a)	stripInputSort pulls the prior sort out and threads it into ROW_NUMBER's ORDER BY. Bucket-non-null filter shape is unchanged.	Yes	apply extracts collation from the window. The user where can sit below or above the sort; either way the bottom filter is the same bucket-non-null shape (possibly merged with the user predicate).
keepempty=true (with or without where)	buildDedupOrNull does not emit a bucket-non-null filter at all. The 4-level operand chain has no matching bottom filter.	No, by design	The rule's operand chain doesn't match, so the rule never fires. Documented in code: PPLSimplifyDedupRule.java:101 (Since we cannot push down dedup with keepEmpty=true, we don't simplify that pattern) and DedupPushdownRule.java:218 (Cannot push dedup operator if keepEmpty=true). Unchanged by this PR.

Net: this PR covers the keepempty=false cases (the default and only pushable shape) across all four where + dedup <variant> combinations — simple, dedup N, multi-field, and sort/collation. The keepempty=true case is an existing, intentional non-pushable shape, unchanged here.

[RyanL1997]

Checking on the CI failures

[github-actions]

Persistent review updated to latest commit 7725e0b

[github-actions]

Persistent review updated to latest commit f67458f

[github-actions]

Persistent review updated to latest commit a66ad5b

[github-actions]

Persistent review updated to latest commit 95e2aeb

[RyanL1997]

The current CI failure is unrelated.

The failure is:

 * What went wrong:
Execution failed for task ':doctest:startPrometheus'.
> java.net.SocketTimeoutException: Read timed out

[github-actions]

Persistent review updated to latest commit 38fc6c3