imankhadiv commented on PR #29027: URL: https://github.com/apache/flink/pull/29027#issuecomment-5443945780
@nateab thanks for assigning me FLINK-40232 — Azure is green on this one, so it's ready for review. Both rowtime lookups in `OutputConversionOperator` now go through a small `updateRowtime(RowData, int)` helper that null-checks the rowtime. The one part that goes beyond the null checks suggested in the ticket is the `else` branch: `outRecord` is a single `StreamRecord` allocated in `open()` and reused for every element, and `StreamRecord#replace(Object)` leaves the timestamp untouched — so only skipping `setTimestamp()` would leave the *previous* record's timestamp on the record with the null rowtime, turning the NPE into a silently wrong timestamp. Hence the explicit `eraseTimestamp()`. Test coverage in `OutputConversionOperatorTest` follows that split: for both the rowtime metadata path and the rowtime column path, one case asserts the record is emitted without a timestamp, and one asserts it does not inherit the preceding record's timestamp. The latter two were confirmed failing against a null-checks-only version of the fix, which is what motivated the erase. Locally `flink-table-runtime` (1824 tests) and `DataStreamJavaITCase` (38 tests, covers the non-null rowtime path end to end) pass. Could you take a look, or point me to the right committer for table-runtime? Happy to rebase or adjust anything. -- 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]
