SEZ9 commented on PR #10453:
URL: https://github.com/apache/seatunnel/pull/10453#issuecomment-6104663404
Thanks @wgzhao — the plan lines up with what I had in mind, and the split
works for me.
**1/2 (`maxOptionListLength` keying off the raw `column.typeName()`)** —
Agreed on passing the normalized `dataType` into the helper and deciding `isSet
= MYSQL_SET.equals(dataType)`. Please make sure the new test drives the sizing
path with a stubbed option list for a `SET UNSIGNED` column and asserts the
SET-style length (sum of members + commas), not just the type mapping.
**3 (`ENUM('...UNSIGNED...')`)** — Fine to leave this to #12333, since that
is where the producer in `MySqlCatalog.buildColumn` gets replaced. Your comment
looks like it was cut off mid-sentence after the `OceanBaseMySqlCatalog`
mention — could you finish that thought? In particular, I'd like to confirm
whether #12333 removes the need for the synthetic `SET UNSIGNED` whitelist
entry added here, or whether both are meant to coexist. A short pointer to
#12333 in the PR description would also help readers of this change.
**4 (docs + title)** — Good; please add `SET` to the JDBC MySQL source docs
alongside the MySQL-CDC update, since the shared `MySqlTypeConverter` is where
the production fix lives.
**5 (test shape)** — Please have `testConvertSetUnsigned` build the column
with the bare `SET` data type plus the unsigned flag, so the `isUnsigned()`
concatenation path is what's exercised.
**6 (new test class)** — Either fold the case into
`MySqlTypeUtilsIntTypeNarrowingTest` or keep the new class but build a real
`Column.editor()` instead of the Mockito mock, consistent with the sibling test.
**7 (bare `SET` as `columnType`/sourceType)** — Please either carry the full
option list through or adjust the comment so it doesn't claim the bare `SET` is
valid for auto-create DDL.
**8 (JDBC warn log context)** — Adding the column/table name to the
JDBC-side warn, matching the CDC-side log, is all that's needed.
Once those are pushed I'll take another look. Nothing else from my side.
<!-- streview-comment:1662 -->
--
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]
