SEZ9 commented on PR #12015: URL: https://github.com/apache/seatunnel/pull/12015#issuecomment-5738485145
@luozihen Thanks for the detailed walkthrough on c15f9fd. **F1** — Understood. If `ReadonlyConfig#toConfig()` is unchanged and `ReadonlyConfig.java` is not in this PR's diff, then the finding as written was aimed at the wrong place, and I'm happy to retarget it to `ConfigShadeUtils.processConfig`. The reasoning for the JSON round-trip there makes sense — a regex key like `^t_nova_.*$` being re-read as a HOCON path and failing with `ConfigException$BadPath` would block this feature. Two things I'd still like so we can close F1 and F7 together: 1. Confirm that existing configs using dotted keys still resolve to the same values after `processConfig` as before. Since the map holds already-parsed values I expect they do, but a small unit test asserting both cases (literal regex key preserved, dotted key unchanged) would make the guarantee explicit. 2. Your comment cuts off at "it merges the two option maps" — could you finish that thought on `MultiTableFailureHelper#mergeOptions()`? Specifically, how does the merge handle a key present in both maps, and does it keep the same precedence `withFallback()` gave? **F4** — Good to see a test added for the placeholder pass ordering; that was the piece I most wanted covered. **F2, F3, F5, F6, F8** — You mention these were pointed at the current source, but that part of the comment didn't come through. Could you re-post the pointers? In particular, for F2/F5 I'd like to see where declaration order for the map-typed option is preserved (or where the docs were adjusted to stop promising it), and for F6 where the user regex is compiled up front and mapped to the JDBC-12 error code. Once those land I'll do a final pass against c15f9fd. <!-- streview-comment:1148 --> -- 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]
