Skip to content

Commit 498b25f

Browse files
committed
fix(calcite): handle Limit, Alias projection, RelationSubquery, and Values dual-table in visitor
Add visitLimit to support LIMIT/OFFSET clauses. Handle Alias nodes in project list by referencing already-computed aggregate fields instead of re-analyzing. Add visitRelationSubquery for derived tables in FROM clause. Fix visitValues to treat a single empty row as a dual-table (SELECT without FROM). Make bucketNullable lookup null-safe with getOrDefault in visitAggregation. Add integration tests covering LIMIT OFFSET, aggregate with alias, GROUP BY without bucket nullable, SELECT with alias, derived table subquery, and SELECT without FROM clause. Signed-off-by: Chen Dai <daichen@amazon.com>
1 parent b10d541 commit 498b25f

2 files changed

Lines changed: 128 additions & 5 deletions

File tree

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

Lines changed: 92 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -259,4 +259,96 @@ SELECT department, count(*)
259259
""")
260260
.assertErrorMessage("Encountered");
261261
}
262+
263+
@Test
264+
public void testSqlLimitOffset() {
265+
givenQuery(
266+
"""
267+
SELECT name
268+
FROM catalog.employees
269+
LIMIT 10 OFFSET 5\
270+
""")
271+
.assertPlan(
272+
"""
273+
LogicalProject(name=[$1])
274+
LogicalSort(offset=[5], fetch=[10])
275+
LogicalTableScan(table=[[catalog, employees]])
276+
""");
277+
}
278+
279+
@Test
280+
public void testSqlAggregateWithAlias() {
281+
givenQuery(
282+
"""
283+
SELECT department, COUNT(*) AS cnt
284+
FROM catalog.employees
285+
GROUP BY department\
286+
""")
287+
.assertPlan(
288+
"""
289+
LogicalAggregate(group=[{0}], COUNT(*)=[COUNT()])
290+
LogicalProject(department=[$3])
291+
LogicalTableScan(table=[[catalog, employees]])
292+
""");
293+
}
294+
295+
@Test
296+
public void testSqlGroupByWithoutBucketNullable() {
297+
givenQuery(
298+
"""
299+
SELECT age, COUNT(*) AS cnt
300+
FROM catalog.employees
301+
GROUP BY age\
302+
""")
303+
.assertPlan(
304+
"""
305+
LogicalAggregate(group=[{0}], COUNT(*)=[COUNT()])
306+
LogicalProject(age=[$2])
307+
LogicalTableScan(table=[[catalog, employees]])
308+
""");
309+
}
310+
311+
@Test
312+
public void testSqlSelectWithAlias() {
313+
givenQuery(
314+
"""
315+
SELECT age AS employee_age, name AS employee_name
316+
FROM catalog.employees\
317+
""")
318+
.assertPlan(
319+
"""
320+
LogicalProject(employee_age=[$2], employee_name=[$1])
321+
LogicalTableScan(table=[[catalog, employees]])
322+
""");
323+
}
324+
325+
@Test
326+
public void testSqlDerivedTableInFromClause() {
327+
// SELECT ... FROM (SELECT ...) AS t — exercises visitRelationSubquery override.
328+
givenQuery(
329+
"""
330+
SELECT t.id
331+
FROM (SELECT id, name FROM catalog.employees WHERE age > 30) AS t\
332+
""")
333+
.assertPlan(
334+
"""
335+
LogicalProject(t.id=[$0])
336+
LogicalFilter(condition=[>($2, 30)])
337+
LogicalTableScan(table=[[catalog, employees]])
338+
""");
339+
}
340+
341+
@Test
342+
public void testSqlSelectWithoutFromClause() {
343+
// SELECT 1 — exercises visitValues dual-table case (single empty row).
344+
givenQuery(
345+
"""
346+
SELECT 1\
347+
""")
348+
.assertPlan(
349+
"""
350+
LogicalSort(sort0=[$0], dir0=[ASC])
351+
LogicalValues(tuples=[[]])
352+
""");
353+
}
262354
}

core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java

Lines changed: 36 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -132,6 +132,7 @@
132132
import org.opensearch.sql.ast.tree.Head;
133133
import org.opensearch.sql.ast.tree.Join;
134134
import org.opensearch.sql.ast.tree.Kmeans;
135+
import org.opensearch.sql.ast.tree.Limit;
135136
import org.opensearch.sql.ast.tree.Lookup;
136137
import org.opensearch.sql.ast.tree.Lookup.OutputStrategy;
137138
import org.opensearch.sql.ast.tree.ML;
@@ -146,6 +147,7 @@
146147
import org.opensearch.sql.ast.tree.RareTopN;
147148
import org.opensearch.sql.ast.tree.Regex;
148149
import org.opensearch.sql.ast.tree.Relation;
150+
import org.opensearch.sql.ast.tree.RelationSubquery;
149151
import org.opensearch.sql.ast.tree.Rename;
150152
import org.opensearch.sql.ast.tree.Replace;
151153
import org.opensearch.sql.ast.tree.ReplacePair;
@@ -542,7 +544,18 @@ private List<RexNode> expandProjectFields(
542544
.forEach(field -> expandedFields.add(context.relBuilder.field(field)));
543545
}
544546
case Alias alias -> {
545-
expandedFields.add(rexVisitor.analyze(alias, context));
547+
// SQL aggregate aliases (e.g., COUNT(*) AS cnt): reference the already-computed field
548+
// and rebind under the user's alias, since re-analyzing the alias returns null.
549+
if (alias.getDelegated() instanceof AggregateFunction
550+
&& alias.getName() != null
551+
&& currentFields.contains(alias.getName())) {
552+
String displayName =
553+
Strings.isNullOrEmpty(alias.getAlias()) ? alias.getName() : alias.getAlias();
554+
expandedFields.add(
555+
context.relBuilder.alias(context.relBuilder.field(alias.getName()), displayName));
556+
} else {
557+
expandedFields.add(rexVisitor.analyze(alias, context));
558+
}
546559
}
547560
default ->
548561
throw new IllegalStateException(
@@ -766,6 +779,13 @@ public RelNode visitHead(Head node, CalcitePlanContext context) {
766779
return context.relBuilder.peek();
767780
}
768781

782+
@Override
783+
public RelNode visitLimit(Limit node, CalcitePlanContext context) {
784+
visitChildren(node, context);
785+
context.relBuilder.limit(node.getOffset(), node.getLimit());
786+
return context.relBuilder.peek();
787+
}
788+
769789
/**
770790
* Insert a reversed sort node after finding the original sort in the tree. This rebuilds the tree
771791
* with the reversed sort inserted right after the original sort.
@@ -1624,7 +1644,9 @@ private Pair<List<RexNode>, List<AggCall>> resolveAttributesForAggregation(
16241644
@Override
16251645
public RelNode visitAggregation(Aggregation node, CalcitePlanContext context) {
16261646
Argument.ArgumentMap statsArgs = Argument.ArgumentMap.of(node.getArgExprList());
1627-
Boolean bucketNullable = (Boolean) statsArgs.get(Argument.BUCKET_NULLABLE).getValue();
1647+
// SQL aggregations don't carry the PPL-only BUCKET_NULLABLE argument; default to true.
1648+
boolean bucketNullable =
1649+
(Boolean) statsArgs.getOrDefault(Argument.BUCKET_NULLABLE, Literal.TRUE).getValue();
16281650
int nGroup = node.getGroupExprList().size() + (Objects.nonNull(node.getSpan()) ? 1 : 0);
16291651
BitSet nonNullGroupMask = new BitSet(nGroup);
16301652
if (!bucketNullable) {
@@ -1931,6 +1953,14 @@ public RelNode visitSubqueryAlias(SubqueryAlias node, CalcitePlanContext context
19311953
return context.relBuilder.peek();
19321954
}
19331955

1956+
@Override
1957+
public RelNode visitRelationSubquery(RelationSubquery node, CalcitePlanContext context) {
1958+
// Handle SQL derived tables in FROM clause: SELECT ... FROM (SELECT ...) AS t.
1959+
visitChildren(node, context);
1960+
context.relBuilder.as(node.getAliasAsTableName());
1961+
return context.relBuilder.peek();
1962+
}
1963+
19341964
@Override
19351965
public RelNode visitLookup(Lookup node, CalcitePlanContext context) {
19361966
// 1. resolve source side
@@ -4125,12 +4155,13 @@ public RelNode visitMvExpand(MvExpand mvExpand, CalcitePlanContext context) {
41254155

41264156
@Override
41274157
public RelNode visitValues(Values values, CalcitePlanContext context) {
4128-
if (values.getValues() == null || values.getValues().isEmpty()) {
4158+
// Accept SQL SELECT without FROM (dual table), encoded as Values([[]]) — one row, zero columns.
4159+
List<List<Literal>> rows = values.getValues();
4160+
if (rows == null || rows.isEmpty() || (rows.size() == 1 && rows.get(0).isEmpty())) {
41294161
context.relBuilder.values(context.relBuilder.getTypeFactory().builder().build());
41304162
return context.relBuilder.peek();
4131-
} else {
4132-
throw new CalciteUnsupportedException("Explicit values node is unsupported in Calcite");
41334163
}
4164+
throw new CalciteUnsupportedException("Inline VALUES with literal rows is unsupported");
41344165
}
41354166

41364167
@Override

0 commit comments

Comments
 (0)