MartijnVisser commented on code in PR #29003:
URL: https://github.com/apache/flink/pull/29003#discussion_r3971824529


##########
flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/plan/nodes/exec/stream/StreamExecSort.java:
##########
@@ -100,12 +103,25 @@ public StreamExecSort(
     @Override
     protected Transformation<RowData> translateToPlanInternal(
             PlannerBase planner, ExecNodeConfig config) {
+        ExecEdge inputEdge = getInputEdges().get(0);
+        RowType inputType = (RowType) inputEdge.getOutputType();
         if 
(!config.get(InternalConfigOptions.TABLE_EXEC_NON_TEMPORAL_SORT_ENABLED)) {
-            throw new TableException("Sort on a non-time-attribute field is 
not supported.");
+            // Backstop for compiled plans loaded without passing through 
StreamPhysicalSortRule.
+            if (sortSpec.getFieldSize() == 0) {
+                throw new TableException(
+                        "Compiled plan contains a streaming sort without sort 
keys.");
+            }
+            int firstSortField = sortSpec.getFieldIndices()[0];
+            String column = inputType.getFieldNames().get(firstSortField);
+            LogicalType type = inputType.getTypeAt(firstSortField);
+            if (LogicalTypeChecks.isTimeAttribute(type)
+                    && !sortSpec.getFieldSpecs()[0].getIsAscendingOrder()) {
+                throw new TableException(
+                        
SortUtil.sortKeyTimeAttributeMustBeAscendingMessage(column));
+            }
+            throw new 
TableException(SortUtil.sortKeyNotTimeAttributeMessage(column, type));

Review Comment:
   The condition is right: a descending time attribute takes the first branch 
and gets the "must be sorted in ascending order" message. Verified for `rowtime 
DESC`, `proctime DESC` and a TIMESTAMP_LTZ rowtime, both through the rule and 
through a compiled plan loaded without the flag; 
`testLoadedPlanRejectsDescendingTimeAttributeSort` and the `rowtime DESC, c` 
case in `nonTemporalSorts` assert exactly that. The message you quote only 
comes out if a plan is hand-edited to put an ascending time attribute into a 
`StreamExecSort`, which the planner never emits (that becomes a temporal sort). 
Did you see it with a specific query?



-- 
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]

Reply via email to