wgzhao commented on PR #10453:
URL: https://github.com/apache/seatunnel/pull/10453#issuecomment-5690624881
Thanks @SEZ9 and @DanielLeens for the thorough passes. Follow-up pushed as
`04abfb922`.
## Answers to the two open points from @SEZ9's review of `7136a0a956d1`
**(1) Is the unconditional `builder.dataType(dataType)` in `MySqlTypeUtils`
intentional, and does it change behaviour for other types?**
Yes, and it is a behavioural no-op. Two reasons, both verifiable in the
current head:
- `dataType` was already being set unconditionally for every column before
this PR: `BasicTypeDefine.builder()...dataType(column.typeName())` at the top
of `convertToSeaTunnelColumn`. The new line only changes the *casing* of the
value that was already being written.
- The only reader of that field is `MySqlTypeConverter.convert()`, which
does `typeDefine.getDataType().toUpperCase()` on entry. So the raw-vs-uppercase
difference is normalized downstream regardless.
It was still worth flagging, so the synthesis path is now documented
explicitly rather than left implicit — see the new comment on
`MYSQL_SET_UNSIGNED`.
**(2) Can other `<type> UNSIGNED` variants (e.g. `ENUM`) also appear from
Debezium, and would they need the same normalization?**
Yes, and that one is genuinely still open. It is the same root cause:
`MySqlCatalog.buildColumn()` derives `unsigned` from
`columnType.toLowerCase(Locale.ROOT).contains("unsigned")`, a substring match
over the whole `COLUMN_TYPE` literal. A column declared as
`ENUM('...unsigned...')` therefore still synthesizes an unhandled `ENUM
UNSIGNED` and still falls through to `default -> COMMON-17`.
I deliberately did not fold that into this PR. Making the derivation
type-aware (only genuinely numeric types can be unsigned) changes behaviour for
every non-numeric type, which is a much larger blast radius than this
SET-specific fix — it belongs in its own change with its own catalog-level
regression test. I'll follow up separately.
## What changed in `04abfb922`
All of these come from the non-blocking findings in the reviews:
- Added a comment on `MYSQL_SET_UNSIGNED` explaining that it is a locally
synthesized key, not a real MySQL type, and when the catalog produces it
(#10451).
- Added a `log.warn` in the converter's `SET UNSIGNED` branch. This is the
path that actually triggers #10451, and unlike the numeric `*_UNSIGNED` cases
it converted silently.
- Normalized `ColumnType` along with `DataType` on the CDC path.
`ColumnType` is what surfaces as `Column#getSourceType()`, and
`MysqlCreateTableSqlBuilder` emits it verbatim into generated auto-create DDL —
so leaving it as `SET UNSIGNED` would have produced DDL that MySQL rejects.
This also means `MySqlTypeUtilsTest` now asserts `"SET"` instead of `"SET
UNSIGNED"`; the old assertion encoded the leak as expected behaviour.
- Replaced the duplicated `ENUM` entry with `SET` in both the `docs/en` and
`docs/zh` MySQL-CDC data type mapping tables.
Not changed, as explained above: the `MySqlCatalog` unsigned-derivation root
cause.
## Verification
```
spotless:apply clean (no reformatting of the change)
connector-jdbc 986 tests, 0 failures
connector-cdc-mysql 37 tests, 0 failures
```
`connector-jdbc` reports 4 errors in that run, all from Testcontainers
classes (`KingbaseCatalogContainerTest`, `KingbaseDialectContainerTest`,
`OracleCatalogTest`, `OracleDialectTest`) failing with `Previous attempts to
find a Docker environment failed` — this machine has no Docker, and those cases
do not touch the MySQL type conversion paths changed here.
This is a new head, so the `Build` check will need to re-run.
--
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]
