SEZ9 commented on PR #12592: URL: https://github.com/apache/seatunnel/pull/12592#issuecomment-6308105577
Thanks for the latest push. I went through `743ed3a0` against my earlier notes; since the production code is unchanged since `a18e06f0` and this push is test-only, most of my points stand as they were, with a few closed and a few concrete asks left. **Where I think we are on the previous findings** - **F3 (one-time WARN) and F8 (Javadoc tied to driver 1.3.1.0)** – these were addressed in `de8f9d02` as far as I can tell. If the Javadoc now states that the metadata-based alias detection is version-independent and will not automatically pick up a future typed getter, I consider F8 closed; a quick pointer to the updated wording would help me confirm. - **F5 / F7 (ResultSetMetaData lookup + Calendar allocation per alias cell)** – I'm fine with deferring this as a follow-up; it's Low and not a blocker. Please just drop a TODO or a short note in the PR description so it doesn't get lost. - **F1 (hardcoded scale 6/9 and `reconvert` ignoring scale)** – the description says `reconvert` is now precision-aware. Can you confirm that a DuckDB-to-DuckDB pipeline with a `TIMESTAMP_NS` source column now produces `TIMESTAMP_NS` (or at least does not silently fall back to `TIMESTAMP`) in the generated DDL, and point me at the test that pins the round trip? That was the core of F1 and I don't see it explicitly covered by the new test-only commit. - **F2 / F6 (pre-epoch alias fallback via JVM-default-zone `getTimestamp`)** – the limitation is documented now, which is good, but the new `DuckDBSourceAndSinkTest` additions cover UPDATE/DELETE rather than the two documented edge cases. Please add at least one test each for a pre-1970 DST-gap value and a pre-1582 alias date so the legacy-getter branch is pinned to the behaviour the docs describe (even if that behaviour is "value is shifted, and we accept it"). - **F4 (upgrade note)** – I still don't see the plain-`TIMESTAMP` → MySQL `DATETIME(6)` DDL change called out in `incompatible-changes.md`. That is the change most users will actually hit, so please add a line for it alongside the alias notes. **Remaining asks before I approve** 1. Confirm/point to the `reconvert` scale round-trip coverage (F1). 2. Add the two pre-epoch edge-case tests (F6). 3. Add the `TIMESTAMP` → `DATETIME(6)` entry to the incompatible-changes doc (F4). 4. Optional: TODO/note for the hot-path metadata lookup (F5/F7). Once those land I'll do a final pass. Nice work on the UPDATE/DELETE coverage — that part looks solid. <!-- streview-comment:1923 --> -- 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]
