Timm0 commented on code in PR #28612:
URL: https://github.com/apache/flink/pull/28612#discussion_r3518361548
##########
flink-table/flink-table-planner/src/test/java/org/apache/flink/table/api/QueryOperationSqlSerializationTest.java:
##########
@@ -59,6 +59,9 @@ public List<TableTestProgram> programs() {
QueryOperationTestPrograms.DISTINCT_QUERY_OPERATION,
QueryOperationTestPrograms.JOIN_QUERY_OPERATION,
QueryOperationTestPrograms.ORDER_BY_QUERY_OPERATION,
+ QueryOperationTestPrograms.ORDER_BY_AGGREGATE_QUERY_OPERATION,
+
QueryOperationTestPrograms.ORDER_BY_NO_FETCH_AGGREGATE_QUERY_OPERATION,
+ QueryOperationTestPrograms.AGGREGATE_HAVING_QUERY_OPERATION,
Review Comment:
Could we also add these tests to `QueryOperationSqlSemanticTest`?
##########
flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/plan/QueryOperationConverter.java:
##########
@@ -176,6 +185,14 @@ public RelNode visit(ProjectQueryOperation projection) {
@Override
public RelNode visit(AggregateQueryOperation aggregate) {
+ // Project the input off a sort (as SqlToRelConverter does) so the
sort's collation is
+ // not required on the aggregate output, avoiding an IOOBE in
rules like e.g.
+ // FlinkExpandConversionRule.
+ if (relBuilder.peek() instanceof Sort) {
+ relBuilder.project(
+ relBuilder.fields(),
relBuilder.peek().getRowType().getFieldNames(), true);
Review Comment:
Nit: Maybe only peek once so it is a bit more obvious that we run these
actions on the same node.
##########
flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/calcite/FlinkRelBuilder.java:
##########
@@ -242,6 +242,7 @@ public RelBuilder watermark(int rowtimeFieldIndex, RexNode
watermarkExpr) {
}
public RelBuilder queryOperation(QueryOperation queryOperation) {
+ toRelNodeConverter.setRootOperation(queryOperation);
Review Comment:
Doesn't this mean that the root could be changed mid-traversal? So for
example when I run `tEnv.where($("a").in(sub)).orderBy($("b"))`, the
`InConverter` re-enters `queryOperation` on the same builder and overrides the
root operation. In that case my order by would be dropped. Could you verify if
that is the case?
--
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]
