Preserve original expression text as column name in unified SQL

[dai-chen]

Description

This PR preserves the original expression text as column names by adding a SqlNode rewriter to the existing LanguageSpec.postParseRules hook, instead of Calcite's default synthetic EXPR$0, EXPR$1, etc.

Examples:

SELECT COUNT() FROM t       -> COUNT()   (was EXPR$0)
SELECT SUM(MyCol)            -> SUM(MyCol)
SELECT UPPER(name), age + 1  -> UPPER(name), age + 1
SELECT name, *, x AS y       -> unchanged

Implementation notes:

1. Column-naming conventions vary across engines: See comparison table. This PR produces canonical built-in function names with bare user identifiers (COUNT(*), SUM(MyCol), UPPER(name)).

2. Verbatim preservation (V2/PPL V3/MySQL) is debatable: SQL treats unquoted function names as case-insensitive (COUNT == count), so canonical labels are spec-consistent. Verbatim text also couples column names to source formatting — SUM(x)+1 and SUM(x) + 1 would produce different labels for the same query. We'll revisit verbatim preservation in [FEATURE] Unified SQL language across OpenSearch, Flint and Spark SQL #5346.

Related Issues

Resolves #5332

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 b16ae8c)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

📝 TODO sections api/src/main/java/org/opensearch/sql/api/spec/search/SearchExtension.java [26]: Non-search-specific and temporarily placed here

🔀 No multiple PR themes

