SEZ9 commented on PR #12015: URL: https://github.com/apache/seatunnel/pull/12015#issuecomment-5595019491
@luozihen Thanks for the detailed status — here's my response point by point. **F1 / F4 — direction looks right; closure pending verification.** Reverting `ReadonlyConfig#toConfig()` to `ConfigFactory.parseMap(confData)` and scoping the change to `MultiTableFailureHelper.mergeOptions()` is exactly what I was hoping for, and your explanation of why `TablePlaceholderProcessor.replaceTablePlaceholder` leaves the nested map untouched is consistent with the intent. I'll confirm both against the actual changes on the latest head before marking them resolved. **F2 / F5 — yes, please make it stricter.** Relying on Jackson happening to produce a `LinkedHashMap` is an implementation detail rather than a contract, and since the docs promise "first pattern in declaration order wins", I'd like that pinned down in code: - Make the ordering explicit where the option is read/iterated (e.g. copy into a `LinkedHashMap`, or type it so order is guaranteed by construction). - Add a unit test declaring several patterns that match the same table and asserting the first-declared one wins. **F3 — please handle in this PR.** Since the resolved key columns are comma-joined into `PRIMARY_KEYS` and end up in generated SQL, please validate each resolved column as a plain identifier (at minimum reject empty names) before joining, with a small unit test for a rejected value. **F6 — please also do this in this PR.** Lazy failure at match time means a bad regex only surfaces once data flows for a matching table. Please compile every pattern during config validation and raise `JDBC-12` up front, with a unit test for an invalid pattern. **F7 — two pieces:** - The regression test for the restored `toConfig()` dotted-key expansion directly protects the F1 revert, so please add it in this PR. - The engine-level IT/E2E for the new option can be a follow-up; please note that in the PR description. **F8 — please rename.** The existing JDBC sink options are snake_case, so the mixed hyphen/underscore in `multi-table_config` will stand out to users. A fully snake_case name would fit the convention; please update the docs and tests to match. I understand the current name came from the issue discussion, but I'd rather keep the option surface consistent. Summary for this PR: F2/F5 explicit ordering + test, F3 identifier validation + test, F6 eager regex validation + test, F7 `toConfig()` regression test, F8 rename. F1/F4 to be confirmed against the diff; F7's IT/E2E can be a follow-up. Ping me once pushed and I'll take another pass. <!-- streview-comment:916 --> -- 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]
