SEZ9 commented on PR #12015:
URL: https://github.com/apache/seatunnel/pull/12015#issuecomment-5724181306
Thanks for the quick follow-up in `6043c1507`. Capturing the upstream
`CatalogTable` in `JdbcSinkFactory.createSink()` before the variable is
reassigned to the sink-side table, and passing it into
`resolveMultiTablePrimaryKeys`, is the minimal fix I was hoping for — the regex
now matches on the upstream table name regardless of
`table`/`tablePrefix`/`tableSuffix` remapping, which is what the docs already
describe. Good to see a regression test alongside it.
On the earlier review points, here is what I still need before merging:
- **PR12015-F1 (Compatibility, `ReadonlyConfig.toConfig()`)** — the JSON
round-trip changes how dotted keys are interpreted for every caller of
`toConfig()`, not only the JDBC sink. Please either scope the change so only
the new map option is affected, or explain why dropping the old dotted-key path
expansion is safe for all callers.
- **PR12015-F7 (Test)** — please add a test in `ReadableConfigTest` that
pins the previous dotted-key behavior of `toConfig()`, plus an engine-level
IT/E2E exercising `multi_table_config.primary_keys` end to end (ideally the
MySQL-CDC -> JDBC `generate_sink_sql = true` scenario this PR targets).
- **PR12015-F2 / PR12015-F5 (ordering)** — the docs promise "first pattern
in declaration order wins", but the option is a `Map<String, Object>` and HOCON
object keys don't guarantee declaration order. Either switch to an
order-preserving structure and add a test proving the order survives parsing,
or change the docs and fail fast when more than one pattern matches a table.
- **PR12015-F3 (Security)** — the resolved key columns are comma-joined into
`PRIMARY_KEYS` and end up in generated SQL. Please validate each column name as
an identifier (or quote it via the dialect) before it reaches SQL generation.
- **PR12015-F4 (Functional)** — please document (and test) the order between
the connector-level `${primary_key}`/`${unique_key}` expansion and the
engine-level TablePlaceholder replacement.
- **PR12015-F6 (Robustness)** — compile the user-supplied regex patterns
once at factory time and map invalid patterns to the new JDBC-12 error code,
including the offending pattern in the message.
- **PR12015-F8 (Style)** — I flagged the key `multi-table_config`; the
latest description refers to `multi_table_config`. Please confirm the key is
`multi_table_config` consistently in `JdbcSinkOptions`, the docs, and example
configs.
If any of these were already handled in `6043c1507`, just point me at the
spot and I'll take a look.
<!-- streview-comment:1127 -->
--
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]
