Repository navigation
[CALCITE-7822] AssertionError "type mismatch" in SqlToRelConverter for OVER () in a GROUP BY query - #5293
[CALCITE-7822] AssertionError "type mismatch" in SqlToRelConverter for OVER () in a GROUP BY query#5293kalayciburak wants to merge 3 commits into
Conversation
What is a monotonic column?
I am not sure I understand this |
| * <a href="https://issues.apache.org/jira/browse/CALCITE-7822">[CALCITE-7822] | ||
| * AssertionError "type mismatch" in SqlToRelConverter for OVER () in a | ||
| * GROUP BY query</a>. */ | ||
| @Test void testEmptyOverInAggregateQuery() { |
There was a problem hiding this comment.
would these queries work as quidem tests in an .iq file?
These tests are much more readable
There was a problem hiding this comment.
yep, they do. moved them to winagg.iq and dropped the java assertions.
| } | ||
|
|
||
| /** Rebinds a simple column order key onto the current input, or null. */ | ||
| private @Nullable RexNode rebindImplicitOrderKey(SqlNameMatcher nameMatcher, |
There was a problem hiding this comment.
what does "rebind" mean?
There was a problem hiding this comment.
it meant look the column up by name on the current row, if GROUP BY didn't remove it. renamed the helper so it says that.
| } | ||
| } | ||
|
|
||
| /** Whether every input ref in {@code orderKey} matches the current input. */ |
There was a problem hiding this comment.
in what sense does it match the input?
There was a problem hiding this comment.
the ref index is in range, and its type equals the field at that index (nullability aside). if the types differ, the ordinal now names a different column.
|
monotonic column is a column the table marks as sorted. on scott.EMP that is EMPNO. a RANGE window with no ORDER BY uses it as the logical order. after GROUP BY deptno, EMPNO is gone. the old code still built a ref from EMPNO's old ordinal (0), which is now DEPTNO, so the types don't match (SMALLINT vs TINYINT). that's the assertion. rewrote that prose in the description, and moved the queries to winagg.iq. |
| RelFieldCollation.NullDirection.UNSPECIFIED, | ||
| bb::sortToRex)); | ||
| if (orderList.isEmpty() && !rows) { | ||
| // RANGE with no ORDER BY still needs a logical order. That order is |
| if (orderList.isEmpty() && !rows) { | ||
| // RANGE with no ORDER BY still needs a logical order. That order is | ||
| // the first monotonic column of the FROM item (a column the table | ||
| // declares as sorted, for example EMPNO). After GROUP BY that column |
There was a problem hiding this comment.
or a column that one has sorted on in a subquery
| * Adds RANGE order keys taken from the FROM item when the window has no | ||
| * ORDER BY. Keeps a key whose input refs still name the same fields. | ||
| * Otherwise looks the column up by name on the current input, or drops | ||
| * it if GROUP BY removed it (CALCITE-7822). |
There was a problem hiding this comment.
in general you should not refer to issues in javadoc comments that are not test code.
| if (orderList.isEmpty() && !rows) { | ||
| // RANGE with no ORDER BY still needs a logical order. That order is | ||
| // the first monotonic column of the FROM item (a column the table | ||
| // declares as sorted, for example EMPNO). After GROUP BY that column |
There was a problem hiding this comment.
do not refer to a column name in compiler code. empno is test data, the compiled does not depend on test data.
| // the first monotonic column of the FROM item (a column the table | ||
| // declares as sorted, for example EMPNO). After GROUP BY that column | ||
| // may be gone. Its old ordinal then names a different field, and | ||
| // Project.isValid fails (CALCITE-7822). |
There was a problem hiding this comment.
remove reference to issue
| // may be gone. Its old ordinal then names a different field, and | ||
| // Project.isValid fails (CALCITE-7822). | ||
| final SqlNodeList implicit = bb.scope.getOrderList(); | ||
| if (implicit == null) { |
There was a problem hiding this comment.
I think this is a completely separate issue, which has nothing to do with the bug you are solving.
This is not about implicit sorting.
Can we focus on solving the issue even if the table does not have implicit sorting? This query should work for that case.
There was a problem hiding this comment.
yeah, this shouldn't be about preserving the implicit sort. dropped the name lookup. if the refs don't match the current input the key is left off, so over () works with or without a monotonic column. added the no-monotonic case to winagg.iq.
also cleaned the comments, no test column name and no issue id in the compiler notes.
| RelFieldCollation.Direction.ASCENDING, | ||
| RelFieldCollation.NullDirection.UNSPECIFIED, | ||
| bb::sortToRex)); | ||
| if (orderList.isEmpty() && !rows) { |
There was a problem hiding this comment.
I thought we agreed to leave this part unchanged.
The fix should have nothing to do with the implicit order.
…r OVER () in a GROUP BY query A RANGE window without ORDER BY used the FROM item's monotonic column as an implicit sort key. After GROUP BY that column is no longer an input, but its original ordinal still became a RexInputRef, so Project.isValid failed (SMALLINT NOT NULL vs TINYINT). Keep the converted key when its input refs match the current input. Otherwise rebind a simple column by name, or drop the key if GROUP BY removed it. Unbounded RANGE already allows an empty order list.
The Java assertions were hard to read. Same queries now live in winagg.iq. Dropped the word rebind: a converted order key is kept when its refs still name the same fields, otherwise the column is looked up by name.
A window with no ORDER BY should run even when the FROM item has no monotonic column. Keep an implicit key only when its input refs still name the same fields. Do not rebuild it by column name.
346561b to
f11e99c
Compare
|



Jira Link
CALCITE-7822
Changes Proposed
A RANGE window with no ORDER BY does not need a sort key. Every row is a peer, so an empty order list is valid. That is true whether or not the FROM item has a monotonic column.
If the FROM item does have one (a column declared sorted, or a column sorted on in a subquery), the key is kept only when its input refs still name the same fields. After GROUP BY that column may be gone. The old code still built a RexInputRef from the old ordinal, so the ref type did not match the aggregate field and Project.isValid failed. The key is dropped in that case. It is not rebuilt by column name.
Tests
The queries are in
core/src/test/resources/sql/winagg.iq:select deptno, count(*) over () as c from emp group by deptnoreturns deptno 10, 20 and 30, each with c = 3select deptno, sal from emp)select job, max(max(sal)) over () from emp group by jobRan
sql/winagg.iqthroughCoreQuidemTest(JAVA_HOME=21). Passed.:core:checkstyleMainpassed.