gianm commented on code in PR #5349:
URL: https://github.com/apache/calcite/pull/5349#discussion_r4406904972
##########
core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java:
##########
@@ -648,12 +653,9 @@ public RelRoot convertQuery(
result = new LogicalDelta(cluster, result.getTraitSet(), result);
}
}
- RelCollation collation = RelCollations.EMPTY;
- if (!query.isA(SqlKind.DML)) {
- if (isOrdered(query)) {
- collation = requiredCollation(result);
- }
- }
+ final RelCollation collation = query.isA(SqlKind.DML)
Review Comment:
The change to use `root.collation` for pipe calls is related to this PR (it
helps with preserving order through a pipeline). The new guard
`removeSortInSubQuery(top)` isn't strictly related, it fixes a pre-existing bug
where queries like this one throw `AssertionError`:
```
select * from emp
where deptno in (
with w as (select * from dept) select deptno from w order by deptno)
```
There's a test for that in this PR in
`SqlToRelConverterTest#testWithInsideWhereInOrderBy`. I can remove it if the
preference is keep such a fix out of this PR.
##########
core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java:
##########
@@ -2861,10 +2897,31 @@ protected void convertFrom(
case INTERSECT:
case EXCEPT:
case UNION:
+ case FROM_QUERY:
+ case PIPE_OPERATOR:
final RelNode rel = convertQueryRecursive(from, false, null).project();
bb.setRoot(rel, true);
return;
+ case PIPE_INPUT:
+ // The input's order matters only if the operator is at the top of the
+ // statement. Then convert the input as top, so that its sort is kept.
Review Comment:
It means to set the `top` flag during conversion, which causes the converter
to retain `ORDER BY`. I reworded the comment to try to be clearer:
```
// If the "bb.top" flag is set, it means we are converting a pipe
// operator call at the top of the statement; therefore, we need to
// also convert the input with the top flag set. (This causes it to
// retain its ordering, which would otherwise be dropped for a
// subquery.)
```
##########
core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java:
##########
@@ -2861,10 +2897,31 @@ protected void convertFrom(
case INTERSECT:
case EXCEPT:
case UNION:
+ case FROM_QUERY:
+ case PIPE_OPERATOR:
final RelNode rel = convertQueryRecursive(from, false, null).project();
bb.setRoot(rel, true);
return;
+ case PIPE_INPUT:
+ // The input's order matters only if the operator is at the top of the
+ // statement. Then convert the input as top, so that its sort is kept.
+ final boolean keepOrder = bb.top;
+ SqlNode pipeInput = ((SqlPipeInput) from).getInput();
+ while (pipeInput instanceof SqlWith) {
+ // convertWith would convert the body as the top of the statement.
Review Comment:
I removed this comment, I don't think it adds much.
##########
core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java:
##########
@@ -3498,6 +3555,7 @@ protected RelNode createAsofJoin(
final List<CorrelationId> correlNames = new ArrayList<>();
// Mapping from (correlId, originalFieldIndex) to projectedFieldIndex for
aggregation
final Map<Pair<CorrelationId, Integer>, Integer> fieldMapping = new
HashMap<>();
+ int correlFieldCount = Integer.MAX_VALUE;
Review Comment:
changed it to a `@Nullable Integer`.
##########
core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java:
##########
@@ -3590,8 +3651,19 @@ protected RelNode createAsofJoin(
leaves.put(r, r.getRowType().getFieldCount());
}
- // If there are field mappings (due to aggregation), rewrite the RelNode
tree
- // to update correlation variable row type and field indices
+ // The rel of a pipe operator's input may also have trailing columns for
+ // sort keys (see convertNonAggregateSelectList), which the correlation
+ // variable lacks. Give it the rel's row type.
Review Comment:
"it" is the correlation variable, and giving it the row type means changing
the type of the variable to the rel's row type. I think the code flow here
could be improved, so I restructured it a bit to split the logic for
aggregation and for pipe operators. They were previously back-to-back. The
comment is associated with pipe operators only now, and is reworded to try to
be clearer.
--
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]
