Skip to content

Commit 2166410

Browse files
committed
feat(api): Preserve original SQL expression text as column name
Calcite's SqlToRelConverter names unnamed SELECT items EXPR$0, EXPR$1, etc. - surprising versus PostgreSQL/MySQL/Spark. Fix by adding SelectItemAliasRewriter, a pre-validation SqlShuttle that wraps unnamed, non-identifier items with AS <text> via the existing LanguageSpec.postParseRules hook. SELECT COUNT(*) FROM t -> `COUNT(*)` (was EXPR$0) SELECT UPPER(name) -> `UPPER(name)` SELECT x AS y, name, * -> unchanged Resolves #5332 Signed-off-by: Chen Dai <daichen@amazon.com>
1 parent da1410b commit 2166410

5 files changed

Lines changed: 175 additions & 2 deletions

File tree

api/src/main/java/org/opensearch/sql/api/spec/search/SearchExtension.java

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,8 @@ public SqlOperatorTable operators() {
2222

2323
@Override
2424
public List<SqlVisitor<SqlNode>> postParseRules() {
25-
return List.of(NamedArgRewriter.INSTANCE);
25+
return List.of(
26+
SelectItemAliasRewriter.INSTANCE, // TODO: non-search-specific and temporarily placed here
27+
NamedArgRewriter.INSTANCE);
2628
}
2729
}
Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,62 @@
1+
/*
2+
* Copyright OpenSearch Contributors
3+
* SPDX-License-Identifier: Apache-2.0
4+
*/
5+
6+
package org.opensearch.sql.api.spec.search;
7+
8+
import java.util.function.UnaryOperator;
9+
import lombok.AccessLevel;
10+
import lombok.NoArgsConstructor;
11+
import org.apache.calcite.sql.SqlCall;
12+
import org.apache.calcite.sql.SqlIdentifier;
13+
import org.apache.calcite.sql.SqlKind;
14+
import org.apache.calcite.sql.SqlNode;
15+
import org.apache.calcite.sql.SqlNodeList;
16+
import org.apache.calcite.sql.SqlSelect;
17+
import org.apache.calcite.sql.SqlWriterConfig;
18+
import org.apache.calcite.sql.dialect.AnsiSqlDialect;
19+
import org.apache.calcite.sql.util.SqlShuttle;
20+
import org.apache.calcite.sql.validate.SqlValidatorUtil;
21+
import org.checkerframework.checker.nullness.qual.Nullable;
22+
23+
/**
24+
* Wraps unnamed SELECT-list items with {@code AS <text>} so Calcite uses the expression text as the
25+
* column name instead of synthetic {@code EXPR$N}.
26+
*/
27+
@NoArgsConstructor(access = AccessLevel.PRIVATE)
28+
public final class SelectItemAliasRewriter extends SqlShuttle {
29+
30+
public static final SelectItemAliasRewriter INSTANCE = new SelectItemAliasRewriter();
31+
32+
/** Unparse config: bare identifiers (e.g., {@code SUM(a)} not {@code SUM(`a`)}). */
33+
private static final UnaryOperator<SqlWriterConfig> UNPARSE_CONFIG =
34+
c ->
35+
c.withDialect(AnsiSqlDialect.DEFAULT)
36+
.withQuoteAllIdentifiers(false)
37+
.withAlwaysUseParentheses(false)
38+
.withSelectListItemsOnSeparateLines(false)
39+
.withUpdateSetListNewline(false)
40+
.withIndentation(0);
41+
42+
@Override
43+
public @Nullable SqlNode visit(SqlCall call) {
44+
SqlCall visited = (SqlCall) super.visit(call);
45+
if (visited instanceof SqlSelect select) {
46+
SqlNodeList list = select.getSelectList();
47+
for (int i = 0; i < list.size(); i++) {
48+
list.set(i, aliasIfNeeded(list.get(i)));
49+
}
50+
}
51+
return visited;
52+
}
53+
54+
private static SqlNode aliasIfNeeded(SqlNode item) {
55+
if (item.getKind() == SqlKind.AS
56+
|| item.getKind() == SqlKind.SELECT
57+
|| item instanceof SqlIdentifier) {
58+
return item;
59+
}
60+
return SqlValidatorUtil.addAlias(item, item.toSqlString(UNPARSE_CONFIG).getSql());
61+
}
62+
}

