SEZ9 commented on PR #12592: URL: https://github.com/apache/seatunnel/pull/12592#issuecomment-6357539330
Thanks @hutiefang76 for the update on the supplemental Edge Agent run (39783558215) against `cb6ae01c05481b1085bc3bc3c3f430d74b909fd2`, and for being clear that it is fork CI and does not replace the required PR Build — agreed, we still need that result on this PR. That run doesn't touch the open items from the previous review, so those still need a response or a follow-up push. To make it concrete, here is what I'm still looking for: - **PR12592-F1 (Compatibility)** – The hardcoded scale 6/9 in `DuckDBTypeConverter` changes auto-DDL for every DuckDB-to-X pipeline, not only MySQL, while `reconvert` still ignores scale, so DuckDB-to-DuckDB `TIMESTAMP_NS` silently degrades to `TIMESTAMP`. Please either carry scale through `reconvert` or at least log when precision is dropped, and confirm the intended behaviour for non-MySQL sinks. - **PR12592-F2 (Logic)** – Pre-epoch alias values still fall back to JVM-default-zone `getTimestamp`, which can shift values inside pre-1970 DST gaps and pre-1582 dates. This path is neither guarded nor tested; please add a guard or an explicit test pinning the current behaviour. - **PR12592-F3 (Robustness)** – The one-time WARN fires for every job that reads any alias column and hard-codes a driver version. Please gate it on actually hitting the legacy-getter branch and drop the version literal. - **PR12592-F4 (Docs)** – `docs/en/introduction/concepts/incompatible-changes.md` should call out that standard DuckDB `TIMESTAMP` now generates MySQL `DATETIME(6)` instead of `DATETIME`, since that is the most common user-visible DDL change. - **PR12592-F5 / PR12592-F7 (Performance)** – `isAliasTimestamp` does a `ResultSetMetaData` lookup plus three `equalsIgnoreCase` comparisons per TIMESTAMP cell per row, and allocates a new `Calendar` per alias value. Please cache the per-column alias decision once per `ResultSet` and reuse the `Calendar`. - **PR12592-F6 (Test)** – `DuckDBSourceAndSinkTest` covers pre-epoch rows but not the two documented limitations (pre-1970 DST gap, Gregorian-cutover alias date). Please add cases for both so the legacy-getter branch is pinned against the documented behaviour. - **PR12592-F8 (Docs)** – The Javadoc on the alias path ties the design to driver 1.3.1.0; please note that metadata-based detection is version-independent and will not automatically pick up a future typed getter. If any of these were addressed in a commit I haven't seen, please point me at it; otherwise a short per-item reply (fix / won't fix with reasoning) is enough to move forward. Once the required PR Build has its own result, I'll take another pass. <!-- streview-comment:1977 --> -- 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]
