wgzhao commented on PR #12355:
URL: https://github.com/apache/seatunnel/pull/12355#issuecomment-5851881540
Thanks for taking it over - here are the pointers, all on the unchanged head
`4f6663a9f` (Build is green, so nothing has moved under you).
**1. Does the derived length actually reach `Column.columnLength`?** Yes.
`MySqlTypeUtils.java:165-168` sets `builder.length(...)`/`precision(...)` from
`maxOptionListLength` (`:189`, with the unquote/`''`-collapse helper at
`:212`), and `MySqlTypeConverter`'s `MYSQL_ENUM`/`MYSQL_SET` case copies
`typeDefine.getLength()` into `builder.columnLength(...)`
(`MySqlTypeConverter.java:247-253`). That is why the fix lives in the
conversion rather than in the listener: `reSourceType` only replaces the string
and never touches `columnLength`, so a listener-only fix would still leave
heterogeneous sinks reading the bookkeeping value through `reconvert`. The test
assertions are on `getColumnLength()` of the column the parser returns, i.e.
the same field the sink path reads - not just `getSourceType()`.
**2. Test cases.** All in
`CustomMySqlAntlrDdlParserTest#testParseAlterTableAddSetAndEnumColumnKeepsOptionList`;
the DDL that creates each shape is at `:120-126` and the assertions are:
- multi-character members - `ENUM('active','inactive')`: `:150` (sourceType)
and `:152` (`getColumnLength()` == 8)
- single-option `SET('only')`: `:158` and `:159` (== 4)
- embedded comma plus escaped quote - `SET('a,b','it''s')`: `:165` (exact
round-trip of the text) and `:167` (== 8)
- `CHARACTER SET` clause - DDL at `:126`, assertions at `:174-175`, with the
rationale comment at `:169-171`
- the two baseline shapes: `SET('a','b','c')` `:137` (== 5), `ENUM('x','y')`
`:143` (== 1)
On the charset case specifically: it is pinned, and the behaviour it pins is
that the rebuilt expression carries the option list only - `ENUM('x','y')
CHARACTER SET utf8mb4` comes back as `ENUM('x','y')`, so the sink applies its
own default charset for the added column. That is pre-existing behaviour, not
something this PR changes, and I called it out in the earlier reply
(https://github.com/apache/seatunnel/pull/12355#issuecomment-5806847189). If
you would rather the clause survived into the generated DDL, the parser does
keep `charsetName` on the column so it is a small addition - say the word and I
will push it, otherwise it stays as documented.
For convenience, the four-point answer is
https://github.com/apache/seatunnel/pull/12355#issuecomment-5806847189 and the
original per-item mapping is
https://github.com/apache/seatunnel/pull/12355#issuecomment-5787984776 (that
one is a conversation-level comment, which is why it did not show up in the
review thread).
**3. Javadocs** - both are as described; if your read differs, inline
comments are welcome and I will turn them around quickly.
--
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]
