Skip to content

Commit 17ee6d3

Browse files
committed
Restrict ROW_NUMBER tie-break to comparable columns + sync no-pushdown fixtures
Three follow-ups to the rare/top tie-break commit, surfaced by CI: 1. **Skip non-comparable tie-break columns.** `top address` on a nested-array field reached the in-process `EnumerableWindow` with `ORDER BY count DESC, address` and crashed at runtime — `class java.util.ArrayList cannot be cast to class java.lang.Comparable`. Filter array / multiset / map / struct (row) columns out of the tie-break list before appending. If all field columns are non-comparable, no tie-break is added (falling back to the original count-only ORDER BY). 2. **Sync the parallel `calcite_no_pushdown/` explain fixtures.** `PPLIntegTestCase.loadExpectedPlan` switches to `expectedOutput/calcite_no_pushdown/` when pushdown is disabled, which `CalciteNoPushdownIT` exercises. The previous commit only updated the `expectedOutput/calcite/` copies, so `CalciteNoPushdownIT > CalciteExplainIT` read stale (no-tie-break) YAML. Update the four `explain_{rare,top}_usenull_*` YAMLs under `calcite_no_pushdown/` to include the `, $N` tie-break key in both the logical plan and the `EnumerableWindow` collation. 3. **Collapse the pushdown-disabled branch in `RareCommandIT.testRareWithGroup`.** With the stable tie-break, both pushdown-enabled and pushdown-disabled paths now resolve the count=8 tie deterministically to `AR` (alphabetical). Drop the `isPushdownDisabled() ? rows("F", "NV", 8) : rows("F", "AR", 8)` conditional in favor of the single `AR` expectation. Signed-off-by: Kai Huang <huangkaics@gmail.com>
1 parent 1b19dd3 commit 17ee6d3

6 files changed

Lines changed: 25 additions & 10 deletions

