SEZ9 commented on PR #12015:
URL: https://github.com/apache/seatunnel/pull/12015#issuecomment-5642818292
@luozihen thanks for the detailed per-item status — this looks like the
right direction.
- **F1** — Good. Keeping `ReadonlyConfig#toConfig()` unchanged and scoping
the fix to `MultiTableFailureHelper.mergeOptions()` removes the cross-cutting
compatibility risk. One ask: since `mergeOptions()` now merges the option maps
directly, could you confirm (ideally with a test) that a dotted key present in
both the shared options and a per-table override resolves the same way as
before the PR?
- **F2 / F5** — The `LinkedHashMap<Pattern, List<String>>` in
`toCompiledPatternMap` plus `testResolveMultiTablePrimaryKeysFirstMatchWins`
addresses the core concern. The remaining question is upstream of that method:
the map that `toCompiledPatternMap` receives must itself preserve HOCON
declaration order, otherwise the `LinkedHashMap` only preserves an
already-scrambled order. Could you point me to where that is guaranteed, or add
a test that declares two overlapping patterns in non-alphabetical order in a
real config string and asserts the first-declared one wins?
- **F3** — Looks good. Rejecting blank names and names containing a comma in
`validatePrimaryKeyColumns` is the minimum needed to keep the join safe, and
`testFactoryContextWithMultiTableConfigInvalidColumnFails` covers it.
- **F4** — Your comment appears to be cut off mid-sentence at the
`TablePlaceholderProcessor` part, so I can't see the conclusion. Could you
re-post the rest? Specifically: which expansion runs first, and what happens if
a user writes `${primary_key}` in `multi_table_config.primary_keys` for a table
whose catalog has no primary key — does the connector fall back, fail fast, or
leave the literal placeholder in `PRIMARY_KEYS`?
Items not covered in your update:
- **F6** — Is there explicit validation of user-supplied regex patterns at
factory time, mapped to the JDBC-12 error code, with a test for an invalid
pattern?
- **F7** — With F1 narrowed, the `toConfig()` regression test is less
critical, but an IT/E2E exercising the new multi-table option end-to-end
through the engine (not only the factory unit tests) would still be valuable.
Is one included?
- **F8** — Your comment consistently uses `multi_table_config`; please
confirm the option key in `JdbcSinkOptions` and the docs is
`multi_table_config` throughout and the mixed `multi-table_config` spelling is
gone.
Once F4 and F6–F8 are confirmed, I'm happy to take another pass.
<!-- streview-comment:977 -->
--
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]
