SEZ9 commented on PR #12015:
URL: https://github.com/apache/seatunnel/pull/12015#issuecomment-6207000332
Thanks for keeping the branch in sync with `dev`. To be clear up front: my
earlier "ready to merge" on `e761f0c5b` does not carry over to the current head
(`7340000b87abd01130fdaaa4623f5e6a572e6d88`), so I re-reviewed it from scratch.
The PR-owned files are unchanged, and the feature logic itself still traces
cleanly (declaration order is preserved via the `LinkedHashMap` of compiled
patterns).
What changed is `dev`: the merge pulled in a change that reintroduces the
exact `BadPath` failure this PR exists to fix, on a code path the PR does not
cover. On this head, regex keys such as `^t_nova_.*$` no longer survive config
loading. This is a blocking regression. Could you extend the fix to that path
and add a test that loads a job config with a regex-keyed
`multi_table_config.primary_keys` map end-to-end (ideally an engine-level
IT/E2E with at least two matching tables and one unmatched fallback table)?
Separately, a few smaller carryover items I should have raised earlier,
apologies:
- Resolved key columns are comma-joined into `PRIMARY_KEYS` and reach
generated SQL; please validate each column name and raise
`INVALID_MULTI_TABLE_CONFIG` on failure.
- Please make an invalid regex fail fast at `createSink` time with the
JDBC-12 error code and the offending pattern in the message, with a unit test.
- Please add a short note (code comment and docs) on which layer expands
`${primary_key}` / `${unique_key}` when the engine-level placeholder
replacement is also in play, plus a unit test mixing a placeholder with a
static column such as `DATA_SOURCE`.
Once the config-loading regression is covered I'm happy to do a final pass.
<!-- streview-comment:1787 -->
--
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]