File tree

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

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3044,7 +3044,13 @@ public RelNode visitRareTopN(RareTopN node, CalcitePlanContext context) {
30443044
// Append the rare/top field columns as secondary order keys so ties in the count column
30453045
// resolve deterministically. Without this, ROW_NUMBER's tie-break is insertion-order
30463046
// dependent and varies between backends (e.g. analytics-engine vs in-process Calcite).
3047-
List<RexNode> tieBreakKeys = rexVisitor.analyze(fieldList, context);
3047+
// Skip array / struct / map columns — Calcite's executor can't compare those at runtime
3048+
// (`ArrayList cannot be cast to Comparable`), and pushdown to the OpenSearch terms
3049+
// aggregation only supports scalar keys.
3050+
List<RexNode> tieBreakKeys =
3051+
rexVisitor.analyze(fieldList, context).stream()
3052+
.filter(CalciteRelNodeVisitor::isComparableOrderKey)
3053+
.toList();
30483054
List<RexNode> orderKeys = new ArrayList<>(tieBreakKeys.size() + 1);
30493055
orderKeys.add(countField);
30503056
orderKeys.addAll(tieBreakKeys);
@@ -3080,6 +3086,15 @@ public RelNode visitRareTopN(RareTopN node, CalcitePlanContext context) {
30803086
return context.relBuilder.peek();
30813087
}
30823088

3089+
private static boolean isComparableOrderKey(RexNode key) {
3090+
SqlTypeName typeName = key.getType().getSqlTypeName();
3091+
return typeName != SqlTypeName.ARRAY
3092+
&& typeName != SqlTypeName.MULTISET
3093+
&& typeName != SqlTypeName.MAP
3094+
&& typeName != SqlTypeName.ROW
3095+
&& typeName != SqlTypeName.STRUCTURED;
3096+
}
3097+
30833098
@Override
30843099
public RelNode visitTableFunction(TableFunction node, CalcitePlanContext context) {
30853100
throw new CalciteUnsupportedException("Table function is unsupported in Calcite");

integ-test/src/test/java/org/opensearch/sql/ppl/RareCommandIT.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,7 @@ public void testRareWithGroup() throws IOException {
5959
rows("F", "OK", 7),
6060
rows("F", "KS", 7),
6161
rows("F", "CO", 7),
62-
isPushdownDisabled() ? rows("F", "NV", 8) : rows("F", "AR", 8),
62+
rows("F", "AR", 8),
6363
rows("M", "NE", 5),
6464
rows("M", "RI", 5),
6565
rows("M", "NV", 5),

integ-test/src/test/resources/expectedOutput/calcite_no_pushdown/explain_rare_usenull_false.yaml

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,15 +3,15 @@ calcite:
33
LogicalSystemLimit(fetch=[10000], type=[QUERY_SIZE_LIMIT])
44
LogicalProject(gender=[$0], state=[$1], count=[$2])
55
LogicalFilter(condition=[<=($3, 2)])
6-
LogicalProject(gender=[$0], state=[$1], count=[$2], _row_number_rare_top_=[ROW_NUMBER() OVER (PARTITION BY $0 ORDER BY $2)])
6+
LogicalProject(gender=[$0], state=[$1], count=[$2], _row_number_rare_top_=[ROW_NUMBER() OVER (PARTITION BY $0 ORDER BY $2, $1)])
77
LogicalAggregate(group=[{0, 1}], count=[COUNT()])
88
LogicalProject(gender=[$4], state=[$7])
99
LogicalFilter(condition=[AND(IS NOT NULL($4), IS NOT NULL($7))])
1010
CalciteLogicalIndexScan(table=[[OpenSearch, opensearch-sql_test_index_account]])
1111
physical: |
1212
EnumerableLimit(fetch=[10000])
1313
EnumerableCalc(expr#0..3=[{inputs}], expr#4=[2], expr#5=[<=($t3, $t4)], proj#0..2=[{exprs}], $condition=[$t5])
14-
EnumerableWindow(window#0=[window(partition {0} order by [2] rows between UNBOUNDED PRECEDING and CURRENT ROW aggs [ROW_NUMBER()])])
14+
EnumerableWindow(window#0=[window(partition {0} order by [2, 1] rows between UNBOUNDED PRECEDING and CURRENT ROW aggs [ROW_NUMBER()])])
1515
EnumerableAggregate(group=[{4, 7}], count=[COUNT()])
1616
EnumerableCalc(expr#0..16=[{inputs}], expr#17=[IS NOT NULL($t4)], expr#18=[IS NOT NULL($t7)], expr#19=[AND($t17, $t18)], proj#0..16=[{exprs}], $condition=[$t19])
1717
CalciteEnumerableIndexScan(table=[[OpenSearch, opensearch-sql_test_index_account]])

integ-test/src/test/resources/expectedOutput/calcite_no_pushdown/explain_rare_usenull_true.yaml

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,13 +3,13 @@ calcite:
33
LogicalSystemLimit(fetch=[10000], type=[QUERY_SIZE_LIMIT])
44
LogicalProject(gender=[$0], state=[$1], count=[$2])
55
LogicalFilter(condition=[<=($3, 2)])
6-
LogicalProject(gender=[$0], state=[$1], count=[$2], _row_number_rare_top_=[ROW_NUMBER() OVER (PARTITION BY $0 ORDER BY $2)])
6+
LogicalProject(gender=[$0], state=[$1], count=[$2], _row_number_rare_top_=[ROW_NUMBER() OVER (PARTITION BY $0 ORDER BY $2, $1)])
77
LogicalAggregate(group=[{0, 1}], count=[COUNT()])
88
LogicalProject(gender=[$4], state=[$7])
99
CalciteLogicalIndexScan(table=[[OpenSearch, opensearch-sql_test_index_account]])
1010
physical: |
1111
EnumerableLimit(fetch=[10000])
1212
EnumerableCalc(expr#0..3=[{inputs}], expr#4=[2], expr#5=[<=($t3, $t4)], proj#0..2=[{exprs}], $condition=[$t5])
13-
EnumerableWindow(window#0=[window(partition {0} order by [2] rows between UNBOUNDED PRECEDING and CURRENT ROW aggs [ROW_NUMBER()])])
13+
EnumerableWindow(window#0=[window(partition {0} order by [2, 1] rows between UNBOUNDED PRECEDING and CURRENT ROW aggs [ROW_NUMBER()])])
1414
EnumerableAggregate(group=[{4, 7}], count=[COUNT()])
1515
CalciteEnumerableIndexScan(table=[[OpenSearch, opensearch-sql_test_index_account]])

integ-test/src/test/resources/expectedOutput/calcite_no_pushdown/explain_top_usenull_false.yaml

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,15 +3,15 @@ calcite:
33
LogicalSystemLimit(fetch=[10000], type=[QUERY_SIZE_LIMIT])
44
LogicalProject(gender=[$0], state=[$1], count=[$2])
55
LogicalFilter(condition=[<=($3, 2)])
6-
LogicalProject(gender=[$0], state=[$1], count=[$2], _row_number_rare_top_=[ROW_NUMBER() OVER (PARTITION BY $0 ORDER BY $2 DESC)])
6+
LogicalProject(gender=[$0], state=[$1], count=[$2], _row_number_rare_top_=[ROW_NUMBER() OVER (PARTITION BY $0 ORDER BY $2 DESC, $1)])
77
LogicalAggregate(group=[{0, 1}], count=[COUNT()])
88
LogicalProject(gender=[$4], state=[$7])
99
LogicalFilter(condition=[AND(IS NOT NULL($4), IS NOT NULL($7))])
1010
CalciteLogicalIndexScan(table=[[OpenSearch, opensearch-sql_test_index_account]])
1111
physical: |
1212
EnumerableLimit(fetch=[10000])
1313
EnumerableCalc(expr#0..3=[{inputs}], expr#4=[2], expr#5=[<=($t3, $t4)], proj#0..2=[{exprs}], $condition=[$t5])
14-
EnumerableWindow(window#0=[window(partition {0} order by [2 DESC] rows between UNBOUNDED PRECEDING and CURRENT ROW aggs [ROW_NUMBER()])])
14+
EnumerableWindow(window#0=[window(partition {0} order by [2 DESC, 1] rows between UNBOUNDED PRECEDING and CURRENT ROW aggs [ROW_NUMBER()])])
1515
EnumerableAggregate(group=[{4, 7}], count=[COUNT()])
1616
EnumerableCalc(expr#0..16=[{inputs}], expr#17=[IS NOT NULL($t4)], expr#18=[IS NOT NULL($t7)], expr#19=[AND($t17, $t18)], proj#0..16=[{exprs}], $condition=[$t19])
1717
CalciteEnumerableIndexScan(table=[[OpenSearch, opensearch-sql_test_index_account]])

integ-test/src/test/resources/expectedOutput/calcite_no_pushdown/explain_top_usenull_true.yaml

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,13 +3,13 @@ calcite:
33
LogicalSystemLimit(fetch=[10000], type=[QUERY_SIZE_LIMIT])
44
LogicalProject(gender=[$0], state=[$1], count=[$2])
55
LogicalFilter(condition=[<=($3, 2)])
6-
LogicalProject(gender=[$0], state=[$1], count=[$2], _row_number_rare_top_=[ROW_NUMBER() OVER (PARTITION BY $0 ORDER BY $2 DESC)])
6+
LogicalProject(gender=[$0], state=[$1], count=[$2], _row_number_rare_top_=[ROW_NUMBER() OVER (PARTITION BY $0 ORDER BY $2 DESC, $1)])
77
LogicalAggregate(group=[{0, 1}], count=[COUNT()])
88
LogicalProject(gender=[$4], state=[$7])
99
CalciteLogicalIndexScan(table=[[OpenSearch, opensearch-sql_test_index_account]])
1010
physical: |
1111
EnumerableLimit(fetch=[10000])
1212
EnumerableCalc(expr#0..3=[{inputs}], expr#4=[2], expr#5=[<=($t3, $t4)], proj#0..2=[{exprs}], $condition=[$t5])
13-
EnumerableWindow(window#0=[window(partition {0} order by [2 DESC] rows between UNBOUNDED PRECEDING and CURRENT ROW aggs [ROW_NUMBER()])])
13+
EnumerableWindow(window#0=[window(partition {0} order by [2 DESC, 1] rows between UNBOUNDED PRECEDING and CURRENT ROW aggs [ROW_NUMBER()])])
1414
EnumerableAggregate(group=[{4, 7}], count=[COUNT()])
1515
CalciteEnumerableIndexScan(table=[[OpenSearch, opensearch-sql_test_index_account]])

0 commit comments

Comments
 (0)