ykisana commented on code in PR #29424:
URL: https://github.com/apache/flink/pull/29424#discussion_r4224739257
##########
flink-table/flink-table-runtime/src/main/java/org/apache/flink/table/runtime/operators/window/tvf/unslicing/UnsliceAssigners.java:
##########
@@ -229,7 +229,10 @@ public Collection<TimeWindow> assignWindows(RowData
element, long timestamp)
private TimeWindow createWindow(RowData element) {
if (element.isNullAt(windowStartIndex) ||
element.isNullAt(windowEndIndex)) {
- throw new RuntimeException("RowTime field should not be
null.");
+ throw new IllegalStateException(
+ "Window start or window end of the input row is null. "
+ + "Both should be set by the upstream window
operator. "
+ + "This is a bug. Please file an issue.");
Review Comment:
@snuyanzin I'm happy to leave message as is, or maybe just change to invalid
state exception.
The purpose of ticket was clearer messaging:
https://issues.apache.org/jira/browse/FLINK-40949
As for why it should be unreachable:
First Check:
https://github.com/apache/flink/blob/master/flink-table/flink-table-runtime/src/main/java/org/apache/flink/table/runtime/operators/wmassigners/WatermarkAssignerOperator.java#L136-L140
Second Check:
https://github.com/apache/flink/blob/master/flink-table/flink-table-runtime/src/main/java/org/apache/flink/table/runtime/operators/window/tvf/operator/UnalignedWindowTableFunctionOperator.java#L200-L205
If survived these checks, they get real bounds:
https://github.com/apache/flink/blob/master/flink-table/flink-table-runtime/src/main/java/org/apache/flink/table/runtime/operators/window/tvf/operator/WindowTableFunctionOperatorBase.java#L109-L121
Expected real bounds here:
https://github.com/apache/flink/blob/master/flink-table/flink-table-planner/src/main/scala/org/apache/flink/table/planner/plan/rules/physical/stream/StreamPhysicalWindowAggregateRule.scala#L111-L115
Thus real bounds here (which calls the function I changed):
https://github.com/apache/flink/blob/master/flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/plan/nodes/exec/stream/StreamExecWindowAggregateBase.java#L77-L90
Note, your counter example only proved for SliceAssigners in a DIFFERENT
path.
In this path, SliceAssigners also is expected real bounds.
UnsliceAssigners only lives here. It doesn't live in other paths without
real bound pruning like here.
Let me know if I missed anything or how I should handle this.
Change exception? Or just leave as is? Ticket wanted something different
here.
--
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]
