SEZ9 commented on PR #12015:
URL: https://github.com/apache/seatunnel/pull/12015#issuecomment-5770190113
@luozihen Yes — I'll treat the "Review status (F1-F8)" section of the PR
description as the source of truth for the next pass. Note that this comment
was cut off mid-F4 on my side too, so everything from F4 onward I can only take
from the description.
On what did come through:
- **F1** — If `ReadonlyConfig.java` is not in the diff and `toConfig()` is
unchanged, the compatibility concern as originally written does not apply, so
I'm fine dropping it as a `ReadonlyConfig` issue. One remaining ask: please
confirm (in the F1 entry) whether the parsing change in
`ConfigShadeUtils.processConfig` / `MultiTableFailureHelper#mergeOptions` is
scoped to the new multi-table option only, or whether it alters how dotted keys
are parsed for any other option that flows through those two methods.
- **F2 / F5** — `LinkedHashMap<Pattern, List<String>>` in
`toCompiledPatternMap` guarantees the compiled map preserves whatever order it
receives; the real question is the order of the input map, which is exactly
what `testResolveMultiTablePrimaryKeysFromHoconFirstMatchWins` needs to prove.
Please make sure that test goes through the same path the runtime uses (HOCON
string → `ReadonlyConfig` → option → `toCompiledPatternMap`) rather than a
hand-built map. If it does, F2 and F5 are resolved for me.
- **F3** — Rejecting blank names and commas with `JDBC-12` plus dialect
quoting covers the cases I raised. Only follow-up: does the dialect quoting
also escape a quote character appearing inside a column name, or should
`validatePrimaryKeyColumns` reject those as well? Either answer is fine; I just
want it to be deliberate.
For **F4–F8** I'll read the description, but to save a round trip please
make sure those entries name: the test for the `${primary_key}` /
`${unique_key}` vs. engine-level placeholder ordering (F4), where invalid regex
patterns are compiled and mapped to `JDBC-12` (F6), the regression test and the
IT/E2E for the new option (F7), and the final decision on the
`multi-table_config` key name (F8).
<!-- streview-comment:1218 -->
--
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]
