DanielLeens commented on PR #12592: URL: https://github.com/apache/seatunnel/pull/12592#issuecomment-6310081237
Thanks, @SEZ9, for the careful pass. I re-read the PR at the current head `743ed3a0` against your remaining asks, and a few of them are already covered by what is on the branch, so here are the pointers (all paths relative to `seatunnel-connectors-v2/connector-jdbc/src/test/java/org/apache/seatunnel/connectors/seatunnel/jdbc/internal/dialect/duckdb/`): - **F6 (pre-epoch edge cases).** `DuckDBSourceAndSinkTest.testPreEpochAliasReadsKeepTheLegacyGetterForDstGapAndCutover` (around line 1255) stores a 1918-03-31 02:30 value inside a US spring-forward gap, a 1918 value outside it, and a 1582-10-10 Gregorian-cutover value, and reads them through the real source path. `DuckDBJdbcRowConverterTest.testPreEpochAliasValueKeepsTheLegacyGetter` (around line 157) pins the converter branch itself. - **F1 (scale round trip).** `DuckDBTypeConverterTest.testReconvertTimestampFollowsDeclaredPrecision` (around line 525) maps scales null/0/3/6/9 to `TIMESTAMP`/`TIMESTAMP_S`/`TIMESTAMP_MS`/`TIMESTAMP`/`TIMESTAMP_NS`, and `testReconvertTimestampWidensIntermediatePrecision` covers the in-between scales. `DuckDBSourceAndSinkTest.testSinkAutoCreatedTimestampColumnsKeepNanos` (around line 198) runs the production `JdbcSaveModeHandler` from columns declared at the four native scales and asserts the DuckDB column types it creates. So a `TIMESTAMP_NS` source column does not fall back to plain `TIMESTAMP` in the generated DuckDB DDL. - **F4 (upgrade note).** `docs/en/introduction/concepts/incompatible-changes.md` (the "DuckDB timestamp source values" section, around lines 119-129; the zh page mirrors it) already says that a standard DuckDB `TIMESTAMP` now reports scale 6, so generated DDL can declare `DATETIME(6)` for MySQL (and `timestamp(6)`, `DATETIME2(6)`, `TIMESTAMP(6)` for the others) where it previously relied on the target default, and that a `TIMESTAMP_NS` source becomes `DATETIME(6)` on MySQL. If you would like the wording more prominent, e.g. a one-line summary at the top of that section, that would be a small docs-only tweak. F5/F7 and the `ResultSet` reference remain the Low, deferred follow-up items, and I agree a short TODO or a note in the PR description is a good way to keep them from getting lost. One thing that is still genuinely open: I could not find a finished `Build` run for this head. The apache-side check is `pending`, and the fork run for `743ed3a0` has been queued for many hours, so there is no green CI result yet. @hutiefang76, if that run does not start, it may be worth checking the fork's Actions queue or runner availability rather than pushing again, so the result stays attributable to this head. -- 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]
