SEZ9 commented on PR #12015: URL: https://github.com/apache/seatunnel/pull/12015#issuecomment-6306844335
Thanks @luozihen for the follow-up on `82fc5e61a`. The thread already has pointers for the first few of my earlier points, so I won't re-ask those: - **F1 / F7 (`ReadonlyConfig.toConfig()` round-trip):** `ReadonlyConfig.java` unchanged, with `ReadableConfigTest#testToConfigPreservesDottedKeyExpansion` pinning the behaviour and the regex-key fix moved into `ConfigBuilder.rebuildWithLiteralKeys` / `MultiTableFailureHelper.mergeOptions`. - **F2 / F5 (declaration order):** `LinkedHashMap` in `toCompiledPatternMap`, covered by `JdbcSinkFactoryTest#testResolveMultiTablePrimaryKeysFromHoconFirstMatchWins`, with the en/zh docs stating the same rule. - **F3 (identifier validation):** `validatePrimaryKeyColumns` running before the comma-join, covered by `testFactoryContextWithMultiTableConfigInvalidColumnFails`. - **F4 (placeholder interaction):** documented in `Jdbc.md` and `sink-options-placeholders.md`. I'll check those against the diff on this head. A few items are still open from my side: 1. **F6 (regex validation):** Please confirm that an invalid user-supplied pattern fails with `JDBC-12` when the pattern is compiled, and point me to the test covering it. 2. **F7 (engine-level coverage):** Could you link the E2E case/config that exercises the new option through the engine rather than only the factory? 3. **F8 (option key naming):** Is the user-facing key still `multi-table_config`? If so, I'd still like it aligned with the JDBC sink's snake_case convention before merge; if it has already been renamed, just tell me the final key. Once those are clear, I'm happy to move this forward. <!-- streview-comment:1912 --> -- 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]
