Lower frontend-private rel nodes before the analytics-engine handoff (fix dedup 500)

[ahkcs]

Description

PPL dedup followed by any pipe stage fails with a 500 on the analytics-engine route:

source=idx | dedup category | fields category
→ 500 RuntimeException: There was internal problem at backend

Server-side: IllegalStateException: Project rule encountered unmarked child [LogicalDedup] at OpenSearchProjectRule.onMatch. Deterministic for every dedup … | <anything> shape.

Resolves #22671.

Supersedes opensearch-project/OpenSearch#22675. That PR fixed the same issue on the analytics-engine side by having the engine reflect over foreign rel nodes to discover and run their convert rules. Per review feedback, the frontend should own its own rules rather than the engine repairing plans it receives — this PR takes that direction. The companion QA IT (DedupCommandIT) will land as a small OpenSearch-core PR.

Root cause

dedup is planned as a ROW_NUMBER() OVER (PARTITION BY keys) + Filter composite. CalciteToolsHelper.optimize — run on both the Lucene and analytics routes — then collapses that composite into a LogicalDedup via PPLSimplifyDedupRule, so that DedupPushdownRule can push dedup into a Lucene scan.

On the Lucene path, physical planning (Volcano) lowers the LogicalDedup back (or pushes it into the scan), so it disappears. On the analytics path, the optimized RelNode goes straight to the analytics engine with no Volcano step, so the LogicalDedup survives to the engine's marking phase, which rejects it as an unmarked child.

LogicalDedup exists solely to enable the Lucene pushdown (DedupPushdownRule is its only consumer); the analytics engine has no equivalent pushdown and cannot plan the node.

Fix

Add CalciteToolsHelper.optimizeForAnalytics, which runs the same HEP rules as optimize minus PPLSimplifyDedupRule. RestUnifiedQueryAction calls it on the analytics route, so the dedup composite is never collapsed and the analytics engine receives the standard ROW_NUMBER window form it can plan directly.

• No collapse-then-expand. Rather than generate a LogicalDedup and convert it back, the analytics route simply doesn't generate it — the rule that creates it (only useful for the Lucene pushdown) is skipped.
• Lucene path untouched. optimize is unchanged; the Lucene route keeps PPLSimplifyDedupRule and its dedup pushdown.
• Script-based thread-pool routing preserved. Optimization still runs before dispatch (ScriptDetector inspects the optimized plan), so worker-pool routing is unaffected.

Testing

test	before	after
CalcitePPLDedupTest.testOptimizeForAnalyticsKeepsRowNumberForm (optimize produces LogicalDedup; optimizeForAnalytics keeps ROW_NUMBER, no LogicalDedup, predicate preserved)	—	pass
CalcitePPLDedupTest.testOptimizeForAnalyticsMatchesOptimizeWithoutDedup (dedup-free plan identical under both variants)	—	pass
CalcitePPLDedupTest (full class, regression)	12/12	14/14 pass
Live analytics-engine cluster, parquet-backed index: dedup category | fields category, dedup category | stats count(), dedup 2 category | fields category	500 unmarked child [LogicalDedup]	200, correct rows

Check List

• New functionality includes testing.
• New functionality has been documented (method Javadoc + PR).
• Commits are signed per the DCO using --signoff or -s.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit bf03399)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ No major issues detected

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to bf03399

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Use instanceof for rule filtering The reference equality check (!=) may fail if PPLSimplifyDedupRule.DEDUP_SIMPLIFY_RULE is recreated or if the rule instance changes. Consider using equals() or filtering by rule class type to ensure robust filtering across different rule instances. core/src/main/java/org/opensearch/sql/calcite/utils/CalciteToolsHelper.java [562-568] private static final HepProgram ANALYTICS_HEP_PROGRAM = new HepProgramBuilder() .addRuleCollection( hepRuleList.stream() - .filter(rule -> rule != PPLSimplifyDedupRule.DEDUP_SIMPLIFY_RULE) + .filter(rule -> !(rule instanceof PPLSimplifyDedupRule)) .toList()) .build(); Suggestion importance[1-10]: 3 __ Why: While using instanceof could be more robust in some scenarios, the current reference equality check (!=) is valid when DEDUP_SIMPLIFY_RULE is a static constant. The suggestion assumes potential issues that may not exist in this codebase. The improvement is marginal and the current code is correct for filtering a specific rule instance from a static list.	Low

