luozihen commented on PR #12015:
URL: https://github.com/apache/seatunnel/pull/12015#issuecomment-5755023969
@SEZ9 Thanks — to stop the truncation we keep hitting, I have consolidated
every open item into the
"Review status (F1-F8)" section of the PR description, with the file and
test name for each.
Could you use that section as the source of truth for the next pass?
One-line index below, in case
it helps:
- F1 — `ReadonlyConfig#toConfig()` is unchanged
(`ConfigFactory.parseMap(confData)`,
`ReadonlyConfig.java:76`) and `ReadonlyConfig.java` is not in the PR diff
at all; the parsing fix
is scoped to `ConfigShadeUtils.processConfig` and
`MultiTableFailureHelper#mergeOptions`.
- F2 / F5 — explicit `LinkedHashMap<Pattern, List<String>>` in
`toCompiledPatternMap`, so the order
is guaranteed by construction, plus
`JdbcSinkFactoryTest#testResolveMultiTablePrimaryKeysFromHoconFirstMatchWins`,
which uses a real
HOCON string with non-alphabetical overlapping patterns; both docs state
the same rule.
- F3 — `validatePrimaryKeyColumns` rejects blank names and commas with
`JDBC-12`, and the resolved
names are quoted through the dialect when SQL is generated, so those two
are the only inputs that
cannot be represented safely.
- F4 — the order is documented in
`docs/{en,zh}/introduction/configuration/sink-options-placeholders.md` and
pinned by
`JdbcSinkFactoryTest#testPlaceholderOrderBetweenEngineAndConnectorPasses`.
- F6 — patterns are compiled once up front (`compilePattern` + bounded
cache) and an invalid
pattern raises `JDBC-12` with the offending pattern in the message; three
tests cover it.
- F7 — the `toConfig()` half is not open: the regression test already exists
in this PR's diff
(`ReadableConfigTest#testToConfigPreservesDottedKeyExpansion`,
`seatunnel-api` line 383). The E2E
half is now updated exactly as you asked in `2656c4b6a`: two dedicated
source tables
(`source.mtc_table1` / `source.mtc_table2`) whose key columns are `NOT
NULL`, the `.conf` pointed
at them, and the shared `source.table1` / `source.table2` left untouched.
- F8 — confirmed: `multi_table_config` is used consistently in
`JdbcSinkOptions`, both docs, the E2E
config and all tests.
The MySQL/OceanBase DDL gap is not fixed here, as you suggested. I will open
a separate issue for it
(emit `NOT NULL` for key columns plus a unit test in
`MysqlCreateTableSqlBuilderTest`) and link it
here once it is created, then follow up with a PR.
`jdbc-connectors-it-part-1` on `2656c4b6a` is the check to watch; I will
paste the link here once it
finishes.
--
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]
