SEZ9 commented on PR #11660: URL: https://github.com/apache/seatunnel/pull/11660#issuecomment-5230721657
Thanks for the correction on the `~48 ms` figure — I appreciate you flagging it explicitly rather than letting it sit in the thread. Agreed that it doesn't change the conclusion here: whether `Configuration` parsing is 3% or 90% of per-file cost, writer creation is per-file work and hoisting the data model to an instance field stays a non-goal for this PR. I'll treat #11661 as the authoritative numbers going forward. ## On the round-trip test Let's go with **option 2 — a dedicated logical-type round-trip test class**. Your reasoning is right: this has nothing to do with schema evolution, and a dedicated class gives the three `addLogicalTypeConversion` registrations a clear home for coverage. Please include it in this PR rather than a follow-up, since it directly covers the code this diff relocates. Two concrete asks for that test, tying back to my earlier review: 1. **Assert the actual output file count is 1**, not just that `rows.size() == readBack.size()`. An equal row count doesn't prove the reuse branch was exercised if a future rolling-policy or batch-size change splits the output. 2. **Use timestamp fixtures with sub-second precision** (non-zero millis), so the millis conversion path is actually exercised rather than degenerating at whole seconds. ## On the possibly-unreachable `local-timestamp-millis` conversion Good catch, and I agree with your restraint — removing a registration based on "looks unreachable" without proving no path produces that logical type doesn't belong in a relocation PR. Please open a separate issue capturing your analysis (`timestamp-millis` vs `local-timestamp-millis`, plus the manual `LocalDateTime` handling in `resolveObject`), so the investigation isn't lost in this thread. If the round-trip test happens to surface it as dead, the removal can reference that issue. ## CI Agreed on the diagnosis — the `JobStateEventTest` failure is Hazelcast node bring-up in `seatunnel-engine-server`, untouched by this diff. Thanks for triggering the rerun; let's wait for it to settle before merging. So, to summarize the remaining asks: (1) dedicated round-trip test class with a file-count assertion and sub-second timestamps, (2) a follow-up issue for the `local-timestamp-millis` question, (3) green CI. Once those land I'm happy to merge. <!-- streview-comment:108 --> -- 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]
