SEZ9 commented on PR #12015:
URL: https://github.com/apache/seatunnel/pull/12015#issuecomment-5825482928
@luozihen thanks for mapping F1-F8 to concrete locations against head
00acc23a5 — that makes the re-review much easier.
I still need to walk the actual diff before marking anything resolved, so a
few things that would help me close the items out:
- F1: please confirm in the PR description that `ReadonlyConfig.toConfig()`
is left unchanged in this PR and briefly describe what the changes at
`ConfigShadeUtils.java:224` and `MultiTableFailureHelper.java:64` do instead,
so the compatibility impact on other callers is clear.
- F2/F5: a `LinkedHashMap` in `toCompiledPatternMap` preserves the order it
receives, but "first pattern in declaration order wins" also depends on the
parsed `Map<String, Object>` arriving in HOCON declaration order. Could you
either add a unit test with two overlapping patterns declared in a known order
that asserts the first one wins end to end from a parsed config, or soften the
docs to say behaviour is undefined for overlapping patterns? Either is fine
with me.
- F4: please add a sentence to the option docs stating whether
`${primary_key}`/`${unique_key}` are expanded by the connector before or after
the engine-level table placeholder replacement, so users know which one applies
if both appear.
- F7: could you summarize what `ReadableConfigTest` and
`JdbcMysqlMultipleTablesIT` assert for the dotted-key round-trip and the new
option, respectively?
Once I have those and have gone through the diff for F3, F6 and F8, I'll
post the updated status.
<!-- streview-comment:1289 -->
--
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]
