SEZ9 commented on PR #12015: URL: https://github.com/apache/seatunnel/pull/12015#issuecomment-6258694329
Thanks for the update, and thanks to davidzollo for the +1. Before I sign off on head `231cbdea588a499c88d970ee6c1e031984e48f9b`, I'd like to tie the earlier review points back to the actual changes, since I haven't verified them against the diff myself: - **F1 (`ReadonlyConfig.toConfig()` dotted-key semantics):** could you point me to the change that keeps the `toConfig()` behavior unchanged for existing callers, and to the regression test that locks the dotted-key lookup? - **F2 / F5 (first pattern in declaration order wins):** could you point to where the declaration order is preserved for the pattern map, and to the test that parses real HOCON with non-alphabetical patterns and asserts the first match wins? - **F8 (option key naming):** which spelling is final for the user-facing option key — `multi-table_config` or `multi_table_config` — and is it the same in the option definition and the docs? The remaining Medium items (F3 identifier validation of the comma-joined primary keys, F4 interaction with the engine-level placeholder replacement, F6 explicit regex validation mapped to `JDBC-12`, F7 engine-level IT/E2E coverage) don't need to block this focused change; a follow-up issue tracking them would be appreciated so they don't get lost. Once the above is confirmed, I'm happy to approve. Nice work on this. <!-- streview-comment:1862 --> -- 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]
