Skip to content

Commit 820da3e

Browse files
committed
fix(calcite): Default SQL aggregate window frame to running window
Aggregate window functions with ORDER BY but no explicit frame defaulted to the whole partition, so COUNT(DISTINCT x) OVER(ORDER BY y) returned the total for every row instead of a running count. Default to ROWS UNBOUNDED PRECEDING .. CURRENT ROW for aggregates in the SQL parser (ranking functions ignore frames), and add equals/hashCode to CurrentRowWindowBound. PPL windows carry no ORDER BY, so are unaffected. Signed-off-by: Chen Dai <daichen@amazon.com>
1 parent cf14aba commit 820da3e

4 files changed

Lines changed: 55 additions & 5 deletions

File tree

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

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -424,6 +424,32 @@ GROUP BY department HAVING MAX(age) > 30 AND MIN(age) < 50
424424
""");
425425
}
426426

427+
@Test
428+
public void testCountDistinctWindowWithOrderBy() {
429+
givenQuery(
430+
"""
431+
SELECT department, COUNT(DISTINCT name) OVER(ORDER BY department) FROM catalog.employees
432+
""")
433+
.assertPlan(
434+
"""
435+
LogicalProject(department=[$3], COUNT(DISTINCT name) OVER(ORDER BY department)=[COUNT(DISTINCT $1) OVER (ORDER BY $3 NULLS FIRST ROWS UNBOUNDED PRECEDING)])
436+
LogicalTableScan(table=[[catalog, employees]])
437+
""");
438+
}
439+
440+
@Test
441+
public void testSumWindowWithPartitionAndOrderBy() {
442+
givenQuery(
443+
"""
444+
SELECT name, SUM(age) OVER(PARTITION BY department ORDER BY age) FROM catalog.employees
445+
""")
446+
.assertPlan(
447+
"""
448+
LogicalProject(name=[$1], SUM(age) OVER(PARTITION BY department ORDER BY age)=[SUM($2) OVER (PARTITION BY $3 ORDER BY $2 NULLS FIRST ROWS UNBOUNDED PRECEDING)])
449+
LogicalTableScan(table=[[catalog, employees]])
450+
""");
451+
}
452+
427453
@Test
428454
public void testWindowOrderByDefaultsNullsFirst() {
429455
// Window function ORDER BY without explicit NULLS FIRST/LAST defaults to NULLS FIRST,

core/src/main/java/org/opensearch/sql/ast/expression/WindowBound.java

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,16 @@ public boolean isPreceding() {
2828
public static class CurrentRowWindowBound extends WindowBound {
2929
CurrentRowWindowBound() {}
3030

31+
@Override
32+
public boolean equals(Object o) {
33+
return this == o || o instanceof CurrentRowWindowBound;
34+
}
35+
36+
@Override
37+
public int hashCode() {
38+
return CurrentRowWindowBound.class.hashCode();
39+
}
40+
3141
@Override
3242
public String toString() {
3343
return "CURRENT ROW";

sql/src/main/java/org/opensearch/sql/sql/parser/AstExpressionBuilder.java

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -222,7 +222,16 @@ public UnresolvedExpression visitWindowFunctionClause(WindowFunctionClauseContex
222222
.map(item -> ImmutablePair.of(createSortOption(item), visit(item.expression())))
223223
.collect(Collectors.toList());
224224
}
225-
return new WindowFunction(visit(ctx.function), partitionByList, sortList);
225+
UnresolvedExpression function = visit(ctx.function);
226+
WindowFunction windowFunction = new WindowFunction(function, partitionByList, sortList);
227+
// SQL standard: an aggregate window function with ORDER BY but no explicit frame defaults to a
228+
// running window (ROWS UNBOUNDED PRECEDING AND CURRENT ROW), not the whole partition. The
229+
// grammar exposes no frame clause and the AST default is whole-partition (added for PPL), so
230+
// apply the running default here. Ranking functions (RANK/DENSE_RANK/ROW_NUMBER) ignore frames.
231+
if (function instanceof AggregateFunction && !sortList.isEmpty()) {
232+
windowFunction.setWindowFrame(WindowFrame.toCurrentRow());
233+
}
234+
return windowFunction;
226235
}
227236

228237
@Override

sql/src/test/java/org/opensearch/sql/sql/parser/AstExpressionBuilderTest.java

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,8 @@
4444
import org.opensearch.sql.ast.expression.DataType;
4545
import org.opensearch.sql.ast.expression.Literal;
4646
import org.opensearch.sql.ast.expression.RelevanceFieldList;
47+
import org.opensearch.sql.ast.expression.WindowFrame;
48+
import org.opensearch.sql.ast.expression.WindowFunction;
4749
import org.opensearch.sql.ast.tree.Sort.SortOption;
4850
import org.opensearch.sql.common.antlr.CaseInsensitiveCharStream;
4951
import org.opensearch.sql.common.antlr.SyntaxAnalysisErrorListener;
@@ -308,12 +310,15 @@ public void canBuildWindowFunctionWithoutOrderBy() {
308310

309311
@Test
310312
public void canBuildAggregateWindowFunction() {
311-
assertEquals(
312-
window(
313+
WindowFunction expected =
314+
new WindowFunction(
313315
aggregate("AVG", qualifiedName("age")),
314316
ImmutableList.of(qualifiedName("state")),
315-
ImmutableList.of(ImmutablePair.of(new SortOption(null, null), qualifiedName("age")))),
316-
buildExprAst("AVG(age) OVER (PARTITION BY state ORDER BY age)"));
317+
ImmutableList.of(ImmutablePair.of(new SortOption(null, null), qualifiedName("age"))));
318+
// Aggregate window with ORDER BY defaults to a running frame (UNBOUNDED PRECEDING .. CURRENT
319+
// ROW) rather than the whole partition.
320+
expected.setWindowFrame(WindowFrame.toCurrentRow());
321+
assertEquals(expected, buildExprAst("AVG(age) OVER (PARTITION BY state ORDER BY age)"));
317322
}
318323

319324
@Test

0 commit comments

Comments
 (0)