wgzhao commented on PR #10453:
URL: https://github.com/apache/seatunnel/pull/10453#issuecomment-6096813860
Thanks @SEZ9 for the careful pass and for tracing both convergence points
again.
Short version: I agree with 1/2, 5, 6, 7 and 8 and will fix them here; 4
I'll fix as well (docs + title); 3 stays in #12333, which exists precisely for
that root cause. Details:
**Issue 1/2 (maxOptionListLength keys off the raw type name)** — Agreed, and
thanks for catching the interaction with the sizing block that #12355 added.
The branch normalizes `dataType`, but the helper still decides SET-vs-ENUM from
`column.typeName()`, so a `SET UNSIGNED` column ends up sized like an ENUM.
Fix: pass the normalized name in (`maxOptionListLength(column, dataType)` with
`isSet = MYSQL_SET.equals(dataType)`) and exercise the sizing path with a
stubbed option list in the test.
**Issue 3 (ENUM('...UNSIGNED...'))** — This one deliberately stays out of
this PR: it *is* the reason #12333 exists. Whitelisting `MYSQL_ENUM_UNSIGNED`
here would add a second synthetic key while leaving the producer,
`MySqlCatalog.buildColumn`'s `columnType.toLowerCase().contains("unsigned")`,
in place. #12333 replaces that with
`JdbcCatalogUtils.isNumericUnsignedColumnType(...)`, applies it to both
`MySqlCatalog` and `OceanBaseMySqlCatalog`, and covers the shape end-to-end in
`JdbcMysqlIT.testSetAndEnumColumnWithUnsignedWordInValueList()`:
`SET('REAL_AS_FLOAT','NO_UNSIGNED_SUBTRACTION')` and `ENUM('unsigned','other')`
map to STRING, while a genuine `INT UNSIGNED` control still widens. This PR
remains the defensive arm: if a synthetic `SET UNSIGNED` is supplied from
outside, it converts instead of throwing.
**Issue 4 (JDBC docs / title)** — Agreed. The behavioural change is in the
shared JDBC `MySqlTypeConverter`, so I'm adding `SET` to the STRING row in
`docs/en/connectors/source/Mysql.md` and `docs/zh/connectors/source/Mysql.md`,
and updating the PR title so it names the JDBC MySQL source rather than only
MySQL-CDC.
**Issue 5** — Agreed; the fixture now builds `dataType("SET")` +
`unsigned(true)` with a realistic `columnType("SET('...')")`, so the test
drives the concatenation branch that produced the synthetic key in #10451. I
kept one explicit `dataType("SET UNSIGNED")` case, labelled as guarding direct
callers only.
**Issue 6** — Agreed; the case moved into
`MySqlTypeUtilsIntTypeNarrowingTest` in the same package and builds a real
`Column` via `Column.editor()...create()`, with the Mockito mock and the
`MySqlSourceConfigFactory` gone. That also matches the repo's rule about
extending the existing test class rather than adding a parallel one.
**Issue 7** — Agreed that the comment overstates what the line guarantees.
On the DDL path the source type is finalized by
`SeatunnelDDLParser.toSeatunnelColumnWithFullTypeInfo` ->
`getSourceColumnTypeWithLengthScale`, which #12355 (merged) now renders as
`SET('m1','m2')` from `enumValues()`; this branch only strips the synthetic
suffix so it cannot leak. I reworded the comment (and the test's intent
comment) to claim only that, and left the remaining shapes to #12354 rather
than duplicating the option-list rendering here.
**Issue 8** — Agreed; the JDBC-side warn now carries the column name and the
full column type, matching the CDC-side message.
I'll push the fixes as a single commit and re-request review.
--
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]
