DanielLeens commented on PR #11843:
URL: https://github.com/apache/seatunnel/pull/11843#issuecomment-5379383066
Hi @zhang-arvin, thanks for the follow-up, but I want to flag a mismatch
before we go further: I just re-checked the branch and the head is still
`bd840d0486` (`bd840d04867697e39962ed56b880d6c98bf80036`) — the same commit my
round-5 review (2026-08-21T14:08:59Z) was based on. I don't see any new commit
on top of it, so there's nothing new for me to re-review yet.
To be precise about what's still open: `DmdbTypeConverter.java:209` already
builds `sourceType` from `DM_NVARCHAR2` (that part has been correct since round
3/round 4). The one remaining blocker is that the pre-existing, untouched
`testNvarchar()` test (`DmdbTypeConverterTest.java:349-362`) still asserts the
raw input literal `"nvarchar(2)"`, while the actual normalized output is
`"nvarchar2(2)"` — so it fails deterministically, confirmed by the fork CI run
on this exact commit (`expected: <nvarchar(2)> but was: <nvarchar2(2)>` on all
4 lanes).
My recommendation (Option A from round 5) is a one-line fix in the test
itself: update `testNvarchar()`'s assertion to expect the normalized value, e.g.
```java
Assertions.assertEquals(
String.format("nvarchar2(%s)", typeDefine.getLength()),
column.getSourceType().toLowerCase());
```
This keeps the production code (and the round-3 fix for the DM→DM
auto-create-table path) untouched and just brings the sibling test in line with
it. Option B (changing the production code back to preserve the original type
name) also works but reintroduces the round-1 inconsistency versus the
`VARCHAR`/`VARCHAR2` precedent, so I'd avoid it unless there's a reason to
prefer it.
Once you push that one-line change, please ping me again and I'll re-review
the new head right away. Thanks for sticking with this through several rounds —
we're very close.
--
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]
