SEZ9 commented on PR #12592: URL: https://github.com/apache/seatunnel/pull/12592#issuecomment-6392270079
@hutiefang76 thanks for the condensed walkthrough against `cb6ae01c` — going point by point: - **F1/F4:** The split you describe (source exposes scale, each sink's converter owns its DDL) sounds right, and the upgrade-note wording you summarise (MySQL/PostgreSQL/SQL Server/Oracle, plain DuckDB `TIMESTAMP` → MySQL `DATETIME(6)`, existing tables not resized) is what I was looking for. I'll confirm it against the diff before closing F4. One thing your summary doesn't cover: does `DuckDBTypeConverter.reconvert` now honour scale for the DuckDB-to-DuckDB `TIMESTAMP_NS` round-trip, or is that also a documented deferred limitation? - **F2/F6:** Accepting the deferral in principle. If the regression test pins the 1918 DST-gap and 1582 cutover shifts in four zones through both source modes, and the note calls them out as known limitations, that resolves F2/F6 as "known and pinned" rather than "fixed", which is fine for this PR. I'll verify the test cases and the note text in the diff. - **F3/F8:** Your comment was cut off mid-sentence right at the row converter link, so I haven't seen this part. Could you repost it? Specifically: (a) is the one-time WARN still unconditional for any alias column read, or has it been narrowed/downgraded, and (b) does the Javadoc now state that metadata-based detection is driver-version-independent and will not pick up a future typed getter? - **F5/F7:** Also missing from the truncated text. Please confirm whether the per-cell `ResultSetMetaData` lookup and per-value `Calendar` allocation stay as a named follow-up rather than being hoisted in this PR, and link a tracking item if there is one so it doesn't get lost. I haven't independently verified the CI runs (`119994188490` / `39774824149`) for head `cb6ae01c05481b1085bc3bc3c3f430d74b909fd2` yet; I'll do that as part of the final pass. Remaining asks before I approve: the truncated F3/F8 and F5/F7 text, and the `reconvert` answer under F1. No new commit needed unless one of those turns up a gap. <!-- streview-comment:2005 --> -- 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]
