SEZ9 commented on PR #12592: URL: https://github.com/apache/seatunnel/pull/12592#issuecomment-6030280480
Thanks @hutiefang76, the typed-read change in eb693c5 and the dev sync in 743a440 (on top of eb2eb1d) look like the right direction, and the DST-gap / cutover regression through both table_path and query is what I wanted to see for standard TIMESTAMP. A few earlier points are still open or only partly covered: 1. **Silent demotion on SQLException.** Caching the unsupported capability per result-set column solves the per-row exception cost, but I don't see anything about logging. Please emit at least a one-time log when a column is demoted to the plain getter, and ideally only demote on the specific unsupported-conversion failure rather than any SQLException, so a transient error doesn't permanently downgrade the column for the rest of the result set. 2. **Alias precision.** TIMESTAMP_S/MS/NS still map to TIMESTAMP with no scale. Since downstream auto-DDL derives fractional precision from that, could you carry an explicit scale (0/3/9) for those aliases, or explain why that isn't feasible here? 3. **Alias fallback choice.** I understand the driver rejects typed reads for the aliases, but that doesn't force `getTimestamp`: a text read plus parse would keep the previously lossless behaviour. If you've tested that and it fails with 1.3.1.0, please note that in the PR; otherwise I'd prefer the text read over documenting a known-lossy fallback. 4. **Incompatible-changes wording.** The note still scopes the change to "table columns", while your regression shows query mode goes through the same path. Please adjust the note to cover both modes. 5. **Alias test coverage.** The new regression covers standard TIMESTAMP only. Please add an assertion for at least one alias (e.g. TIMESTAMP_MS) so the fallback behaviour the docs warn about is pinned by a test and we notice if a future driver starts accepting typed reads. 6. **Cache lifetime / key and comments.** Still open: the converter holds a strong reference to the last ResultSet and keys on identity. A weak reference, or clearing the cache when a new ResultSet is seen, would be enough. Please also expand the readTimestamp Javadoc and add a short comment on the per-ResultSet cache describing its contract and single-thread assumption. 7. **Docs cross-link.** Please link the new DuckDB timestamp paragraph to its incompatible-changes section, and phrase the alias limitation as a runtime probe against the bundled driver version rather than fixed behaviour. Once the above are in, ping me and I'll take another look. <!-- streview-comment:1576 --> -- 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]
