Skip to content

Commit ec433f4

Browse files
dai-chendai-chen
authored
fix(calcite): support HAVING, FROM-less SELECT, and window ORDER BY in V2 SQL (#5468)
- HAVING / post-aggregate refs: visitAggregateFunction resolves via a Map<AggregateFunction, Integer> registry on CalcitePlanContext, populated by visitAggregation using position arithmetic. Covers HAVING aggr > N, COUNT(*), alias references, scalar wrappers (ABS(MAX(x))), and arithmetic between aggregates. - 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 semantics. Signed-off-by: Chen Dai <daichen@amazon.com>
1 parent 7b4ebcf commit ec433f4

4 files changed

Lines changed: 178 additions & 4 deletions

File tree

‎api/src/test/java/org/opensearch/sql/api/UnifiedQueryPlannerSqlV2Test.java‎

Lines changed: 133 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -218,4 +218,137 @@ WHERE NOT EXISTS (SELECT 1 FROM catalog.departments WHERE dept_id = age)
218218
LogicalTableScan(table=[[catalog, employees]])
219219
""");
220220
}
221+
222+
@Test
223+
public void selectLiteralWithoutFrom() {
224+
// FROM-less SELECT produces a one-row result via LogicalValues so the downstream
225+
// Project evaluates over a single row.
226+
givenQuery("SELECT 1")
227+
.assertPlan(
228+
"""
229+
LogicalSort(sort0=[$0], dir0=[ASC])
230+
LogicalValues(tuples=[[{ 1 }]])
231+
""");
232+
}
233+
234+
@Test
235+
public void selectExpressionWithoutFrom() {
236+
givenQuery("SELECT 1 + 1")
237+
.assertPlan(
238+
"""
239+
LogicalProject(1 + 1=[+(1, 1)])
240+
LogicalValues(tuples=[[{ 0 }]])
241+
""");
242+
}
243+
244+
@Test
245+
public void testHavingMaxCol() {
246+
givenQuery(
247+
"""
248+
SELECT department FROM catalog.employees
249+
GROUP BY department HAVING MAX(age) > 30
250+
""")
251+
.assertPlan(
252+
"""
253+
LogicalProject(department=[$1])
254+
LogicalFilter(condition=[>($0, 30)])
255+
LogicalProject(MAX(age)=[$1], department=[$0])
256+
LogicalAggregate(group=[{0}], MAX(age)=[MAX($1)])
257+
LogicalProject(department=[$3], age=[$2])
258+
LogicalTableScan(table=[[catalog, employees]])
259+
""");
260+
}
261+
262+
@Test
263+
public void testScalarFnOverAggregate() {
264+
givenQuery("SELECT ABS(MAX(age)) FROM catalog.employees")
265+
.assertPlan(
266+
"""
267+
LogicalProject(ABS(MAX(age))=[ABS($0)])
268+
LogicalAggregate(group=[{}], MAX(age)=[MAX($0)])
269+
LogicalProject(age=[$2])
270+
LogicalTableScan(table=[[catalog, employees]])
271+
""");
272+
}
273+
274+
@Test
275+
public void testArithmeticOnAggregates() {
276+
givenQuery("SELECT MAX(age) + MIN(age) AS range_sum FROM catalog.employees")
277+
.assertPlan(
278+
"""
279+
LogicalProject(range_sum=[+($0, $1)])
280+
LogicalAggregate(group=[{}], MAX(age)=[MAX($0)], MIN(age)=[MIN($0)])
281+
LogicalProject(age=[$2])
282+
LogicalTableScan(table=[[catalog, employees]])
283+
""");
284+
}
285+
286+
@Test
287+
public void testHavingCountStar() {
288+
givenQuery(
289+
"""
290+
SELECT department FROM catalog.employees
291+
GROUP BY department HAVING COUNT(*) > 5
292+
""")
293+
.assertPlan(
294+
"""
295+
LogicalProject(department=[$1])
296+
LogicalFilter(condition=[>($0, 5)])
297+
LogicalProject(COUNT(*)=[$1], department=[$0])
298+
LogicalAggregate(group=[{0}], COUNT(*)=[COUNT()])
299+
LogicalProject(department=[$3])
300+
LogicalTableScan(table=[[catalog, employees]])
301+
""");
302+
}
303+
304+
@Test
305+
public void testHavingWithAlias() {
306+
givenQuery(
307+
"""
308+
SELECT department, COUNT(*) AS cnt FROM catalog.employees
309+
GROUP BY department HAVING cnt > 1
310+
""")
311+
.assertPlan(
312+
"""
313+
LogicalProject(department=[$1], cnt=[$0])
314+
LogicalFilter(condition=[>($0, 1)])
315+
LogicalProject(COUNT(*)=[$1], department=[$0])
316+
LogicalAggregate(group=[{0}], COUNT(*)=[COUNT()])
317+
LogicalProject(department=[$3])
318+
LogicalTableScan(table=[[catalog, employees]])
319+
""");
320+
}
321+
322+
@Test
323+
public void testHavingCompoundAnd() {
324+
givenQuery(
325+
"""
326+
SELECT department FROM catalog.employees
327+
GROUP BY department HAVING MAX(age) > 30 AND MIN(age) < 50
328+
""")
329+
.assertPlan(
330+
"""
331+
LogicalProject(department=[$2])
332+
LogicalFilter(condition=[AND(>($0, 30), <($1, 50))])
333+
LogicalProject(MAX(age)=[$1], MIN(age)=[$2], department=[$0])
334+
LogicalAggregate(group=[{0}], MAX(age)=[MAX($1)], MIN(age)=[MIN($1)])
335+
LogicalProject(department=[$3], age=[$2])
336+
LogicalTableScan(table=[[catalog, employees]])
337+
""");
338+
}
339+
340+
@Test
341+
public void testWindowOrderByDefaultsNullsFirst() {
342+
// Window function ORDER BY without explicit NULLS FIRST/LAST defaults to NULLS FIRST,
343+
// matching top-level ORDER BY semantics.
344+
givenQuery(
345+
"""
346+
SELECT name, ROW_NUMBER() OVER (ORDER BY id) AS rn FROM catalog.employees
347+
""")
348+
.assertPlan(
349+
"""
350+
LogicalProject(name=[$1], rn=[ROW_NUMBER() OVER (ORDER BY $0 NULLS FIRST)])
351+
LogicalTableScan(table=[[catalog, employees]])
352+
""");
353+
}
221354
}

‎core/src/main/java/org/opensearch/sql/calcite/CalcitePlanContext.java‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121
import org.apache.calcite.rex.RexLambdaRef;
2222
import org.apache.calcite.rex.RexNode;
2323
import org.apache.calcite.tools.FrameworkConfig;
24+
import org.opensearch.sql.ast.expression.AggregateFunction;
2425
import org.opensearch.sql.ast.expression.UnresolvedExpression;
2526
import org.opensearch.sql.ast.tree.HighlightConfig;
2627
import org.opensearch.sql.calcite.utils.CalciteToolsHelper;
@@ -64,6 +65,12 @@ public class CalcitePlanContext {
6465

6566
@Getter public Map<String, RexLambdaRef> rexLambdaRefMap;
6667

68+
/**
69+
* Maps AggregateFunction AST nodes to their output field index for HAVING/post-aggregate
70+
* resolution.
71+
*/
72+
@Getter private final Map<AggregateFunction, Integer> aggregateOutputIndex = new HashMap<>();
73+
6774
/**
6875
* List of captured variables from outer scope for lambda functions. When a lambda body references
6976
* a field that is not a lambda parameter, it gets captured and stored here. The captured

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

Lines changed: 24 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1748,6 +1748,23 @@ private void visitAggregation(
17481748
reordered.addAll(aggRexList);
17491749
}
17501750
context.relBuilder.project(reordered);
1751+
1752+
// Register aggregate output indices for HAVING / post-aggregate resolution. clear() is safe:
1753+
// V2 grammar doesn't allow subqueries that nest aggregation scopes above this point.
1754+
context.getAggregateOutputIndex().clear();
1755+
int aggStartIdx = metricsFirst ? 0 : aliasedGroupByList.size();
1756+
for (int i = 0; i < aggExprList.size(); i++) {
1757+
AggregateFunction aggFunc = extractAggregateFunction(aggExprList.get(i));
1758+
if (aggFunc != null) {
1759+
context.getAggregateOutputIndex().put(aggFunc, aggStartIdx + i);
1760+
}
1761+
}
1762+
}
1763+
1764+
private static AggregateFunction extractAggregateFunction(UnresolvedExpression expr) {
1765+
if (expr instanceof AggregateFunction af) return af;
1766+
if (expr instanceof Alias alias) return extractAggregateFunction(alias.getDelegated());
1767+
return null;
17511768
}
17521769

17531770
private Optional<UnresolvedExpression> getTimeSpanField(UnresolvedExpression expr) {
@@ -4181,12 +4198,17 @@ public RelNode visitMvExpand(MvExpand mvExpand, CalcitePlanContext context) {
41814198

41824199
@Override
41834200
public RelNode visitValues(Values values, CalcitePlanContext context) {
4184-
// Accept SQL SELECT without FROM (dual table), encoded as Values([[]]) — one row, zero columns.
41854201
List<List<Literal>> rows = values.getValues();
4186-
if (rows == null || rows.isEmpty() || (rows.size() == 1 && rows.get(0).isEmpty())) {
4202+
if (rows == null || rows.isEmpty()) {
4203+
// PPL empty subsearch (e.g., `... | append [ ]`): zero rows, no columns.
41874204
context.relBuilder.values(context.relBuilder.getTypeFactory().builder().build());
41884205
return context.relBuilder.peek();
41894206
}
4207+
if (rows.size() == 1 && rows.get(0).isEmpty()) {
4208+
// SQL FROM-less SELECT (dual table) encoded as Values([[]]): one-row relation for Project.
4209+
context.relBuilder.push(LogicalValues.createOneRow(context.relBuilder.getCluster()));
4210+
return context.relBuilder.peek();
4211+
}
41904212
throw new CalciteUnsupportedException("Inline VALUES with literal rows is unsupported");
41914213
}
41924214

‎core/src/main/java/org/opensearch/sql/calcite/CalciteRexNodeVisitor.java‎

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -107,6 +107,17 @@ public RexNode analyzeJoinCondition(UnresolvedExpression unresolved, CalcitePlan
107107
return context.resolveJoinCondition(unresolved, this::analyze);
108108
}
109109

110+
@Override
111+
public RexNode visitAggregateFunction(AggregateFunction node, CalcitePlanContext context) {
112+
// Resolve post-aggregate AggregateFunction via registry populated in visitAggregation.
113+
Integer index = context.getAggregateOutputIndex().get(node);
114+
if (index == null) {
115+
throw new IllegalStateException(
116+
"Aggregate function " + node + " not registered (planner bug)");
117+
}
118+
return context.relBuilder.field(index);
119+
}
120+
110121
@Override
111122
public RexNode visitLiteral(Literal node, CalcitePlanContext context) {
112123
RexBuilder rexBuilder = context.rexBuilder;
@@ -652,9 +663,10 @@ private List<RexNode> translateOrderKeys(
652663
field = b.desc(field);
653664
}
654665
return switch (opt.getNullOrder()) {
655-
case NULL_LAST -> b.nullsLast(field);
666+
// Unspecified NULLS defaults to NULLS FIRST, matching top-level ORDER BY.
667+
case null -> b.nullsFirst(field);
656668
case NULL_FIRST -> b.nullsFirst(field);
657-
default -> field;
669+
case NULL_LAST -> b.nullsLast(field);
658670
};
659671
})
660672
.toList();

0 commit comments

Comments
 (0)