You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by "ykisana (via GitHub)" <gi...@apache.org> on 2026/10/07 20:13:39 UTC
[PR] [FLINK-40949][table-runtime] Fail with a clear error on NULL window bounds in windowed assigners [flink]
ykisana opened a new pull request, #29424:
URL: https://github.com/apache/flink/pull/29424
## What is the purpose of the change
`UnsliceAssigners.WindowedUnsliceAssigner` reported a NULL `window_start`/`window_end` as `"RowTime field should not be null."`. That is the same message `WatermarkAssignerOperator` and `SliceAssigners.AbstractSliceAssigner` use for a NULL user rowtime, which the user can fix in their data. The windowed assigners only see rows produced by an upstream window operator, where these columns are `TIMESTAMP(3) NOT NULL`, so a NULL there is an internal bug. The shared message points users at their data and makes the two cases indistinguishable.
The slicing counterparts, `SliceAssigners.WindowedSliceAssigner` (`window_end`) and the sliced assigners used by two-phase window aggregation (`slice_end`), had no null check at all. With a `BinaryRowData`, `getTimestamp` on a NULL compact timestamp reads the zeroed slot, so the row is silently assigned to a slice ending at epoch 0 instead of failing.
## Brief change log
- `WindowedUnsliceAssigner#createWindow` throws an `IllegalStateException` saying the window start or end of a windowed input row is null and that this is a bug.
- `WindowedSliceAssigner#assignSliceEnd` and `AbstractSlicedSliceAssigner#assignSliceEnd` check for a NULL `window_end`/`slice_end` and throw the same kind of exception.
- `WatermarkAssignerOperator` and `AbstractSliceAssigner` are unchanged; their message is correct for a NULL user rowtime.
## Verifying this change
This change added tests and can be verified as follows:
- `WindowedSliceAssignerTest#testNullWindowEnd`: NULL `window_end` fails with the new message.
- New `SlicedSliceAssignerTest`: slice assignment for the shared and unshared sliced assigners, and NULL `slice_end` failure for both.
- New `WindowedUnsliceAssignerTest`: window assignment, plus NULL `window_start` and NULL `window_end` failures.
- Added `nullRow()` and an `assignSliceEnd(SliceAssigner, RowData)` overload to `SliceAssignerTestBase`.
- Verified red-green: without the fix the slicing tests fail because no exception is thrown (the NULL is read as 0), and the unslicing tests fail on the exception type.
## Does this pull request potentially affect one of the following parts:
- Dependencies (does it add or upgrade a dependency): no
- The public API, i.e., is any changed class annotated with `@Public(Evolving)`: no
- The serializers: no
- The runtime per-record code paths (performance sensitive): yes. One null-bit check per record in `WindowedSliceAssigner` and the sliced assigners, the same check `AbstractSliceAssigner` already does per record.
- Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn, ZooKeeper: no
- The S3 file system connector: no
## Documentation
- Does this pull request introduce a new feature? no
- If yes, how is the feature documented? not applicable
---
##### Was generative AI tooling used to co-author this PR?
- [X] Yes (please specify the tool below)
Generated-by: Claude Opus 5.5
Fix written by and all code manually reviewed by a human. Tests, PR description and comments by Claude.
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-40949][table-runtime] Don't report NULL window bounds as a NULL rowtime in WindowedUnsliceAssigner [flink]
Posted by "ykisana (via GitHub)" <gi...@apache.org>.
ykisana commented on PR #29424:
URL: https://github.com/apache/flink/pull/29424#issuecomment-6069428141
> you can repro this with a query like
>
> ```sql
> SELECT a, window_start, window_end, COUNT(*) FROM TUMBLE(TABLE src, DESCRIPTOR(rt), INTERVAL '5' SECOND) GROUP BY a, window_start, window_end
> ```
>
> and then pass `null` for one of the values of `rt`
Thanks for looking into this and finding this! Yeah those code paths are still reachable. I was able to confirm with that query.
I believe WindowedUnsliceAssigner seeing NULL bounds is still unreachable, so I kept that in. Updated message following other examples in Flink.
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-40949][table-runtime] Don't report NULL window bounds as a NULL rowtime in WindowedUnsliceAssigner [flink]
Posted by "ykisana (via GitHub)" <gi...@apache.org>.
ykisana commented on code in PR #29424:
URL: https://github.com/apache/flink/pull/29424#discussion_r4224765209
##########
flink-table/flink-table-runtime/src/test/java/org/apache/flink/table/runtime/operators/window/tvf/unslicing/WindowedUnsliceAssignerTest.java:
##########
@@ -0,0 +1,84 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.flink.table.runtime.operators.window.tvf.unslicing;
+
+import org.apache.flink.table.data.RowData;
+import org.apache.flink.table.data.TimestampData;
+import org.apache.flink.table.data.binary.BinaryRowData;
+import org.apache.flink.table.data.writer.BinaryRowWriter;
+import org.apache.flink.table.runtime.operators.window.TimeWindow;
+
+import org.junit.jupiter.api.Test;
+
+import java.time.Duration;
+import java.time.ZoneId;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+/** Tests for {@link UnsliceAssigners.WindowedUnsliceAssigner}. */
+class WindowedUnsliceAssignerTest {
+
+ private final UnsliceAssigners.WindowedUnsliceAssigner assigner =
+ UnsliceAssigners.windowed(
+ 0, 1, UnsliceAssigners.session(2, ZoneId.of("UTC"), Duration.ofSeconds(5)));
+
+ @Test
+ void testAssignWindows() throws Exception {
+ assertThat(assigner.assignWindows(row(1000L, 6000L), 0L))
+ .containsExactly(new TimeWindow(1000L, 6000L));
+ }
+
+ @Test
+ void testNullWindowStart() {
+ assertThatThrownBy(() -> assigner.assignWindows(row(null, 6000L), 0L))
+ .isInstanceOf(IllegalStateException.class)
+ .hasMessage(
+ "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.");
+ }
+
+ @Test
+ void testNullWindowEnd() {
+ assertThatThrownBy(() -> assigner.assignWindows(row(1000L, null), 0L))
Review Comment:
Yeah makes sense, will update once we get a clearer picture what to do for this ticket 👍
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-40949][table-runtime] Update UnsliceAssigners NULL window exception message [flink]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin merged PR #29424:
URL: https://github.com/apache/flink/pull/29424
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-40949][table-runtime] Don't report NULL window bounds as a NULL rowtime in WindowedUnsliceAssigner [flink]
Posted by "ykisana (via GitHub)" <gi...@apache.org>.
ykisana commented on code in PR #29424:
URL: https://github.com/apache/flink/pull/29424#discussion_r4227119639
##########
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 updated, thanks for confirming. simple pr now
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-40949][table-runtime] Don't report NULL window bounds as a NULL rowtime in WindowedUnsliceAssigner [flink]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on code in PR #29424:
URL: https://github.com/apache/flink/pull/29424#discussion_r4224414520
##########
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:
why do we need this?
i'm pretty sure same story
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-40949][table-runtime] Fail with a clear error on NULL window bounds in windowed assigners [flink]
Posted by "flinkbot (via GitHub)" <gi...@apache.org>.
flinkbot commented on PR #29424:
URL: https://github.com/apache/flink/pull/29424#issuecomment-6046126155
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "14eb2e4503ab1431efc3eebf98fd68c45e85ada2",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "14eb2e4503ab1431efc3eebf98fd68c45e85ada2",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 14eb2e4503ab1431efc3eebf98fd68c45e85ada2 UNKNOWN
<details>
<summary>Bot commands</summary>
The @flinkbot bot supports the following commands:
- `@flinkbot run azure` re-run the last Azure build
</details>
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-40949][table-runtime] Don't report NULL window bounds as a NULL rowtime in WindowedUnsliceAssigner [flink]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on code in PR #29424:
URL: https://github.com/apache/flink/pull/29424#discussion_r4224603015
##########
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:
>I believe this path is unreachable.
also even if it is true, why do we even touch unreachable message?
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-40949][table-runtime] Fail with a clear error on NULL window bounds in windowed assigners [flink]
Posted by "MartijnVisser (via GitHub)" <gi...@apache.org>.
MartijnVisser commented on PR #29424:
URL: https://github.com/apache/flink/pull/29424#issuecomment-6054882336
@ykisana For the next time, please wait with opening a PR until you're assigned to the Jira ticket
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-40949][table-runtime] Fail with a clear error on NULL window bounds in windowed assigners [flink]
Posted by "ykisana (via GitHub)" <gi...@apache.org>.
ykisana commented on code in PR #29424:
URL: https://github.com/apache/flink/pull/29424#discussion_r4218220667
##########
flink-table/flink-table-runtime/src/main/java/org/apache/flink/table/runtime/operators/window/tvf/slicing/SliceAssigners.java:
##########
@@ -629,6 +633,10 @@ public AbstractSlicedSliceAssigner(int sliceEndIndex, SliceAssigner innerAssigne
@Override
public long assignSliceEnd(RowData element, ClockService clock) {
+ if (element.isNullAt(sliceEndIndex)) {
+ throw new IllegalStateException(
+ "Slice end of a sliced input row is null. This is a bug.");
Review Comment:
@snuyanzin I tried to follow bug messages in the rest of the repo 'This is a bug' is found in other places. It's an IllegalStateException. User can't do anything about it, bug on our side.
Updating to make this more clear though. Let me know if you have suggestions.
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-40949][table-runtime] Don't report NULL window bounds as a NULL rowtime in WindowedUnsliceAssigner [flink]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on PR #29424:
URL: https://github.com/apache/flink/pull/29424#issuecomment-6076417026
also ci is red
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-40949][table-runtime] Fail with a clear error on NULL window bounds in windowed assigners [flink]
Posted by "ykisana (via GitHub)" <gi...@apache.org>.
ykisana commented on code in PR #29424:
URL: https://github.com/apache/flink/pull/29424#discussion_r4222083057
##########
flink-table/flink-table-runtime/src/main/java/org/apache/flink/table/runtime/operators/window/tvf/slicing/SliceAssigners.java:
##########
@@ -629,6 +633,10 @@ public AbstractSlicedSliceAssigner(int sliceEndIndex, SliceAssigner innerAssigne
@Override
public long assignSliceEnd(RowData element, ClockService clock) {
+ if (element.isNullAt(sliceEndIndex)) {
+ throw new IllegalStateException(
+ "Slice end of a sliced input row is null. This is a bug.");
Review Comment:
@snuyanzin Yeah I agree. What's the process for that?
The ticket is for error messaging. I can do a follow up for looking at paths where we actually enter this state.
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-40949][table-runtime] Fail with a clear error on NULL window bounds in windowed assigners [flink]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on code in PR #29424:
URL: https://github.com/apache/flink/pull/29424#discussion_r4218387378
##########
flink-table/flink-table-runtime/src/main/java/org/apache/flink/table/runtime/operators/window/tvf/slicing/SliceAssigners.java:
##########
@@ -629,6 +633,10 @@ public AbstractSlicedSliceAssigner(int sliceEndIndex, SliceAssigner innerAssigne
@Override
public long assignSliceEnd(RowData element, ClockService clock) {
+ if (element.isNullAt(sliceEndIndex)) {
+ throw new IllegalStateException(
+ "Slice end of a sliced input row is null. This is a bug.");
Review Comment:
>User can't do anything about it, bug on our side.
if somewhere is NPE it is also bug in Flink, however I hardly tend to think that we need to insert tons of checks for NPE and for every tell that bug in our side because something is null...
Can we better try to understand cases we come here and why?
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-40949][table-runtime] Update UnsliceAssigners NULL window exception message [flink]
Posted by "ykisana (via GitHub)" <gi...@apache.org>.
ykisana commented on PR #29424:
URL: https://github.com/apache/flink/pull/29424#issuecomment-6302502978
@snuyanzin Thanks for the review. I've updated the commit message and PR title to describe the change more precisely, and CI is green on the latest commit (8086cad). Could you take another look when you get a chance?
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-40949][table-runtime] Fail with a clear error on NULL window bounds in windowed assigners [flink]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on code in PR #29424:
URL: https://github.com/apache/flink/pull/29424#discussion_r4217678673
##########
flink-table/flink-table-runtime/src/main/java/org/apache/flink/table/runtime/operators/window/tvf/slicing/SliceAssigners.java:
##########
@@ -629,6 +633,10 @@ public AbstractSlicedSliceAssigner(int sliceEndIndex, SliceAssigner innerAssigne
@Override
public long assignSliceEnd(RowData element, ClockService clock) {
+ if (element.isNullAt(sliceEndIndex)) {
+ throw new IllegalStateException(
+ "Slice end of a sliced input row is null. This is a bug.");
Review Comment:
this is a bug where?
flink, jdk, data, something else?
if I see it as a user how can this message help me?
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-40949][table-runtime] Don't report NULL window bounds as a NULL rowtime in WindowedUnsliceAssigner [flink]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on code in PR #29424:
URL: https://github.com/apache/flink/pull/29424#discussion_r4227074951
##########
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:
change only message like
```
throw new RuntimeException("Both window start and window end should not be null.");
```
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-40949][table-runtime] Don't report NULL window bounds as a NULL rowtime in WindowedUnsliceAssigner [flink]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on PR #29424:
URL: https://github.com/apache/flink/pull/29424#issuecomment-6076403800
the commit/pr message is still vague
>Don't report NULL window bounds as a NULL rowtime
reporting it as NOT NULL rowtime also satisfying this message however it is not true
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-40949][table-runtime] Fail with a clear error on NULL window bounds in windowed assigners [flink]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on PR #29424:
URL: https://github.com/apache/flink/pull/29424#issuecomment-6067573393
you can repro this with a query like
```sql
SELECT a, window_start, window_end, COUNT(*) FROM TUMBLE(TABLE src, DESCRIPTOR(rt), INTERVAL '5' SECOND) GROUP BY a, window_start, window_end
```
and then pass `null` for one of the values of `rt`
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-40949][table-runtime] Don't report NULL window bounds as a NULL rowtime in WindowedUnsliceAssigner [flink]
Posted by "ykisana (via GitHub)" <gi...@apache.org>.
ykisana commented on code in PR #29424:
URL: https://github.com/apache/flink/pull/29424#discussion_r4224808249
##########
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:
cc @MartijnVisser
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-40949][table-runtime] Don't report NULL window bounds as a NULL rowtime in WindowedUnsliceAssigner [flink]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on code in PR #29424:
URL: https://github.com/apache/flink/pull/29424#discussion_r4224651308
##########
flink-table/flink-table-runtime/src/test/java/org/apache/flink/table/runtime/operators/window/tvf/unslicing/WindowedUnsliceAssignerTest.java:
##########
@@ -0,0 +1,84 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.flink.table.runtime.operators.window.tvf.unslicing;
+
+import org.apache.flink.table.data.RowData;
+import org.apache.flink.table.data.TimestampData;
+import org.apache.flink.table.data.binary.BinaryRowData;
+import org.apache.flink.table.data.writer.BinaryRowWriter;
+import org.apache.flink.table.runtime.operators.window.TimeWindow;
+
+import org.junit.jupiter.api.Test;
+
+import java.time.Duration;
+import java.time.ZoneId;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+/** Tests for {@link UnsliceAssigners.WindowedUnsliceAssigner}. */
+class WindowedUnsliceAssignerTest {
+
+ private final UnsliceAssigners.WindowedUnsliceAssigner assigner =
+ UnsliceAssigners.windowed(
+ 0, 1, UnsliceAssigners.session(2, ZoneId.of("UTC"), Duration.ofSeconds(5)));
+
+ @Test
+ void testAssignWindows() throws Exception {
+ assertThat(assigner.assignWindows(row(1000L, 6000L), 0L))
+ .containsExactly(new TimeWindow(1000L, 6000L));
+ }
+
+ @Test
+ void testNullWindowStart() {
+ assertThatThrownBy(() -> assigner.assignWindows(row(null, 6000L), 0L))
+ .isInstanceOf(IllegalStateException.class)
+ .hasMessage(
+ "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.");
+ }
+
+ @Test
+ void testNullWindowEnd() {
+ assertThatThrownBy(() -> assigner.assignWindows(row(1000L, null), 0L))
Review Comment:
what is the reason of these tests 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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-40949][table-runtime] Don't report NULL window bounds as a NULL rowtime in WindowedUnsliceAssigner [flink]
Posted by "ykisana (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-40949][table-runtime] Fail with a clear error on NULL window bounds in windowed assigners [flink]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on code in PR #29424:
URL: https://github.com/apache/flink/pull/29424#discussion_r4217685792
##########
flink-table/flink-table-runtime/src/test/java/org/apache/flink/table/runtime/operators/window/tvf/slicing/SlicedSliceAssignerTest.java:
##########
@@ -0,0 +1,61 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.flink.table.runtime.operators.window.tvf.slicing;
+
+import org.junit.jupiter.api.Test;
+
+import java.time.Duration;
+import java.time.ZoneId;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+/**
+ * Tests for {@link SliceAssigners.SlicedSharedSliceAssigner} and {@link
+ * SliceAssigners.SlicedUnsharedSliceAssigner}.
+ */
+class SlicedSliceAssignerTest extends SliceAssignerTestBase {
Review Comment:
why do we need class at all?
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-40949][table-runtime] Fail with a clear error on NULL window bounds in windowed assigners [flink]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on code in PR #29424:
URL: https://github.com/apache/flink/pull/29424#discussion_r4217669068
##########
flink-table/flink-table-runtime/src/main/java/org/apache/flink/table/runtime/operators/window/tvf/slicing/SliceAssigners.java:
##########
@@ -629,6 +633,10 @@ public AbstractSlicedSliceAssigner(int sliceEndIndex, SliceAssigner innerAssigne
@Override
public long assignSliceEnd(RowData element, ClockService clock) {
+ if (element.isNullAt(sliceEndIndex)) {
+ throw new IllegalStateException(
+ "Slice end of a sliced input row is null. This is a bug.");
Review Comment:
and what is call to action to user?
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-40949][table-runtime] Fail with a clear error on NULL window bounds in windowed assigners [flink]
Posted by "ykisana (via GitHub)" <gi...@apache.org>.
ykisana commented on code in PR #29424:
URL: https://github.com/apache/flink/pull/29424#discussion_r4218365945
##########
flink-table/flink-table-runtime/src/test/java/org/apache/flink/table/runtime/operators/window/tvf/slicing/SlicedSliceAssignerTest.java:
##########
@@ -0,0 +1,61 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.flink.table.runtime.operators.window.tvf.slicing;
+
+import org.junit.jupiter.api.Test;
+
+import java.time.Duration;
+import java.time.ZoneId;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+/**
+ * Tests for {@link SliceAssigners.SlicedSharedSliceAssigner} and {@link
+ * SliceAssigners.SlicedUnsharedSliceAssigner}.
+ */
+class SlicedSliceAssignerTest extends SliceAssignerTestBase {
Review Comment:
Removed it. The sliced test now sits in WindowedSliceAssignerTest.
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-40949][table-runtime] Don't report NULL window bounds as a NULL rowtime in WindowedUnsliceAssigner [flink]
Posted by "ykisana (via GitHub)" <gi...@apache.org>.
ykisana commented on code in PR #29424:
URL: https://github.com/apache/flink/pull/29424#discussion_r4224577165
##########
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 At this stage we only expect valid windows, NULL rowtimes are rejected upstream. I believe this path is unreachable.
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-40949][table-runtime] Don't report NULL window bounds as a NULL rowtime in WindowedUnsliceAssigner [flink]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on code in PR #29424:
URL: https://github.com/apache/flink/pull/29424#discussion_r4224660547
##########
flink-table/flink-table-runtime/src/test/java/org/apache/flink/table/runtime/operators/window/tvf/unslicing/WindowedUnsliceAssignerTest.java:
##########
@@ -0,0 +1,84 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.flink.table.runtime.operators.window.tvf.unslicing;
+
+import org.apache.flink.table.data.RowData;
+import org.apache.flink.table.data.TimestampData;
+import org.apache.flink.table.data.binary.BinaryRowData;
+import org.apache.flink.table.data.writer.BinaryRowWriter;
+import org.apache.flink.table.runtime.operators.window.TimeWindow;
+
+import org.junit.jupiter.api.Test;
+
+import java.time.Duration;
+import java.time.ZoneId;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+/** Tests for {@link UnsliceAssigners.WindowedUnsliceAssigner}. */
+class WindowedUnsliceAssignerTest {
+
+ private final UnsliceAssigners.WindowedUnsliceAssigner assigner =
+ UnsliceAssigners.windowed(
+ 0, 1, UnsliceAssigners.session(2, ZoneId.of("UTC"), Duration.ofSeconds(5)));
+
+ @Test
+ void testAssignWindows() throws Exception {
+ assertThat(assigner.assignWindows(row(1000L, 6000L), 0L))
+ .containsExactly(new TimeWindow(1000L, 6000L));
+ }
+
+ @Test
+ void testNullWindowStart() {
+ assertThatThrownBy(() -> assigner.assignWindows(row(null, 6000L), 0L))
+ .isInstanceOf(IllegalStateException.class)
+ .hasMessage(
+ "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.");
+ }
+
+ @Test
+ void testNullWindowEnd() {
+ assertThatThrownBy(() -> assigner.assignWindows(row(1000L, null), 0L))
Review Comment:
we don't need to accumulate low level tests which are always subject to refactor and nobody can explain why we need it.
Better to focus on SQL tests
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-40949][table-runtime] Don't report NULL window bounds as a NULL rowtime in WindowedUnsliceAssigner [flink]
Posted by "snuyanzin (via GitHub)" <gi...@apache.org>.
snuyanzin commented on code in PR #29424:
URL: https://github.com/apache/flink/pull/29424#discussion_r4224590655
##########
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:
>I believe this path is unreachable.
it is not a proof, need something more reliable as a proof
--
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: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org