Myracle commented on code in PR #27554:
URL: https://github.com/apache/flink/pull/27554#discussion_r2797361396
##########
flink-table/flink-table-api-java/src/main/java/org/apache/flink/table/api/config/ExecutionConfigOptions.java:
##########
@@ -106,6 +106,31 @@ public class ExecutionConfigOptions {
+ "an additional stateful
operator.")
.build());
+ @Documentation.TableOption(execMode = Documentation.ExecMode.STREAMING)
+ public static final ConfigOption<RowtimeNullHandling>
TABLE_EXEC_SOURCE_ROWTIME_NULL_HANDLING =
+ key("table.exec.source.rowtime-null-handling")
+ .enumType(RowtimeNullHandling.class)
+ .defaultValue(RowtimeNullHandling.FAIL)
+ .withDescription(
+ Description.builder()
+ .text(
+ "Specifies the behavior when the
rowtime field is null "
+ + "during watermark
generation.")
+ .linebreak()
+ .linebreak()
+ .text(
+ "FAIL: Throw a runtime exception
when encountering null rowtime (default). "
+ + "This preserves the
current behavior.")
Review Comment:
Thank you for the thoughtful review! Your suggestion is valid and I agree
with your points.
**Changes made:**
1. **Removed the ambiguous phrase** - Replaced "This preserves the current
behavior" as it could be misread as "something occurring in the flow" rather
than "the previous default behavior of the code."
2. **Marked FAIL as recommended** - Added "(default, recommended)" to the
FAIL option description, as you correctly pointed out that null rowtime
typically indicates data quality issues that need attention.
3. **Added risk warnings** - Enhanced descriptions for DROP and
SKIP_WATERMARK to clarify their potential implications:
4. **DROP**: "Note that this may result in data loss."
5. **SKIP_WATERMARK**: "Note that this may cause issues in downstream
operators if they expect valid rowtime values."
6. **Added context about null rowtime scenarios** - The FAIL description now
explains: "A null rowtime typically indicates that the source is receiving
unexpected null values in a column that should not be null, which suggests the
data flow needs to be cleaned up."
The updated description now reads:
`FAIL (default, recommended): Throw a runtime exception when encountering
null rowtime.
A null rowtime typically indicates that the source is receiving unexpected
null values
in a column that should not be null, which suggests the data flow needs to
be cleaned up.
DROP: Drop the record silently. The 'numNullRowtimeRecordsDropped' metric
will be incremented.
Note that this may result in data loss.
SKIP_WATERMARK: Forward the record without advancing the watermark.
The 'numNullRowtimeRecordsSkipped' metric will be incremented.
Note that this may cause issues in downstream operators if they expect valid
rowtime values.
`
I've also updated the HTML configuration documentation accordingly.
--
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]