---

Previous suggestions

Suggestions up to commit 55c51fd

Category	Suggestion	Impact
Possible issue	Add null check for plan parameter Add null check for the plan parameter to prevent NullPointerException when setRoot is called with null. The HepPlanner will throw an exception if a null root is set, which could cause unexpected failures in the analytics execution path. core/src/main/java/org/opensearch/sql/executor/analytics/AnalyticsPlanLowering.java [35-39] public static RelNode lower(RelNode plan) { + if (plan == null) { + throw new IllegalArgumentException("plan cannot be null"); + } HepPlanner planner = new HepPlanner(PROGRAM); planner.setRoot(plan); return planner.findBestExp(); } Suggestion importance[1-10]: 5 __ Why: While adding null checks is generally good practice, this suggestion receives a moderate score because: (1) the callers in the PR (doExecute and doExplain) already handle exceptions and don't appear to pass null values, and (2) this is defensive programming rather than fixing an actual bug. The suggestion is valid but addresses a hypothetical edge case rather than a critical issue.	Low

Suggestions up to commit fad6e50

Category	Suggestion	Impact
General	Add null check for plan parameter Add null check for the plan parameter to prevent NullPointerException when setRoot is called with null. This ensures the method fails fast with a clear error message rather than propagating a null through the planner. core/src/main/java/org/opensearch/sql/executor/analytics/AnalyticsPlanLowering.java [52-56] public static RelNode lower(RelNode plan) { + if (plan == null) { + throw new IllegalArgumentException("plan cannot be null"); + } HepPlanner planner = new HepPlanner(PROGRAM); planner.setRoot(plan); return planner.findBestExp(); } Suggestion importance[1-10]: 5 __ Why: Adding a null check is a reasonable defensive programming practice that would provide a clearer error message. However, the impact is moderate since HepPlanner.setRoot() likely handles null appropriately, and the callers in the PR (RestUnifiedQueryAction) appear to always pass valid RelNode instances from the planner.	Low

[penghuo]

LogicalDedup is added becuase Pushdown needed it? If true, The other options is in RelNodeVisitor avoid generated LogicalDedup when use analytics engine.

[ahkcs]

Good call — done. Rather than generate a LogicalDedup and convert it back, the analytics route now just doesn't generate it: PPLSimplifyDedupRule only exists to collapse the ROW_NUMBER composite into a LogicalDedup for the Lucene DedupPushdownRule, and the analytics engine has no such pushdown. Added CalciteToolsHelper.optimizeForAnalytics (the same rules minus that one) and call it on the analytics route, so the engine gets the standard ROW_NUMBER window form directly.

[github-actions]

Persistent review updated to latest commit 55c51fd

[dai-chen]

1. At line 237, we've triggered optimizations already?
2. I think both lines changed the previous boundary (SQL/PPL generates raw plan and backend like AE responsible for optimizations). Shall we move both into AnalyticsExecutionEngine and later if needed explicitly call new UnifiedQueryPlanner.optmize or UnifiedQueryOptimizer API?

[mch2]

I'd like to re-think our SQL → AE integration. We've accumulated a lot of one-offs in AE, substrait adapters that exist to accommodate PPL specific operators and a test time circular dependency since AE's integration tests now need the SQL plugin installed to produce plans and test e2e.

Thinking AE should publish a contract and have front-ends like SQL/PPL compile and optimize to it. AE validates against it at the boundary and rejects up front instead of accepting whatever arrives and failing late in substrait conversion or out on a data node.

[ahkcs]

Reworked since your comment — dropped lowering; the route now calls optimizeForAnalytics (rules minus PPLSimplifyDedupRule), so no LogicalDedup is generated.

Line 237 already optimized (predates this PR, #5656 — just swapped it). And it can't move into AnalyticsExecutionEngine yet — dispatchTask() routes thread pools via ScriptDetector on the optimized plan before execute().

Agree the boundary cleanup + Marc's AE-contract idea are worth a follow-up.

[github-actions]

Persistent review updated to latest commit bf03399