DanielLeens commented on PR #11843: URL: https://github.com/apache/seatunnel/pull/11843#issuecomment-5354563017
Hi @zhang-arvin, thanks for following up — but I want to make sure we're on the same page, because I don't think this comment reflects the actual remaining blocker. The `DM_NVARCHAR → DM_NVARCHAR2` normalization fix itself was already confirmed correct in my last review (round 3, on commit `2fb28032`, which is still the current head — I re-checked and no new commit has landed since then). That part is done and I'm not asking for further changes to it. The reason this PR is still **not ready to merge** is different: the branch currently carries 5 commits, and only the last one (`2fb28032`) touches Dameng/NVARCHAR2. The other 4 commits are unrelated — they add MySQL and DB2 test coverage (closing #10211 and #10216 respectively) and, more importantly, one of them contains a real **production** behavior change in `MySqlTypeMapper.java` (removing the `charTo4ByteLength()` multiplier for `CHAR`/`VARCHAR`/`ENUM` column-length computation) that has nothing to do with this PR's stated scope and hasn't been reviewed on its own merits. To get this merged, please: 1. Reset/rebase this branch so it contains only the DM NVARCHAR2 commit(s) — nothing from the MySQL/DB2 test-optimization work. 2. Open the MySQL and DB2 test-optimization changes as their own separate PR(s) against #10211 / #10216, so the `MySqlTypeMapper.java` precision change gets its own focused review and compatibility discussion. Once the branch only contains the DM fix, I expect this to be a quick approve — the fix itself is correct. Let me know if you'd like a hand splitting the commits out. -- 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]
