DanielLeens commented on PR #11843: URL: https://github.com/apache/seatunnel/pull/11843#issuecomment-5553495379
Thanks @SEZ9 — appreciate you being explicit about which items are hard asks vs. negotiable rather than leaving it implicit. **F1 (`reconvert()` untouched) — confirmed intentional, with evidence.** I checked `DmdbTypeConverter.reconvert()` directly on the current head: the write-direction mapping for any SeaTunnel `STRING_TYPE` column always emits `DM_VARCHAR2` (`DmdbTypeConverter.java:431-432`, `builder.dataType(DM_VARCHAR2)`), regardless of whether the original DM source column was `NCHAR`/`NVARCHAR`/`NVARCHAR2`/`VARCHAR`/`VARCHAR2`. There's no `NVARCHAR2` branch in `reconvert()` today, and this PR doesn't add one. That's consistent with #10635's scope: the bug is that reading an existing `NVARCHAR2` column crashes catalog/schema discovery; auto-create-table/DDL-generation for new columns was never broken and isn't part of this fix. So yes — leaving `reconvert()` untouched is intentional, not an oversight. **F3 (docs) — agreed, elevating to blocking.** This is a genuinely new, user-visible connector capability (DM `NVARCHAR2` columns are now readable), and per the doc-parity convention already followed for the sibling `NCHAR` fix, `docs/en/connectors/source/Jdbc.md` and the `docs/zh` counterpart should list it. I'd flagged this as non-blocking/Low in my round-11 pass, but a doc line is small enough to just require before merge rather than defer — updating my position on that. **F4/F6 (boundary tests) — partial agreement.** A max-width case is reasonable and cheap to add, no objection there. For the degenerate/unset-length case: adding a test here would only pin the *current* behavior, which is the pre-existing `"NVARCHAR2(null)"`-style invalid-DDL gap shared by all five string arms (`CHAR`/`VARCHAR2`/`NCHAR`/`NVARCHAR`/`NVARCHAR2` — my round-11 Issue 1), not something this PR introduces. I'd rather not have this test file lock in a known-broken shared behavior as "expected" — that reads as endorsing it. I'd suggest tracking that case in the follow-up that adds the length guard across all five arms, so the test asserts the *fixed* behavior instead. Happy to be overruled if you still want a documented-limitation test now. **F2 (real-driver IT/E2E) — agreed, negotiable/follow-up.** There's no DM testcontainer in the existing JDBC E2E suite for this connector today, so I'd track this as a separate follow-up rather than block this fix on standing up new DM E2E infra. Net: I'm folding F1 (confirmed) and F3 (docs) into the blocking list along with the pending clean CI run; the F4/F6 max-width case is a welcome addition, the degenerate-length case I'd defer to the guard follow-up; F2 tracked separately. @zhang-arvin, once the doc line and the max-width test land I'm still at approve on the code itself. -- 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]
