DanielLeens commented on PR #12015: URL: https://github.com/apache/seatunnel/pull/12015#issuecomment-5674347303
Thanks for pinning down exactly what's outstanding, @SEZ9 — and thanks @luozihen for posting F1-F8 as separate comments so nothing gets clipped. Quick status check before answering: the head is still `2c5982be87e742c197143c64b8143ebff0fba2a5`, the same one my last full review (a documentation-only Javadoc commit on top of `81f97896`, `+9/-0` on `MultiTableFailureHelper.mergeOptions()`, verified by direct diff) already covered end to end. So there's no new code here for either of us to re-check — just closing the loop on the items you listed as open. F1-F8 were independently re-verified against the actual source across two of my own rounds, not taken on trust from status summaries: - **F1** — confirmed via direct diff: `ReadonlyConfig#toConfig()` is byte-for-byte identical to `dev`. The fix lives entirely in `MultiTableFailureHelper.mergeOptions()`, which I traced call-site by call-site (`MultipleTableJobConfigParser.java:746`, the Spark/Flink `SinkExecuteProcessor`s, `withMultiTableFailurePolicy`/`withFailedTables`) to confirm none of them can hit the shallow-vs-recursive-merge divergence this narrowing introduces. - **F2/F5** — `toCompiledPatternMap` builds an explicit `LinkedHashMap<Pattern, List<String>>`, so "first declared pattern wins" is guaranteed by construction rather than incidental map order; `testResolveMultiTablePrimaryKeysFirstMatchWins` asserts it. - **F3** — `validatePrimaryKeyColumns` is wired into the shared `applyPrimaryKeys` helper, so both the new multi-table-match branch and the legacy top-level `primary_keys` branch reject blank/comma-containing resolved columns before the join. - **F4** — `TablePlaceholderProcessor.replaceTablePlaceholder` runs first but only rewrites top-level String/single-element-String-List values, so the nested `multi_table_config` map is untouched until the connector's own `expandPrimaryKeyPlaceholder` runs; a missing PK/unique-key fails fast with `JdbcConnectorException`/`JDBC-12` rather than leaving a literal placeholder or falling back silently. - **F6** — `toCompiledPatternMap` compiles every declared pattern eagerly via `compilePattern` before any table matching, so an invalid regex fails fast with `JDBC-12` at factory time, not lazily at match time. - **F8** — confirmed no stale `multi-table_config` spelling remains anywhere in code, error strings, tests, or EN/ZH docs. That lines up, item for item, with what @luozihen just posted above — I'd treat those as accurate rather than needing separate re-verification of each pointer, since they match what I already found independently on this exact head. From my side there's nothing open on the current head: Ready to merge, with only the one pre-existing Low/non-blocking carryover noted in my last review (`applyFallbackPrimaryKeys` duplicating the `getPrimaryKeyColumns`/`getUniqueKeyColumns` lookups instead of delegating to them). Happy to take another look if you find something on the actual diff that contradicts this. -- 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]