⚡ Recommended focus areas for review Mutating Shared State The visit(SqlCall call) method mutates the SqlNodeList in-place via list.set(i, ...). Since INSTANCE is a singleton, concurrent use of this rewriter could lead to race conditions or unexpected behavior if the same SqlSelect node is visited from multiple threads. Consider whether the SqlNodeList mutation is safe given Calcite's node sharing semantics. public @Nullable SqlNode visit(SqlCall call) { SqlCall visited = (SqlCall) super.visit(call); if (visited instanceof SqlSelect select) { SqlNodeList list = select.getSelectList(); for (int i = 0; i < list.size(); i++) { list.set(i, aliasIfNeeded(list.get(i))); } } return visited; } Scalar Subquery Skipped The aliasIfNeeded method skips items with SqlKind.SELECT, which means scalar subqueries in the SELECT list won't get an alias. The test comment says "Calcite will fall back to EXPR$N", but this inconsistency (some expressions get aliases, others don't) may surprise users. Consider whether this behavior is intentional and document it clearly. private static SqlNode aliasIfNeeded(SqlNode item) { if (item.getKind() == SqlKind.AS || item.getKind() == SqlKind.SELECT || item instanceof SqlIdentifier) { return item; } return SqlValidatorUtil.addAlias(item, item.toSqlString(UNPARSE_CONFIG).getSql()); } Misplaced TODO The TODO comment notes that SelectItemAliasRewriter is "non-search-specific and temporarily placed here". This suggests the rewriter should eventually be moved to a more appropriate location (e.g., a base LanguageSpec or a shared utility). Leaving it here long-term may cause confusion about its scope and applicability. SelectItemAliasRewriter.INSTANCE, // TODO: non-search-specific and temporarily placed here NamedArgRewriter.INSTANCE);

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to b16ae8c

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Guard against unsafe cast after visiting The super.visit(call) can return a SqlNode that is not a SqlCall in some edge cases, making the unchecked cast unsafe. You should check the type before casting to avoid a potential ClassCastException at runtime. api/src/main/java/org/opensearch/sql/api/spec/search/SelectItemAliasRewriter.java [44] -SqlCall visited = (SqlCall) super.visit(call); +SqlNode visitedNode = super.visit(call); +if (!(visitedNode instanceof SqlCall visited)) { + return visitedNode; +} Suggestion importance[1-10]: 5 __ Why: The cast from super.visit(call) to SqlCall could theoretically fail if the visitor returns a different node type. However, in practice SqlShuttle.visit(SqlCall) is designed to return a SqlCall, making this edge case unlikely. Still, the defensive check improves robustness.	Low
	Add null check for select list *select.getSelectList() can return null when the select list is SELECT at the top level in some Calcite versions. You should add a null check before iterating to avoid a NullPointerException. api/src/main/java/org/opensearch/sql/api/spec/search/SelectItemAliasRewriter.java [46-49] SqlNodeList list = select.getSelectList(); -for (int i = 0; i < list.size(); i++) { - list.set(i, aliasIfNeeded(list.get(i))); +if (list != null) { + for (int i = 0; i < list.size(); i++) { + list.set(i, aliasIfNeeded(list.get(i))); + } } Suggestion importance[1-10]: 5 __ Why: getSelectList() can return null in some Calcite versions for SELECT *, which would cause a NullPointerException. The existing test testSkipsStar passes a SELECT * query, but the null check is still a valid defensive measure.	Low
General	Avoid shared stateful visitor singleton SqlShuttle is a stateful visitor that accumulates state during traversal. Sharing a single INSTANCE across concurrent queries or multiple sequential rewrites may cause incorrect behavior or race conditions. Consider instantiating a new SelectItemAliasRewriter per rewrite call instead of using a shared singleton. api/src/main/java/org/opensearch/sql/api/spec/search/SelectItemAliasRewriter.java [30] -public static final SelectItemAliasRewriter INSTANCE = new SelectItemAliasRewriter(); +// Remove the shared INSTANCE field and instantiate per use: +// In SelectItemAliasRewriter: +// (remove) public static final SelectItemAliasRewriter INSTANCE = new SelectItemAliasRewriter(); +// In callers, use: new SelectItemAliasRewriter() + Suggestion importance[1-10]: 4 __ Why: If SqlShuttle maintains mutable state during traversal, sharing INSTANCE across concurrent calls could cause issues. However, the improved_code doesn't clearly show the replacement pattern and the suggestion is somewhat speculative without confirming SqlShuttle is actually stateful in a problematic way.	Low

---

Previous suggestions

Suggestions up to commit 2166410

Category	Suggestion	Impact
Possible issue	Fix incorrect SqlKind check for scalar subqueries The check for SqlKind.SELECT is intended to skip scalar subqueries, but scalar subqueries have kind SCALAR_QUERY, not SELECT. The SELECT kind is for SqlSelect nodes, which are already handled by the instanceof SqlSelect check in visit. This means scalar subqueries used as select items will incorrectly get an alias applied. api/src/main/java/org/opensearch/sql/api/spec/search/SelectItemAliasRewriter.java [54-61] private static SqlNode aliasIfNeeded(SqlNode item) { if (item.getKind() == SqlKind.AS - || item.getKind() == SqlKind.SELECT + || item.getKind() == SqlKind.SCALAR_QUERY || item instanceof SqlIdentifier) { return item; } return SqlValidatorUtil.addAlias(item, item.toSqlString(UNPARSE_CONFIG).getSql()); } Suggestion importance[1-10]: 7 __ Why: The test testSkipsScalarSubquery actually shows the scalar subquery does get an alias applied (the inner MAX(x) gets AS \MAX(x)`), suggesting the SqlKind.SELECTcheck is intentional to skip the outer subquery wrapper from being aliased again. However, the suggestion raises a valid point thatSqlKind.SCALAR_QUERY` is the correct kind for scalar subqueries used as select items, which could cause incorrect behavior in production.	Medium
	Avoid sharing stateful shuttle as singleton SqlShuttle is not thread-safe because it maintains mutable state during traversal (e.g., an ArgHandler). Sharing a single INSTANCE across concurrent requests can lead to race conditions and incorrect rewrites. A new instance should be created per use instead of sharing a singleton. api/src/main/java/org/opensearch/sql/api/spec/search/SelectItemAliasRewriter.java [30] -public static final SelectItemAliasRewriter INSTANCE = new SelectItemAliasRewriter(); +public static SelectItemAliasRewriter newInstance() { + return new SelectItemAliasRewriter(); +} Suggestion importance[1-10]: 6 __ Why: This is a valid concern if SqlShuttle maintains mutable state during traversal. If the shuttle is used in concurrent contexts, sharing a singleton INSTANCE could cause race conditions. However, the impact depends on whether the actual traversal state in SqlShuttle is thread-local or shared, and the suggested change would require updating all call sites.	Low
	Guard against null select list and unsafe cast *The super.visit(call) may return a SqlNode that is not a SqlCall in some edge cases, causing a ClassCastException. The result should be checked before casting. Additionally, getSelectList() can return null for SELECT queries, which would cause a NullPointerException when calling list.size(). api/src/main/java/org/opensearch/sql/api/spec/search/SelectItemAliasRewriter.java [44-52] -SqlCall visited = (SqlCall) super.visit(call); -if (visited instanceof SqlSelect select) { +SqlNode visitedNode = super.visit(call); +if (visitedNode instanceof SqlSelect select) { SqlNodeList list = select.getSelectList(); - for (int i = 0; i < list.size(); i++) { - list.set(i, aliasIfNeeded(list.get(i))); + if (list != null) { + for (int i = 0; i < list.size(); i++) { + list.set(i, aliasIfNeeded(list.get(i))); + } } } -return visited; +return visitedNode; Suggestion importance[1-10]: 5 __ Why: The null check for getSelectList() is a valid defensive measure, though in practice Calcite typically returns an empty list rather than null for SELECT *. The cast concern is less critical since SqlShuttle.visit(SqlCall) is documented to return SqlNode but the shuttle pattern generally preserves the type. The improvement is minor but adds robustness.	Low

Suggestions up to commit 847d018

Category	Suggestion	Impact
Possible issue	Guard against null return from super visit super.visit(call) can return null for certain node types in SqlShuttle, which would cause a NullPointerException on the cast. Add a null check before casting and processing the result. api/src/main/java/org/opensearch/sql/api/spec/search/SelectItemAliasRewriter.java [43-52] -SqlCall visited = (SqlCall) super.visit(call); +SqlNode visitedNode = super.visit(call); +if (!(visitedNode instanceof SqlCall visited)) { + return visitedNode; +} if (visited instanceof SqlSelect select) { SqlNodeList list = select.getSelectList(); for (int i = 0; i < list.size(); i++) { list.set(i, aliasIfNeeded(list.get(i))); } } return visited; Suggestion importance[1-10]: 6 __ Why: The cast (SqlCall) super.visit(call) could throw a NullPointerException or ClassCastException if super.visit(call) returns null or a non-SqlCall node. The improved code properly handles this edge case with a null/type check before casting.	Low
	Avoid sharing mutable visitor instance SelectItemAliasRewriter extends SqlShuttle which is stateful (it uses a CallCopyingArgHandler internally during traversal). Sharing a single INSTANCE across concurrent requests could lead to race conditions or unexpected behavior. Consider making callers instantiate a new instance per use, or verify that SqlShuttle is truly stateless before sharing. api/src/main/java/org/opensearch/sql/api/spec/search/SelectItemAliasRewriter.java [30] -public static final SelectItemAliasRewriter INSTANCE = new SelectItemAliasRewriter(); +public static SelectItemAliasRewriter newInstance() { + return new SelectItemAliasRewriter(); +} Suggestion importance[1-10]: 5 __ Why: SqlShuttle uses a CallCopyingArgHandler internally during traversal which could be stateful. However, this is a valid concern worth investigating, though the actual risk depends on Calcite's implementation details. The suggestion to use a factory method is reasonable but may be overly cautious.	Low
General	Skip aliasing subquery select items SELECT expands to a SqlIdentifier with isStar() returning true, but SELECT t. is also a SqlIdentifier. Both are correctly skipped. However, SqlNodeList items (e.g., from VALUES) could also appear and calling toSqlString on unexpected node types may produce misleading alias names or throw. Consider also checking for SqlKind.SELECT to avoid aliasing subquery expressions used as select items. api/src/main/java/org/opensearch/sql/api/spec/search/SelectItemAliasRewriter.java [54-59] private static SqlNode aliasIfNeeded(SqlNode item) { - if (item.getKind() == SqlKind.AS || item instanceof SqlIdentifier) { + if (item.getKind() == SqlKind.AS + || item instanceof SqlIdentifier + || item.getKind() == SqlKind.SELECT) { return item; } return SqlValidatorUtil.addAlias(item, item.toSqlString(UNPARSE_CONFIG).getSql()); } Suggestion importance[1-10]: 3 __ Why: The suggestion to skip SqlKind.SELECT items is a minor edge case improvement. Subqueries as select items are uncommon, and the suggestion's rationale about SqlNodeList items is somewhat speculative without clear evidence of actual issues in the current implementation.	Low

Suggestions up to commit e48574a

Category	Suggestion	Impact
Possible issue	Guard against unsafe cast after visiting The super.visit(call) can return a SqlNode that is not a SqlCall (e.g., a SqlIdentifier or other node type), making the unchecked cast to SqlCall unsafe and potentially causing a ClassCastException at runtime. You should check the type before casting, or handle the non-SqlCall case gracefully. api/src/main/java/org/opensearch/sql/api/spec/search/SelectItemAliasRewriter.java [48] -SqlCall visited = (SqlCall) super.visit(call); +SqlNode visitedNode = super.visit(call); +if (!(visitedNode instanceof SqlCall visited)) { + return visitedNode; +} Suggestion importance[1-10]: 6 __ Why: The super.visit(call) in SqlShuttle is designed to return the same type as the input (a SqlCall), so in practice this cast is safe. However, the suggestion is technically valid as a defensive measure since the return type is SqlNode. The improvement is minor given the actual Calcite API contract.	Low
	Guard against null select list *select.getSelectList() can return null when SELECT is used (Calcite may represent it as a null list). Calling .size() on a null reference would throw a NullPointerException. Add a null check before iterating. api/src/main/java/org/opensearch/sql/api/spec/search/SelectItemAliasRewriter.java [50-53] SqlNodeList list = select.getSelectList(); +if (list == null) { + return visited; +} for (int i = 0; i < list.size(); i++) { list.set(i, aliasIfNeeded(list.get(i))); } Suggestion importance[1-10]: 6 __ Why: The existing test testSkipsStar passes SELECT *, and the test in SelectItemAliasRewriterTest also tests this case, suggesting the null check may be needed. If getSelectList() returns null for SELECT *, this would cause a NullPointerException, making this a valid defensive fix.	Low
General	Sanitize derived alias string before use The alias string derived from toSqlString may contain characters (e.g., parentheses, spaces, operators) that are invalid or problematic as SQL identifiers in downstream processing. Consider sanitizing or truncating the alias string to ensure it is a valid identifier, or at least trimming whitespace. api/src/main/java/org/opensearch/sql/api/spec/search/SelectItemAliasRewriter.java [62] -return SqlValidatorUtil.addAlias(item, item.toSqlString(UNPARSE_CONFIG).getSql()); +String alias = item.toSqlString(UNPARSE_CONFIG).getSql().trim(); +return SqlValidatorUtil.addAlias(item, alias); Suggestion importance[1-10]: 2 __ Why: The suggestion only adds a .trim() call, which provides minimal benefit since toSqlString with the configured UNPARSE_CONFIG (indentation=0, no newlines) is unlikely to produce leading/trailing whitespace. The concern about invalid identifier characters is addressed by Calcite's quoting mechanism, not by trimming.	Low

Suggestions up to commit 30c8898

Category	Suggestion	Impact
Possible issue	Handle nullable return from super visit super.visit(call) can return null according to the @Nullable contract of SqlShuttle. Casting a null return value to SqlCall will throw a NullPointerException. Add a null-check before the cast and return null early if needed. api/src/main/java/org/opensearch/sql/api/spec/search/SelectItemAliasRewriter.java [47-53] public @Nullable SqlNode visit(SqlCall call) { - SqlCall visited = (SqlCall) super.visit(call); + SqlNode visitedNode = super.visit(call); + if (visitedNode == null) { + return null; + } + SqlCall visited = (SqlCall) visitedNode; if (visited instanceof SqlSelect select) { rewriteSelectList(select); } return visited; } Suggestion importance[1-10]: 7 __ Why: The @Nullable annotation on super.visit(call) means a null return is possible per contract, and casting null to SqlCall would throw a NullPointerException. This is a valid defensive fix for a potential runtime error.	Medium
	Guard against null select list *If select.getSelectList() returns null (e.g., for SELECT without an explicit list in some Calcite versions), iterating over it will throw a NullPointerException. Add a null-check before processing the select list to guard against this edge case. api/src/main/java/org/opensearch/sql/api/spec/search/SelectItemAliasRewriter.java [55-62] private static void rewriteSelectList(SqlSelect select) { SqlNodeList old = select.getSelectList(); + if (old == null) { + return; + } SqlNodeList rewritten = new SqlNodeList(old.getParserPosition()); for (SqlNode item : old) { rewritten.add(aliasIfNeeded(item)); } select.setSelectList(rewritten); } Suggestion importance[1-10]: 6 __ Why: A null check on getSelectList() is a reasonable defensive guard, though the UNPARSE_CONFIG already sets withSelectListItemsOnSeparateLines(false) and the test for SELECT * passes without it. Still, it's a valid safety improvement for edge cases.	Low
General	Normalize whitespace in derived alias names The alias string derived from toSqlString may contain characters (e.g., newlines, excessive whitespace) that produce unexpected column names. Consider trimming and normalizing whitespace in the alias string to ensure clean, predictable column names. api/src/main/java/org/opensearch/sql/api/spec/search/SelectItemAliasRewriter.java [69] -return SqlValidatorUtil.addAlias(item, item.toSqlString(UNPARSE_CONFIG).getSql()); +String alias = item.toSqlString(UNPARSE_CONFIG).getSql().trim().replaceAll("\\s+", " "); +return SqlValidatorUtil.addAlias(item, alias); Suggestion importance[1-10]: 3 __ Why: The UNPARSE_CONFIG already sets withIndentation(0) and withSelectListItemsOnSeparateLines(false), making unexpected whitespace unlikely. This is a minor defensive improvement with low practical impact.	Low

Suggestions up to commit b856cd7

Category	Suggestion	Impact
Possible issue	Guard against null select list *select.getSelectList() can return null for SELECT queries in some Calcite versions, which would cause a NullPointerException when iterating. A null check should be added before processing the select list. api/src/main/java/org/opensearch/sql/api/spec/search/SelectItemAliasRewriter.java [52-59] private static void rewriteSelectList(SqlSelect select) { SqlNodeList old = select.getSelectList(); + if (old == null) { + return; + } SqlNodeList rewritten = new SqlNodeList(old.getParserPosition()); for (SqlNode item : old) { rewritten.add(aliasIfNeeded(item)); } select.setSelectList(rewritten); } Suggestion importance[1-10]: 6 __ Why: This is a valid defensive programming suggestion - getSelectList() can return null for SELECT * in some Calcite versions, which would cause a NullPointerException. The test testStarUntouched passes a SELECT * query, suggesting this edge case exists, though it may be handled differently in the tested Calcite version.	Low
General	Normalize whitespace in derived alias names The alias string derived from toSqlString may contain characters (e.g., newlines, extra whitespace from complex expressions) that make it an invalid or unexpected column name. Consider trimming and normalizing whitespace in the alias string to avoid surprising column names. api/src/main/java/org/opensearch/sql/api/spec/search/SelectItemAliasRewriter.java [66] -return SqlValidatorUtil.addAlias(item, item.toSqlString(UNPARSE_CONFIG).getSql()); +String alias = item.toSqlString(UNPARSE_CONFIG).getSql().trim().replaceAll("\\s+", " "); +return SqlValidatorUtil.addAlias(item, alias); Suggestion importance[1-10]: 4 __ Why: The UNPARSE_CONFIG already sets withIndentation(0) and withSelectListItemsOnSeparateLines(false), which reduces whitespace issues. However, trimming and normalizing whitespace could still be a minor improvement for edge cases with complex expressions that might produce unexpected whitespace in alias names.	Low
	Document thread-safety assumption for singleton rewriter SelectItemAliasRewriter is stateless and used as a singleton via INSTANCE, but SqlShuttle is not documented as thread-safe. If visit is ever called concurrently (e.g., in a multi-threaded query planner), shared mutable state in the parent class could cause issues. Consider verifying thread-safety of SqlShuttle, or document the assumption that this rewriter is only used single-threaded. api/src/main/java/org/opensearch/sql/api/spec/search/SelectItemAliasRewriter.java [31] +// If SqlShuttle is confirmed stateless/thread-safe, keep as-is and add a comment: +/** Singleton instance. Safe for concurrent use only if SqlShuttle is stateless. */ public static final SelectItemAliasRewriter INSTANCE = new SelectItemAliasRewriter(); Suggestion importance[1-10]: 2 __ Why: This suggestion asks to add a comment about thread-safety, which is a documentation concern. The improved_code only adds a comment rather than making a functional change, and the suggestion score should be low per guidelines about docstring/comment additions.	Low

[github-actions]

Persistent review updated to latest commit 30c8898

[github-actions]

Persistent review updated to latest commit e48574a

[github-actions]

Persistent review updated to latest commit 847d018

[github-actions]

Persistent review updated to latest commit 2166410

[github-actions]

Persistent review updated to latest commit b16ae8c