api/src/test/java/org/opensearch/sql/api/UnifiedQueryPlannerSqlTest.java

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -257,4 +257,22 @@ SELECT department, count(*)
257257
""")
258258
.assertErrorMessage("Encountered");
259259
}
260+
261+
@Test
262+
public void testUnnamedMixedAggregates() {
263+
givenQuery(
264+
"""
265+
SELECT id, COUNT(*), AVG(age) FROM catalog.employees\
266+
""")
267+
.assertFields("id", "COUNT(*)", "AVG(age)");
268+
}
269+
270+
@Test
271+
public void testExplicitAliasStillWorks() {
272+
givenQuery(
273+
"""
274+
SELECT COUNT(*) AS cnt FROM catalog.employees\
275+
""")
276+
.assertFields("cnt");
277+
}
260278
}

api/src/test/java/org/opensearch/sql/api/UnifiedRelevanceSearchSqlTest.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -137,7 +137,7 @@ SELECT upper(name) FROM catalog.employees\
137137
""")
138138
.assertPlan(
139139
"""
140-
LogicalProject(EXPR$0=[UPPER($1)])
140+
LogicalProject(UPPER(name)=[UPPER($1)])
141141
LogicalTableScan(table=[[catalog, employees]])
142142
""");
143143
}
Lines changed: 91 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,91 @@
1+
/*
2+
* Copyright OpenSearch Contributors
3+
* SPDX-License-Identifier: Apache-2.0
4+
*/
5+
6+
package org.opensearch.sql.api.spec.search;
7+
8+
import static org.junit.Assert.assertTrue;
9+
10+
import org.apache.calcite.sql.SqlNode;
11+
import org.apache.calcite.sql.parser.SqlParser;
12+
import org.junit.Test;
13+
import org.opensearch.sql.api.spec.UnifiedSqlSpec;
14+
15+
/** Unit tests for {@link SelectItemAliasRewriter}. */
16+
public class SelectItemAliasRewriterTest {
17+
18+
/** Production parser config from {@link UnifiedSqlSpec}. */
19+
private static final SqlParser.Config PARSER_CONFIG = UnifiedSqlSpec.extended().parserConfig();
20+
21+
@Test
22+
public void testAliasesUnnamedExpression() throws Exception {
23+
SqlNode result = rewrite("SELECT COUNT(*) FROM t");
24+
assertContains(result, "AS `COUNT(*)`");
25+
}
26+
27+
@Test
28+
public void testPreservesExplicitAlias() throws Exception {
29+
SqlNode result = rewrite("SELECT COUNT(*) AS cnt FROM t");
30+
assertContains(result, "SELECT COUNT(*) AS `cnt`");
31+
}
32+
33+
@Test
34+
public void testSkipsSimpleIdentifier() throws Exception {
35+
SqlNode result = rewrite("SELECT name FROM t");
36+
assertContains(result, "SELECT `name`");
37+
}
38+
39+
@Test
40+
public void testSkipsStar() throws Exception {
41+
SqlNode result = rewrite("SELECT * FROM t");
42+
assertContains(result, "SELECT *");
43+
}
44+
45+
@Test
46+
public void testSkipsQualifiedStar() throws Exception {
47+
SqlNode result = rewrite("SELECT t.* FROM t");
48+
assertContains(result, "SELECT `t`.*");
49+
}
50+
51+
@Test
52+
public void testSkipsScalarSubquery() throws Exception {
53+
// Avoids producing a column name like "SELECT MAX(x) AS MAX(x) FROM u" from our own
54+
// recursion. Calcite will fall back to EXPR$N, matching Flink/Druid/Drill default.
55+
SqlNode result = rewrite("SELECT (SELECT MAX(x) FROM u) FROM t");
56+
assertContains(result, "SELECT (SELECT MAX(`x`) AS `MAX(x)` FROM `u`) FROM `t`");
57+
}
58+
59+
@Test
60+
public void testRecursesIntoSubquery() throws Exception {
61+
SqlNode result = rewrite("SELECT x FROM (SELECT COUNT(*) FROM t) s");
62+
assertContains(result, "AS `COUNT(*)`");
63+
}
64+
65+
@Test
66+
public void testHandlesMixedItemsCorrectly() throws Exception {
67+
SqlNode result = rewrite("SELECT id, COUNT(*), name AS n FROM t");
68+
assertContains(result, "SELECT `id`, COUNT(*) AS `COUNT(*)`, `name` AS `n`");
69+
}
70+
71+
@Test
72+
public void testPreservesIdentifierInAggregate() throws Exception {
73+
SqlNode result = rewrite("SELECT SUM(MyCol), SUM(x + 1) FROM t");
74+
assertContains(result, "AS `SUM(MyCol)`");
75+
assertContains(result, "AS `SUM(x + 1)`");
76+
}
77+
78+
private static SqlNode rewrite(String sql) throws Exception {
79+
return parse(sql).accept(SelectItemAliasRewriter.INSTANCE);
80+
}
81+
82+
private static SqlNode parse(String sql) throws Exception {
83+
return SqlParser.create(sql, PARSER_CONFIG).parseStmt();
84+
}
85+
86+
private static void assertContains(SqlNode node, String expected) {
87+
String actual = node.toString().replaceAll("\\n", " ");
88+
assertTrue(
89+
"Expected to contain: " + expected + "\nActual: " + actual, actual.contains(expected));
90+
}
91+
}

0 commit comments

Comments
 (0)