Resolve SQL unified query gaps for HAVING, FROM-less SELECT, window ORDER BY

[dai-chen]

Description

Follow-up to #5450, addressing three more gaps in the SQL V2 path on the Analytics Engine:

• HAVING clause: visitAggregateFunction resolves via a Map<AggregateFunction, Integer> registry on CalcitePlanContext, populated by visitAggregation using position arithmetic.
• FROM-less SELECT: visitValues uses LogicalValues.createOneRow so SELECT 1 returns one row instead of zero.
• Window ORDER BY: translateOrderKeys defaults unspecified NULLS to NULLS FIRST, matching top-level ORDER BY.

Related Issues

Part of #5248

Check List

• New functionality includes testing.
• New functionality has been documented.
• New functionality has javadoc added.
• New functionality has a user manual doc added.
• 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.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 32dc35c)

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 The condition rows == null || rows.isEmpty() creates a zero-row, zero-column relation, but the comment says this is for "PPL empty subsearch". If rows is null, this may indicate a planner bug rather than a valid empty subsearch. Consider whether null should be handled separately or trigger an error. if (rows == null || rows.isEmpty()) { // PPL empty subsearch (e.g., `... | append [ ]`): zero rows, no columns. context.relBuilder.values(context.relBuilder.getTypeFactory().builder().build()); return context.relBuilder.peek(); } Possible Issue If visitAggregateFunction is called before visitAggregation populates the registry, or if the same AggregateFunction instance is reused across multiple aggregation scopes without re-registration, the lookup will fail. The comment mentions "V2 grammar doesn't allow subqueries that nest aggregation scopes", but if that assumption is violated or if the AST reuses nodes, this will throw an exception. public RexNode visitAggregateFunction(AggregateFunction node, CalcitePlanContext context) { // Resolve post-aggregate AggregateFunction via registry populated in visitAggregation. Integer index = context.getAggregateOutputIndex().get(node); if (index == null) { throw new IllegalStateException( "Aggregate function " + node + " not registered (planner bug)"); } return context.relBuilder.field(index); }

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 32dc35c

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Enhance error message with diagnostics The error message should include more diagnostic information to aid debugging. Include the aggregate function's name or details and the current state of the registry to help identify why the lookup failed. core/src/main/java/org/opensearch/sql/calcite/CalciteRexNodeVisitor.java [111-119] @Override public RexNode visitAggregateFunction(AggregateFunction node, CalcitePlanContext context) { // Resolve post-aggregate AggregateFunction via registry populated in visitAggregation. Integer index = context.getAggregateOutputIndex().get(node); if (index == null) { throw new IllegalStateException( - "Aggregate function " + node + " not registered (planner bug)"); + "Aggregate function " + node.getFuncName() + " not registered. Available: " + + context.getAggregateOutputIndex().keySet() + " (planner bug)"); } return context.relBuilder.field(index); } Suggestion importance[1-10]: 5 __ Why: Adding diagnostic information to error messages improves debuggability. However, the suggestion assumes getFuncName() exists on AggregateFunction without verifying this in the PR diff. The improvement is moderate as it aids troubleshooting but doesn't fix a critical issue.	Low
	Document schema expectations for one-row Verify that LogicalValues.createOneRow() produces the expected schema for downstream operations. If the one-row relation needs specific column types or names, ensure they are properly configured to avoid type mismatches in subsequent projections. core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java [4207-4211] if (rows.size() == 1 && rows.get(0).isEmpty()) { // SQL FROM-less SELECT (dual table) encoded as Values([[]]): one-row relation for Project. + // createOneRow() produces a single row with zero columns, suitable for scalar projections. context.relBuilder.push(LogicalValues.createOneRow(context.relBuilder.getCluster())); return context.relBuilder.peek(); } Suggestion importance[1-10]: 2 __ Why: The suggestion only adds a comment clarifying what createOneRow() produces, which is already explained in the existing comment. The improved_code is nearly identical to existing_code with minimal added value. This is a documentation-only change with very low impact.	Low
Possible issue	Validate empty map before clearing The clear() operation on aggregateOutputIndex assumes no nested aggregation scopes, but this assumption may not hold if the grammar evolves or if subqueries with aggregations are introduced. Consider validating that the map is empty before clearing, or use a scoped approach (e.g., stack-based) to handle potential nesting safely. core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java [1752-1761] -// Register aggregate output indices for HAVING / post-aggregate resolution. clear() is safe: -// V2 grammar doesn't allow subqueries that nest aggregation scopes above this point. -context.getAggregateOutputIndex().clear(); +// Register aggregate output indices for HAVING / post-aggregate resolution. +if (!context.getAggregateOutputIndex().isEmpty()) { + throw new IllegalStateException("Nested aggregation scopes detected (planner bug)"); +} int aggStartIdx = metricsFirst ? 0 : aliasedGroupByList.size(); for (int i = 0; i < aggExprList.size(); i++) { AggregateFunction aggFunc = extractAggregateFunction(aggExprList.get(i)); if (aggFunc != null) { context.getAggregateOutputIndex().put(aggFunc, aggStartIdx + i); } } Suggestion importance[1-10]: 3 __ Why: While adding validation could catch potential bugs, the comment explicitly states that clear() is safe for V2 grammar. The suggestion adds defensive programming but may be overly cautious given the documented constraints. The impact is low since the current implementation is intentional.	Low

---

Previous suggestions

Suggestions up to commit 8b8d26a

Category	Suggestion	Impact
General	Handle null explicitly before switch The switch statement handles null as a case, which is unconventional and may cause confusion. Consider explicitly checking for null before the switch or using Optional to make the null-handling intent clearer and more maintainable. core/src/main/java/org/opensearch/sql/calcite/CalciteRexNodeVisitor.java [666-672] NullOrder no = opt.getNullOrder(); +if (no == null) { + // Unspecified NULLS defaults to NULLS FIRST, matching top-level ORDER BY. + return b.nullsFirst(field); +} return switch (no) { - // Unspecified NULLS defaults to NULLS FIRST, matching top-level ORDER BY. - case null -> b.nullsFirst(field); case NULL_FIRST -> b.nullsFirst(field); case NULL_LAST -> b.nullsLast(field); }; Suggestion importance[1-10]: 5 __ Why: The suggestion improves code clarity by making null-handling more explicit and conventional. While the switch with null case works correctly in modern Java, extracting it to an explicit check improves readability and maintainability. This is a minor style improvement rather than a bug fix.	Low
Possible issue	Validate map state before clearing The clear() operation on aggregateOutputIndex assumes no nested aggregation scopes, but this assumption may not hold if the V2 grammar evolves or if subqueries with aggregates are introduced. Consider validating that the map is empty before clearing, or use a scoped approach to prevent accidental data loss. core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java [1752-1761] -// Register aggregate output indices for HAVING / post-aggregate resolution. clear() is safe: -// V2 grammar doesn't allow subqueries that nest aggregation scopes above this point. -context.getAggregateOutputIndex().clear(); +// Register aggregate output indices for HAVING / post-aggregate resolution. +if (!context.getAggregateOutputIndex().isEmpty()) { + throw new IllegalStateException("Nested aggregation scopes detected (planner bug)"); +} int aggStartIdx = metricsFirst ? 0 : aliasedGroupByList.size(); for (int i = 0; i < aggExprList.size(); i++) { AggregateFunction aggFunc = extractAggregateFunction(aggExprList.get(i)); if (aggFunc != null) { context.getAggregateOutputIndex().put(aggFunc, aggStartIdx + i); } } Suggestion importance[1-10]: 4 __ Why: The suggestion adds defensive validation but the comment explicitly states clear() is safe for V2 grammar. The validation would catch future bugs if the grammar evolves, but currently adds overhead without addressing an actual issue. The improved error message is helpful but the check may be overly defensive.	Low

Suggestions up to commit 017a584

Category	Suggestion	Impact
Possible issue	Handle aggregate function lookup robustly The equality check using get(node) relies on AggregateFunction object identity or proper equals()/hashCode() implementation. If AggregateFunction doesn't override these methods, the lookup will fail even for semantically identical functions, causing incorrect IllegalStateException. core/src/main/java/org/opensearch/sql/calcite/CalciteRexNodeVisitor.java [112-120] @Override public RexNode visitAggregateFunction(AggregateFunction node, CalcitePlanContext context) { // Resolve post-aggregate AggregateFunction via registry populated in visitAggregation. Integer index = context.getAggregateOutputIndex().get(node); if (index == null) { - throw new IllegalStateException( - "Aggregate function " + node + " not registered (planner bug)"); + // Fallback: try matching by function signature if direct lookup fails + index = findAggregateBySignature(node, context); + if (index == null) { + throw new IllegalStateException( + "Aggregate function " + node + " not registered (planner bug)"); + } } return context.relBuilder.field(index); } +private Integer findAggregateBySignature(AggregateFunction target, CalcitePlanContext context) { + for (Map.Entry<AggregateFunction, Integer> entry : context.getAggregateOutputIndex().entrySet()) { + if (entry.getKey().toString().equals(target.toString())) { + return entry.getValue(); + } + } + return null; +} + Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies a potential issue with object identity-based lookup in the aggregateOutputIndex map. If AggregateFunction lacks proper equals()/hashCode() implementation, lookups could fail. However, the fallback using toString() comparison is fragile and may not correctly handle all cases. A better solution would be ensuring AggregateFunction implements proper equality methods.	Low
	Prevent potential infinite recursion The recursive call to extractAggregateFunction for Alias may cause infinite recursion if circular aliases exist. Add a depth limit or visited set to prevent stack overflow in edge cases. core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java [1763-1767] private static AggregateFunction extractAggregateFunction(UnresolvedExpression expr) { + return extractAggregateFunction(expr, 0); +} + +private static AggregateFunction extractAggregateFunction(UnresolvedExpression expr, int depth) { + if (depth > 100) return null; // Prevent infinite recursion if (expr instanceof AggregateFunction af) return af; - if (expr instanceof Alias alias) return extractAggregateFunction(alias.getDelegated()); + if (expr instanceof Alias alias) return extractAggregateFunction(alias.getDelegated(), depth + 1); return null; } Suggestion importance[1-10]: 3 __ Why: While adding recursion depth protection is a defensive practice, circular aliases would typically be caught during earlier parsing/validation phases. The suggestion addresses an edge case that is unlikely in practice, making it a minor improvement.	Low

[github-actions]

Persistent review updated to latest commit 8b8d26a

[github-actions]

Persistent review updated to latest commit 32dc35c