SEZ9 commented on PR #12592: URL: https://github.com/apache/seatunnel/pull/12592#issuecomment-6246084463
Thanks for the follow-up in 32c2913955, @hutiefang76. The DST-gap / Gregorian-cutover reproduction with the 1.3.1.0 driver, the switch to timestamp-text binding for all local TIMESTAMP writes, and the seven-row sink regression against independent DuckDB SQL literals sound like the right direction for the sink side; I'll verify them against the diff. The scale-0 migration note and the downstream-converter check for the scale-6 DDL also look like they address the upgrade-note point, pending a look at the actual doc change. A few items from the earlier review aren't mentioned in your update, so could you confirm their status in 32c2913955 (or say if they're deferred): 1. **Alias read path** — you note the historical alias-read limitations "remain documented". Does the read side still use the JVM-default-zone `getTimestamp` fallback for pre-epoch alias values, and is there now a test pinning that branch against the documented pre-1970 DST-gap and cutover behaviour? If this is intentionally deferred, a line in the PR description would help reviewers not assume the new sink regression covers it. 2. **One-time WARN** — does it still fire for every job that reads any alias column and hard-code the driver version? If so, I'd prefer it be gated on an actual pre-epoch alias value being read, or drop the version string so it doesn't go stale. 3. **Per-cell metadata lookup** — does `isAliasTimestamp` still do a `ResultSetMetaData` lookup and the `equalsIgnoreCase` checks, plus a `Calendar` allocation, for every alias value on the read path? Caching the per-column decision once per ResultSet would be enough. 4. **DuckDB-to-DuckDB TIMESTAMP_NS** — with scale 6/9 now emitted, does `reconvert` still ignore scale so TIMESTAMP_NS round-trips to plain TIMESTAMP silently? A one-line WARN or a note in the incompatible-changes doc would be enough for this PR. 5. **Javadoc on the alias path** — a short sentence stating that detection is metadata-based and version-independent (and won't pick up a future typed getter) would close that item. Since the MySQL container E2E hasn't been run locally, please also note that in the PR description so the merger knows to rely on CI for it. Happy to take another pass once those are confirmed or adjusted. <!-- streview-comment:1849 --> -- 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]
