davidradl commented on code in PR #29003:
URL: https://github.com/apache/flink/pull/29003#discussion_r3879982019
##########
flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/plan/nodes/exec/stream/StreamExecSort.java:
##########
@@ -100,12 +103,21 @@ 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.
+ int firstSortField = sortSpec.getFieldIndices()[0];
Review Comment:
Playing with my AI on this. It raised a couple of nits, I thought I would
run by you
The backstop path is not tested. The new code in
StreamExecSort.translateToPlanInternal is the backstop for compiled plans
loaded via loadPlan. There is no test that:
1. Compiles a plan containing a StreamExecSort with a non-temporal sort
(using TABLE_EXEC_NON_TEMPORAL_SORT_ENABLED=true to get past the rule check).
2. Loads that plan with the flag unset.
3. Asserts that translateToPlanInternal throws the correct targeted message.
The previous single-message backstop was arguably not tested either, but the
new two-branch backstop has more logic to verify. This is a minor gap since the
path is explicitly labelled a backstop, but it could be caught by a round-trip
test.
Also it is suggesting that a corrupt plan could end up with no field indices
- a defensive check could be added to check we have elements, before we address
the first element in line:
int firstSortField = sortSpec.getFieldIndices()[0];
WDYT?
--
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]
