mihaibudiu commented on code in PR #5293:
URL: https://github.com/apache/calcite/pull/5293#discussion_r4169847667
##########
core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java:
##########
@@ -2623,26 +2623,30 @@ private RexNode convertOver(Blackboard bb, SqlNode
node) {
bb.convertExpression(requireNonNull(sqlLowerBound, "sqlLowerBound"));
final RexNode upperBound =
bb.convertExpression(requireNonNull(sqlUpperBound, "sqlUpperBound"));
- if (orderList.isEmpty() && !rows) {
- // A logical range requires an ORDER BY clause. Use the implicit
- // ordering of this relation. There must be one, otherwise it would
- // have failed validation.
- orderList = bb.scope.getOrderList();
- if (orderList == null) {
- throw new AssertionError(
- "Relation should have sort key for implicit ORDER BY");
- }
- }
final RexWindowExclusion exclude =
RexWindowExclusion.create(window.getExclude());
final ImmutableList.Builder<RexNode> orderKeys =
ImmutableList.builder();
- for (SqlNode order : orderList) {
- orderKeys.add(
- bb.convertSortExpression(order,
- RelFieldCollation.Direction.ASCENDING,
- RelFieldCollation.NullDirection.UNSPECIFIED,
- bb::sortToRex));
+ 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
Review Comment:
or a column that one has sorted on in a subquery
##########
core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java:
##########
@@ -2674,6 +2678,89 @@ private RexNode convertOver(Blackboard bb, SqlNode node)
{
}
}
+ /**
+ * 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).
Review Comment:
in general you should not refer to issues in javadoc comments that are not
test code.
##########
core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java:
##########
@@ -2623,26 +2623,30 @@ private RexNode convertOver(Blackboard bb, SqlNode
node) {
bb.convertExpression(requireNonNull(sqlLowerBound, "sqlLowerBound"));
final RexNode upperBound =
bb.convertExpression(requireNonNull(sqlUpperBound, "sqlUpperBound"));
- if (orderList.isEmpty() && !rows) {
- // A logical range requires an ORDER BY clause. Use the implicit
- // ordering of this relation. There must be one, otherwise it would
- // have failed validation.
- orderList = bb.scope.getOrderList();
- if (orderList == null) {
- throw new AssertionError(
- "Relation should have sort key for implicit ORDER BY");
- }
- }
final RexWindowExclusion exclude =
RexWindowExclusion.create(window.getExclude());
final ImmutableList.Builder<RexNode> orderKeys =
ImmutableList.builder();
- for (SqlNode order : orderList) {
- orderKeys.add(
- bb.convertSortExpression(order,
- RelFieldCollation.Direction.ASCENDING,
- RelFieldCollation.NullDirection.UNSPECIFIED,
- bb::sortToRex));
+ 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
+ // 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) {
Review 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.
##########
core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java:
##########
@@ -2623,26 +2623,30 @@ private RexNode convertOver(Blackboard bb, SqlNode
node) {
bb.convertExpression(requireNonNull(sqlLowerBound, "sqlLowerBound"));
final RexNode upperBound =
bb.convertExpression(requireNonNull(sqlUpperBound, "sqlUpperBound"));
- if (orderList.isEmpty() && !rows) {
- // A logical range requires an ORDER BY clause. Use the implicit
- // ordering of this relation. There must be one, otherwise it would
- // have failed validation.
- orderList = bb.scope.getOrderList();
- if (orderList == null) {
- throw new AssertionError(
- "Relation should have sort key for implicit ORDER BY");
- }
- }
final RexWindowExclusion exclude =
RexWindowExclusion.create(window.getExclude());
final ImmutableList.Builder<RexNode> orderKeys =
ImmutableList.builder();
- for (SqlNode order : orderList) {
- orderKeys.add(
- bb.convertSortExpression(order,
- RelFieldCollation.Direction.ASCENDING,
- RelFieldCollation.NullDirection.UNSPECIFIED,
- bb::sortToRex));
+ 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
+ // may be gone. Its old ordinal then names a different field, and
+ // Project.isValid fails (CALCITE-7822).
Review Comment:
remove reference to issue
##########
core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java:
##########
@@ -2623,26 +2623,30 @@ private RexNode convertOver(Blackboard bb, SqlNode
node) {
bb.convertExpression(requireNonNull(sqlLowerBound, "sqlLowerBound"));
final RexNode upperBound =
bb.convertExpression(requireNonNull(sqlUpperBound, "sqlUpperBound"));
- if (orderList.isEmpty() && !rows) {
- // A logical range requires an ORDER BY clause. Use the implicit
- // ordering of this relation. There must be one, otherwise it would
- // have failed validation.
- orderList = bb.scope.getOrderList();
- if (orderList == null) {
- throw new AssertionError(
- "Relation should have sort key for implicit ORDER BY");
- }
- }
final RexWindowExclusion exclude =
RexWindowExclusion.create(window.getExclude());
final ImmutableList.Builder<RexNode> orderKeys =
ImmutableList.builder();
- for (SqlNode order : orderList) {
- orderKeys.add(
- bb.convertSortExpression(order,
- RelFieldCollation.Direction.ASCENDING,
- RelFieldCollation.NullDirection.UNSPECIFIED,
- bb::sortToRex));
+ if (orderList.isEmpty() && !rows) {
+ // RANGE with no ORDER BY still needs a logical order. That order is
Review Comment:
still -> always
##########
core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java:
##########
@@ -2623,26 +2623,30 @@ private RexNode convertOver(Blackboard bb, SqlNode
node) {
bb.convertExpression(requireNonNull(sqlLowerBound, "sqlLowerBound"));
final RexNode upperBound =
bb.convertExpression(requireNonNull(sqlUpperBound, "sqlUpperBound"));
- if (orderList.isEmpty() && !rows) {
- // A logical range requires an ORDER BY clause. Use the implicit
- // ordering of this relation. There must be one, otherwise it would
- // have failed validation.
- orderList = bb.scope.getOrderList();
- if (orderList == null) {
- throw new AssertionError(
- "Relation should have sort key for implicit ORDER BY");
- }
- }
final RexWindowExclusion exclude =
RexWindowExclusion.create(window.getExclude());
final ImmutableList.Builder<RexNode> orderKeys =
ImmutableList.builder();
- for (SqlNode order : orderList) {
- orderKeys.add(
- bb.convertSortExpression(order,
- RelFieldCollation.Direction.ASCENDING,
- RelFieldCollation.NullDirection.UNSPECIFIED,
- bb::sortToRex));
+ 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
Review Comment:
do not refer to a column name in compiler code. empno is test data, the
compiled does not depend on test data.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
