SEZ9 commented on PR #12015:
URL: https://github.com/apache/seatunnel/pull/12015#issuecomment-6282033499
Thanks @luozihen for the quick follow-up. I see `82fc5e61a` (on top of
`231cbdea5`) adds `JdbcMultiTableConfigConditionExtension` so that a mis-shaped
`multi_table_config` fails during option validation instead of silently falling
back — that closes the fail-fast concern.
Before approving, I'd like to confirm the status of the other points from
the earlier round. Could you reply to each inline, or point me at the
commit/test that covers it?
1. **`ReadonlyConfig.toConfig()` dotted-key round-trip** — this lives in
`seatunnel-api`, so it affects every connector, not only the JDBC sink. What is
the behaviour for existing callers that relied on dotted keys being expanded
into paths? If the semantics changed, please spell it out and cover it with a
test.
2. **"First pattern in declaration order wins"** — the option is a
`Map<String, Object>`, and neither that map nor HOCON object keys guarantee
declaration order. Either use an order-preserving structure that is actually
populated in declaration order (and assert it in a test), or adjust the en/zh
docs to not promise this.
3. **Resolved key columns flowing into generated SQL** — the matched columns
are comma-joined into `PRIMARY_KEYS` and end up in `generate_sink_sql` output.
Please add identifier validation (or dialect quoting) so a bad regex value
can't produce broken or unsafe SQL.
4. **`${primary_key}` / `${unique_key}` vs. engine `TablePlaceholder`** —
please document (code comment and PR description) which replacement runs first
and what happens when both apply to the same value.
5. **Invalid regex** — does the new condition extension also compile each
pattern key and map a regex syntax error to the new JDBC-12 error code, or is
that still only hit lazily at match time? A unit test with an invalid pattern
would settle it.
6. **Tests** — I still don't see a regression test in `ReadableConfigTest`
for the previous dotted-key expansion, nor an engine-level IT/E2E exercising
`multi_table_config.primary_keys` end to end. Both are needed for a change that
touches `seatunnel-api`.
7. **Option key naming** — the current head reads `multi_table_config`
(snake_case), matching the JDBC sink convention. Please confirm the hyphenated
`multi-table_config` spelling is gone from both code and docs.
Once those are answered I'll do a final pass on the current head.
<!-- streview-comment:1887 -->
--
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]
