SEZ9 commented on PR #12015: URL: https://github.com/apache/seatunnel/pull/12015#issuecomment-6331300208
Thanks @luozihen, both fixes sound right. 1. Key column validation: having `toPrimaryKeyList()` reject non-string / null list elements and empty results with a JDBC-12 error that names the pattern, while keeping `validatePrimaryKeyColumns()` as the final guard, is the shape I was hoping for. Good that the new `JdbcSinkFactoryTest` cases cover both `resolveMultiTablePrimaryKeys()` and the `createSink` path. 2. REST JSON body: moving `ConfigBuilder.of(Map)` onto the same `rebuildWithLiteralKeys` helper as the config-file and shade paths closes the gap, and `testOfMapPreservesLiteralRegexOptionKeys` covering the map → `of(Map)` → `ConfigShadeUtils.decryptConfig` sequence is a solid regression test. My preference is to keep the REST commit in this PR so the literal-key behaviour is fixed in one place for all three entry points; the separate REST PR can be put on hold or closed once this lands. Remaining asks before I can approve: - Please post the CI result for d9bb1574f once it finishes. - Could you confirm whether the earlier points are covered on this head or still pending: declaration-order guarantees for the pattern map (and the matching docs wording), the interaction with the engine-level `TablePlaceholder` replacement, a regression test for the old `toConfig()` dotted-key expansion plus an engine-level IT/E2E for the new option, and the `multi-table_config` key naming. <!-- streview-comment:1932 --> -- 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]
