wgzhao commented on PR #12355:
URL: https://github.com/apache/seatunnel/pull/12355#issuecomment-5787984776
Thanks for the detailed pass - all five are addressed in `4f6663a9f`.
**Issue 1 (Medium)** - fixed in `MySqlTypeUtils.convertToSeaTunnelColumn`,
the place you suggested: `SET` / `ENUM` now derive their length from
`Column#enumValues()` the way the existing `TINYINT` case re-derives the type
it needs. Members are unquoted (and `''` collapsed) before measuring; the
length is the longest member for `ENUM` and the members plus their separators
for `SET`. The regression test now asserts `getColumnLength()` alongside the
type, so the bookkeeping value cannot come back unnoticed: `ENUM('x','y')` ->
1, `ENUM('active','inactive')` -> 8, `SET('a','b','c')` -> 5, `SET('only')` ->
4, `SET('a,b','it''s')` -> 8.
I put it there rather than in the listener on purpose: the same bookkeeping
value reaches sinks that reconvert from either entry point, so deriving it once
at the conversion keeps the schema-change path and any other caller consistent
instead of fixing only the path that has an event.
**Issue 2** - the test Javadoc now matches the production one: the field
emitted verbatim into auto-create DDL is `Column#getSourceType()`, and the base
`getSourceColumnTypeWithLengthScale` renders the bookkeeping length as `SET(5)`
/ `ENUM(1)`.
**Issue 3** - added exactly the case you called out:
`ENUM('active','inactive')` with a `getColumnLength()` assertion, plus a
single-option `SET('only')` whose bookkeeping length is 1, so a wrong fallback
cannot pass for the right reason.
**Issue 4** - the verbatim join is pinned for the boundary shapes:
`SET('a,b','it''s')` has to round-trip as that exact text (embedded comma and
doubled-quote escape), and the length assertion covers the unescaped members
plus the separator. For `CHARACTER SET` I probed the parser and pinned the
behaviour rather than guessing: `ENUM('x','y') CHARACTER SET utf8mb4` rebuilds
to `ENUM('x','y')`, i.e. the charset clause is not part of the rebuilt
expression and the sink applies its own default for the added column. That is
pre-existing behaviour rather than something this PR changes, but it is now
explicit in the test. If you would rather the clause survived into the
generated DDL, say so - the parser does keep `charsetName` on the column, so it
is a small addition, and I am happy to do it here or leave it in #12371.
**Issue 5** - the override's Javadoc now spells out
`org.apache.seatunnel.api.table.catalog.Column#getSourceType()`, consistent
with how the rest of that file disambiguates the two `Column` types.
Locally the module suite is 37/38 with the only failure being
`MySqlSchemaTest` on this machine's JDK 23 (`Mockito`/Byte Buddy), which fails
identically without these changes; CI runs it on 8/11. The new head is
`4f6663a9f` and CI is re-running on it - I will not push again while it is
under 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]
