[BugFix] Preserve aggregate aliases for unified query SQL

[dai-chen]

Description

This PR fixes SQL aggregate aliases being lost in the unified query SQL path — e.g. SELECT city, COUNT(*) AS cnt FROM t GROUP BY city returned the column as COUNT(*) instead of cnt.

• Root cause: when a projection only renames a column without adding/removing/reordering, RelBuilder.project (default force=false) treats it as a no-op and skips emitting the LogicalProject (this minimizes the plan tree, since names of intermediate projections don't matter). We now force the SELECT projection to materialize when it renames a field, so the alias survives in the output schema.
• PPL impact: PPL never produces an AS RexCall through this path (rename/eval/fields/stats build output names differently), so the change never triggers for PPL.

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 9a12430)

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 code assumes AS RexCall always has exactly two operands with specific types (RexInputRef and RexLiteral). If the operands differ (e.g., a complex expression aliased, or unexpected operand types), calling getOperands().get(0) or get(1) and casting them will throw ClassCastException or IndexOutOfBoundsException. This can occur if Calcite's internal representation changes or if an edge-case query produces a different RexCall structure. RexCall as = (RexCall) r; if (as.getOperands().get(0) instanceof RexInputRef ref) { String name = ((RexLiteral) as.getOperands().get(1)).getValueAs(String.class); if (!name.equals(currentFields.get(ref.getIndex()))) { return true; }

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 9a12430

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Add index bounds validation Add bounds checking before accessing currentFields by index. If ref.getIndex() is out of bounds, this will throw an IndexOutOfBoundsException. Validate that the index is within the valid range of currentFields before accessing it. core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java [493-506] private static boolean isRenameFieldsProject(List<RexNode> fields, List<String> currentFields) { for (RexNode r : fields) { if (r.getKind() == AS) { RexCall as = (RexCall) r; if (as.getOperands().get(0) instanceof RexInputRef ref) { + int index = ref.getIndex(); + if (index < 0 || index >= currentFields.size()) { + continue; + } String name = ((RexLiteral) as.getOperands().get(1)).getValueAs(String.class); - if (!name.equals(currentFields.get(ref.getIndex()))) { + if (!name.equals(currentFields.get(index))) { return true; } } } } return false; } Suggestion importance[1-10]: 7 __ Why: Valid suggestion to add bounds checking before accessing currentFields.get(ref.getIndex()). This prevents potential IndexOutOfBoundsException if the index is out of range, improving code robustness.	Medium
	Validate AS operands before access Add null safety checks for the operands access. The getOperands().get(1) call could throw IndexOutOfBoundsException if the AS node doesn't have a second operand, and the cast to RexLiteral could fail if the operand is not a literal. core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java [497-498] if (as.getOperands().get(0) instanceof RexInputRef ref) { + if (as.getOperands().size() < 2 || !(as.getOperands().get(1) instanceof RexLiteral)) { + continue; + } String name = ((RexLiteral) as.getOperands().get(1)).getValueAs(String.class); Suggestion importance[1-10]: 7 __ Why: Valid suggestion to add safety checks before accessing as.getOperands().get(1) and casting to RexLiteral. This prevents potential IndexOutOfBoundsException and ClassCastException, improving error handling.	Medium

---

Previous suggestions

Suggestions up to commit ef61b83

Category	Suggestion	Impact
Possible issue	Add bounds checking for array access Add bounds checking before accessing currentFields.get(ref.getIndex()) to prevent potential IndexOutOfBoundsException. The ref.getIndex() value may exceed the size of currentFields in certain edge cases. core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java [493-506] private static boolean isRenameFieldsProject(List<RexNode> fields, List<String> currentFields) { for (RexNode r : fields) { if (r.getKind() == AS) { RexCall as = (RexCall) r; if (as.getOperands().get(0) instanceof RexInputRef ref) { + int index = ref.getIndex(); + if (index >= currentFields.size()) { + continue; + } String name = ((RexLiteral) as.getOperands().get(1)).getValueAs(String.class); - if (!name.equals(currentFields.get(ref.getIndex()))) { + if (!name.equals(currentFields.get(index))) { return true; } } } } return false; } Suggestion importance[1-10]: 7 __ Why: Valid safety improvement to prevent IndexOutOfBoundsException when ref.getIndex() exceeds currentFields.size(). However, this is likely a defensive check since the fields should normally be aligned in the Calcite plan context.	Medium
	Handle null literal values safely Add null safety check for getValueAs(String.class) result. If the literal value is null or cannot be converted to String, this could cause a NullPointerException when calling equals(). core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java [497-498] if (as.getOperands().get(0) instanceof RexInputRef ref) { String name = ((RexLiteral) as.getOperands().get(1)).getValueAs(String.class); + if (name == null) { + continue; + } Suggestion importance[1-10]: 6 __ Why: Adds null safety for getValueAs(String.class) result before calling equals(). While this is a reasonable defensive check, in the context of Calcite's RexLiteral for AS aliases, the name should typically be non-null.	Low

Suggestions up to commit cb5a0de

Category	Suggestion	Impact
Possible issue	Add index bounds validation Add bounds checking before accessing currentFields by index. If ref.getIndex() is out of bounds, this will throw an IndexOutOfBoundsException. Validate that the index is within the valid range before accessing the list. core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java [493-506] private static boolean isRenameFieldsProject(List<RexNode> fields, List<String> currentFields) { for (RexNode r : fields) { if (r.getKind() == AS) { RexCall as = (RexCall) r; if (as.getOperands().get(0) instanceof RexInputRef ref) { + int index = ref.getIndex(); + if (index < 0 || index >= currentFields.size()) { + continue; + } String name = ((RexLiteral) as.getOperands().get(1)).getValueAs(String.class); - if (!name.equals(currentFields.get(ref.getIndex()))) { + if (!name.equals(currentFields.get(index))) { return true; } } } } return false; } Suggestion importance[1-10]: 7 __ Why: Valid defensive programming suggestion. Adding bounds checking for ref.getIndex() before accessing currentFields prevents potential IndexOutOfBoundsException. However, in a well-formed Calcite plan, the index should always be valid, so this is more of a safety measure than fixing a critical bug.	Medium
	Validate operands list size Add null safety checks for operands access. The getOperands() list could be empty or have fewer than 2 elements, causing IndexOutOfBoundsException. Verify the operands list size before accessing elements. core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java [497-498] -if (as.getOperands().get(0) instanceof RexInputRef ref) { +if (as.getOperands().size() >= 2 && as.getOperands().get(0) instanceof RexInputRef ref) { String name = ((RexLiteral) as.getOperands().get(1)).getValueAs(String.class); Suggestion importance[1-10]: 7 __ Why: Good defensive check to prevent IndexOutOfBoundsException when accessing operands. The AS operator should always have 2 operands in a valid Calcite plan, but adding this validation improves robustness against malformed input.	Medium
	Validate type before casting Add type checking before casting to RexLiteral. If the second operand is not a RexLiteral, this cast will throw a ClassCastException. Verify the operand type before casting. core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java [498] -String name = ((RexLiteral) as.getOperands().get(1)).getValueAs(String.class); +if (!(as.getOperands().get(1) instanceof RexLiteral literal)) { + continue; +} +String name = literal.getValueAs(String.class); Suggestion importance[1-10]: 7 __ Why: Prevents ClassCastException by verifying the operand is a RexLiteral before casting. While the AS operator's second operand should be a literal in valid plans, this check adds safety against unexpected node structures.	Medium

Suggestions up to commit 04a82d3

Category	Suggestion	Impact
Possible issue	Add index bounds validation Add bounds checking before accessing currentFields.get(ref.getIndex()) to prevent potential IndexOutOfBoundsException. The ref.getIndex() value should be validated against the size of currentFields list before accessing it. core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java [492-505] private static boolean isRenameFieldsProject(List<RexNode> fields, List<String> currentFields) { for (RexNode r : fields) { if (r.getKind() == AS) { RexCall as = (RexCall) r; if (as.getOperands().get(0) instanceof RexInputRef ref) { + int index = ref.getIndex(); + if (index >= currentFields.size()) { + continue; + } String name = ((RexLiteral) as.getOperands().get(1)).getValueAs(String.class); - if (!name.equals(currentFields.get(ref.getIndex()))) { + if (!name.equals(currentFields.get(index))) { return true; } } } } return false; } Suggestion importance[1-10]: 7 __ Why: Valid defensive programming suggestion to prevent potential IndexOutOfBoundsException. However, in the context of this code processing Calcite's internal structures, the index should normally be valid, making this a moderate-priority improvement rather than a critical fix.	Medium
	Add operand type validation Add null safety checks for the operands access. The as.getOperands().get(1) could potentially be null or not a RexLiteral, which would cause a ClassCastException or NullPointerException. core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java [496-497] if (as.getOperands().get(0) instanceof RexInputRef ref) { + if (as.getOperands().size() < 2 || !(as.getOperands().get(1) instanceof RexLiteral)) { + continue; + } String name = ((RexLiteral) as.getOperands().get(1)).getValueAs(String.class); Suggestion importance[1-10]: 7 __ Why: Reasonable safety check to prevent ClassCastException when accessing operands. The code assumes the AS operator structure, but adding validation improves robustness. Score is moderate as this is defensive programming rather than fixing a known bug.	Medium

[github-actions]

Persistent review updated to latest commit cb5a0de

[github-actions]

Persistent review updated to latest commit ef61b83

[github-actions]

Persistent review updated to latest commit 9a12430