DanielLeens commented on code in PR #12736: URL: https://github.com/apache/seatunnel/pull/12736#discussion_r4357172943
########## seatunnel-connectors-v2/connector-cdc/connector-cdc-base/src/main/java/org/apache/seatunnel/connectors/cdc/debezium/row/SeaTunnelRowDebeziumDeserializationConverters.java: ########## Review Comment: Thanks @davidzollo for the catch and @LeonYoah for the quick answer. I checked this thread against the unchanged head `c55f68b040` (static reading, nothing executed). One small request so we are all talking about the same thing: the reply says "I will remove this PR", which I read as "this change" (a typo for "this part"). Please confirm which hunk you are removing. My reading of the point: in `parseOffsetDateTimeFromString`, step 1 is `OffsetDateTime.parse(str)`, which uses `DateTimeFormatter.ISO_OFFSET_DATE_TIME`. The new `TemporalConversions.parseDebeziumTimestamp` used in step 4 parses with the very same formatter, so any string it can parse has already been returned by step 1, and step 4 can never succeed. That makes the step 4 edit and the matching Javadoc line (`parseDebeziumTimestamp` ... "numeric offset") dead, as @davidzollo says. The method itself is not dead: `convertToTimestampTz` calls it for every `String` value, and `SeaTunnelRowDebeziumDeserializationConvertersTest` exercises it. So the concrete next step is to revert only the step 4 hunk and the Javadoc list entry in `parseOffsetDateTimeFromString` back to `dev` (the `java.time.Instant` import that this PR removed has to come back with it, since step 4 uses `Instant.parse` again), and keep the two hunks that actually change behavior: `TemporalConversions.toLocalDateTime` and the `String` branch of `convertToTimestamp`, which now go through `parseDebeziumTimestamp`. If you drop the whole method instead, `convertToTimestampTz` and its tests need to change as well, so please do not do that in this PR. Nothing has been pushed yet, so I will recheck the diff once the new commit lands. My earlier approval was given on this head with a green `Build`; a small revert like this does not change my source-level conclusion, but I will re-read the whole diff on the new head as usual. -- 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]
