SEZ9 commented on PR #12015:
URL: https://github.com/apache/seatunnel/pull/12015#issuecomment-6228523437
@luozihen Thanks for the ping, and apologies for the delay.
I see CI run `38840923843` on head `231cbdea5` is reported green, so CI is
no longer a gate from my side.
Before I give a clear yes on merging, I'd like to confirm the status of the
two HIGH items from the earlier review on this head:
1. The `ReadonlyConfig.toConfig()` dotted-key round-trip change, which
affects every caller rather than only the JDBC sink (and whether a regression
test for the old behaviour exists).
2. The documented "first pattern in declaration order wins" guarantee, given
the option is a map whose key order is not preserved.
Could you point me to where these were addressed in the current head, or say
explicitly if you intend to leave them as-is? If they are resolved, I'm fine
with your plan to merge first and treat whatever remains of the Medium items
(compile-time regex validation mapped to the `JDBC-12` error code, identifier
validation of the resolved key columns before they reach
`PRIMARY_KEYS`/generated SQL, the `${primary_key}`/`${unique_key}` vs
engine-level placeholder interaction, and the `multi-table_config` naming) as
follow-ups, ideally tracked in a follow-up issue.
<!-- streview-comment:1809 -->
--
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]
