wgzhao commented on PR #12333: URL: https://github.com/apache/seatunnel/pull/12333#issuecomment-6119572125
Thanks @SEZ9 — I went through all five points against the current head and I agree with every one of them. None of them changes behaviour, so my plan is to land them together as one small follow-up change rather than push another commit here. Per item: **1 — whitelist drift.** Agreed, this is the one that actually matters. The set is a literal copy of the `MYSQL_*_UNSIGNED` constants in `MySqlTypeConverter` with only a Javadoc sentence tying them together, so adding a type on one side can silently drop the `unsigned` widening — or recreate the `COMMON-17` failure this PR fixes. I'll derive the set from the converter constants (strip the `" UNSIGNED"` suffix) and keep a unit test asserting every constant's base name is accepted, so drift fails CI either way. **2 — use `DATA_TYPE`.** Agreed. Both `buildColumn` implementations already read the server-normalised `DATA_TYPE`, so re-tokenising `COLUMN_TYPE` is a second, independently parsed notion of the type name. I'll change the helper to `isNumericUnsignedColumnType(String dataType, String columnType)` and keep the single-argument form only if the pure-string unit tests are clearer with it. **3 — OceanBase IT.** Agreed, and this one is on me: the PR changes `OceanBaseMySqlCatalog.buildColumn` but only `JdbcMysqlIT` got the regression case, which breaks our "update the existing IT for the connector you changed" rule. I'll mirror the four assertions into `JdbcOceanBaseMysqlIT`, since `OceanBaseMySqlTypeConverter` is a different class (it has no `BIT`/`YEAR` unsigned cases) and that is the only place its interaction with the new guard meets a real server. **4 — Javadoc wording.** Agreed. `BIT` and `YEAR` do not accept `UNSIGNED` in MySQL; they are in the set only for parity with `MYSQL_BIT_UNSIGNED` / `MYSQL_YEAR_UNSIGNED`. I'll reword the first sentence so nobody later "corrects" the list from the MySQL rule — membership itself stays as is, for the reason given in the second sentence. **5 — placement.** Agreed, and it bundles nicely with (1): moving the helper into `internal/dialect/mysql` puts it next to the constants it has to stay in step with, and `OceanBaseMySqlCatalog` can call the same method. Why not in this PR: the head is green and already carries an approving review, and a new commit would dismiss that review and re-run the full Build, while all five points are Low and behaviour-neutral. If you would rather see them here instead, say so and I'll push them onto this branch instead of opening a follow-up — happy either way. -- 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]
