DanielLeens commented on PR #11843:
URL: https://github.com/apache/seatunnel/pull/11843#issuecomment-5412548220
Thanks for the fresh pass, @SEZ9. I re-checked all four points against the
current head (`f413fea7ed20`) directly in the source before replying.
**Issue 1 (null/zero-length -> `NVARCHAR2(null)`):** Confirmed the gap
exists, but it's not something this PR introduces or worsens — the same
unguarded `String.format("%s(%s)", ..., typeDefine.getLength())` shape is
shared by `DM_CHAR`/`DM_CHARACTER`, `DM_VARCHAR`/`DM_VARCHAR2`, and the
pre-existing `DM_NCHAR`/`DM_NVARCHAR` arm this PR extends (that arm predates
this PR, added by #11860). This is exactly the point I raised and dispositioned
as Low/informational/pre-existing/out-of-scope back in round 5 of this thread.
If we want to guard it, that belongs in a follow-up that fixes the whole
converter's char-family arms consistently, not a one-off carve-out for
`NVARCHAR2` alone — happy to file that as a separate issue if you'd like to
pick it up.
**Issue 2 (other DM type-resolution paths / reconvert round-trip):** I don't
think this holds up — I checked `DmdbTypeMapper.java` (the query-based,
`ResultSetMetaData`-driven path you're describing) directly:
`mappingColumn(ResultSetMetaData, int)` builds a `BasicTypeDefine` from
`metadata.getColumnTypeName(colIndex)` and calls straight into
`mappingColumn(BasicTypeDefine)`, which is
`DmdbTypeConverter.INSTANCE.convert(typeDefine)` — the exact same converter and
the exact same `case DM_NVARCHAR2:` arm this PR adds. There's no second,
independent type-resolution path for DM; query-based sources go through this
converter too. So a plain `query` DM source with an `NVARCHAR2` column is
covered by this fix.
**Issue 3 (`toLowerCase()` masks exact casing) and Issue 4 (naming nit):**
Both fair observations in isolation, but both are pre-existing conventions in
this file, not something new introduced here — every sibling test
(`testConvertChar`, `testNvarchar`, `testConvertNchar`) already uses the same
`.toLowerCase()` comparison, and `testNvarchar2` follows the same naming
pattern as the untouched `testNvarchar` right next to it (the `testConvertXxx`
naming lives on different, older tests in the same file, so the convention was
already mixed before this PR). I'd treat these as candidates for a broader
test-hygiene pass across the file rather than blockers on this specific fix.
None of these change my merge recommendation — I confirmed my prior approval
(round 9, head `f413fea7ed20`) still stands: no blocking issue in this PR's own
diff.
One CI update since round 9: the fork's `Build` run on this exact head
(`32733264668`) has now completed and shows `failure`, but I pulled the actual
failing job log (`updated-modules-integration-test-part-3`) rather than
trusting the red X — the DM-related unit tests all pass, and the real failure
is a Maven Central network timeout resolving an unrelated dependency: `Could
not transfer artifact com.google.code.gson:gson:pom:2.13.1 ... Connection timed
out (Read failed)` while resolving `connector-milvus`'s transitive deps for
`connector-jdbc-e2e-part-2`. That's an infra flake, not something in this diff
— a rerun of that job should clear it.
--
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]
