SEZ9 commented on PR #11843: URL: https://github.com/apache/seatunnel/pull/11843#issuecomment-5552599198
Thanks @DanielLeens for the thorough round-11 verification. To answer your comment directly: yes, the head moving to `3402895b61f188ee2a4eaef37e3b0572c34f5234` is purely from rebase/reset cycles, and your independent re-diff matching the previously approved version is exactly the confirmation we needed — no new code review is required on my side. On CI, I agree with your earlier read: the two failing jobs are known unrelated flakes, so a rerun of those jobs is all that's blocking there. That said, before merge I still need the following from the earlier review scope closed out: 1. **PR11843-F1** — an explicit confirmation (a comment on the PR is fine) that leaving `reconvert()` untouched is intentional, i.e. catalog/auto-create flows are out of scope for #10635 and only the read path is being fixed. 2. **PR11843-F3** — the Dameng data type mapping in the connector documentation should list `NVARCHAR2`; right now the docs don't reflect the new support. 3. **PR11843-F4 / F6** — `testNvarchar2()` only pins the happy path (length=2). Please add boundary cases in `DmdbTypeConverterTest`: DM max width and an unset/zero length, to lock in the 4x expansion via `charTo4ByteLength` and the behavior when `typeDefine.getLength()` is degenerate. 4. **PR11843-F2** — ideally an IT/E2E case exercising `NVARCHAR2` through the real DM driver/catalog metadata path. If that's impractical for this PR, say so and we can track it separately, but I'd like that decision stated explicitly rather than left implicit. Items 1–3 are the hard asks; item 4 is negotiable with an explicit follow-up. Once those land and we get a clean rerun of the two flaky jobs, this is good to merge. Thanks again for the persistence across eleven rounds. <!-- streview-comment:796 --> -- 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]
