SEZ9 commented on PR #12015:
URL: https://github.com/apache/seatunnel/pull/12015#issuecomment-6137986285
Thanks for keeping the branch in sync with `dev`. Since the only new commit
on this head is the merge (`e761f0c5b`) and the PR-owned files are reported
byte-identical to `00acc23a5`, my earlier review points stand as-is. Here's
where each one lands and what I still need:
**F1 – `ReadonlyConfig.toConfig()` dotted-key semantics (HIGH).** The latest
re-review states that `ReadonlyConfig#toConfig()` is no longer part of the diff
and that the regex-key handling now lives in `ConfigShadeUtils.processConfig`
and `MultiTableFailureHelper.mergeOptions` instead. If that holds, this
resolves my main compatibility concern, and dotted-key expansion for all other
callers stays as before.
**F2 / F5 – "first pattern in declaration order wins" (HIGH / MEDIUM).**
Still open. The option is a `Map<String, Object>`, so declaration order is not
guaranteed unless the map is explicitly order-preserving end to end (HOCON
parse → `ReadonlyConfig` → `resolveMultiTablePrimaryKeys`). Please either (a)
show where an insertion-ordered map is enforced and add a unit test with two
overlapping patterns asserting the first one wins, or (b) change the contract
(e.g. reject overlapping matches, or document that order is undefined) in both
`JdbcSinkOptions` and `docs/en/connectors/sink/Jdbc.md`.
**F3 – resolved key columns flow into generated SQL (MEDIUM).** Still open.
Before the comma-joined value is written into `PRIMARY_KEYS`, please validate
each column against the upstream table schema (or at minimum a strict
identifier pattern) and fail with a clear error instead of letting arbitrary
strings reach SQL generation.
**F4 – connector-level `${primary_key}` / `${unique_key}` vs engine-level
`TablePlaceholderProcessor` (MEDIUM).** The re-traced runtime path (engine
placeholders first, then connector expansion) is helpful. Please add a short
note to the docs describing this ordering and what happens when the upstream
table has no PK/unique key (empty expansion vs. error), plus a test covering
that case.
**F6 – regex compile validation (MEDIUM).** Please make sure an invalid
pattern fails at `createSink` time with the new JDBC-12 error code (not a raw
regex exception from the cache), and add a unit test for it.
**F7 – tests (MEDIUM).** If F1 is indeed resolved by not touching
`ReadonlyConfig`, the regression test there is no longer required. Still
needed: an IT/E2E running a real multi-table JDBC sink job that exercises
`multi_table_config.primary_keys` with at least one matched and one unmatched
table.
**F8 – option naming (MEDIUM).** The latest text refers to the option as
`multi_table_config.primary_keys` (snake_case). Please confirm the actual key
in `JdbcSinkOptions` and the docs match that spelling and that the earlier
`multi-table_config` mixed form is gone everywhere.
Once F2/F5, F3, F6 and the IT from F7 are addressed, I'm happy to take
another full pass.
<!-- streview-comment:1723 -->
--
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]
