DanielLeens commented on PR #12015:
URL: https://github.com/apache/seatunnel/pull/12015#issuecomment-6143673372
@SEZ9 thanks for the item-by-item pass on `e761f0c5b`. I re-checked each
point against the pinned head (author code is byte-identical to `00acc23a5`),
and most of them are already covered in the diff. Details below so you can
confirm against the files.
- **F1 (`ReadonlyConfig#toConfig()`):** agreed, it is not in the diff. The
regex-key handling is in `ConfigShadeUtils.processConfig` and
`MultiTableFailureHelper.mergeOptions`.
- **F2 / F5 (first declared pattern wins):** `toCompiledPatternMap` builds a
`LinkedHashMap` (`JdbcSinkFactory.java:344`), and the contract is stated in the
Javadoc and in `docs/en/connectors/sink/Jdbc.md` ("If a table matches multiple
patterns, the first pattern in declaration order wins"). The end-to-end
ordering is pinned by
`JdbcSinkFactoryTest#testResolveMultiTablePrimaryKeysFromHoconFirstMatchWins`
(`JdbcSinkFactoryTest.java:348`): it parses a real HOCON string with three
overlapping patterns (`^table2$`, `^table1$`, `^table.*$`) and asserts that
`table1` resolves to the `^table1$` mapping, not the later wildcard.
`testResolveMultiTablePrimaryKeysFirstMatchWins` (line 329) covers the same
rule with an explicit `LinkedHashMap`. So the "show where order is enforced
plus a two-overlapping-patterns test" ask from option (a) is already met.
- **F3 (key columns flowing into SQL):** this one is only partly covered,
and I want to be precise. `validatePrimaryKeyColumns`
(`JdbcSinkFactory.java:282`) rejects blank names and names containing a comma,
with `JDBC-12`. It does not check the column against the upstream schema, and I
would not call that a blocker: the generated statements quote identifiers
through the dialect (see the Javadoc at lines 270-280), and a column that does
not exist surfaces as a clear DDL/DML error at the sink instead of silently
writing wrong data. A schema-existence check would be a reasonable hardening,
but it would also be a behaviour choice for the author and maintainers, so I
would treat it as optional.
- **F4 (placeholder ordering, no-PK case):** `docs/en` and `docs/zh`
`sink-options-placeholders.md` now describe the order (engine-level pass first,
connector-level expansion per matched table at sink creation). The no-key case
is an error, not an empty expansion: `expandPrimaryKeyPlaceholder`
(`JdbcSinkFactory.java:416-443`) throws `JDBC-12` naming the table and pattern,
documented in `Jdbc.md` and tested by
`testResolveMultiTablePrimaryKeysMissingPrimaryKeyFails` (line 317). The pass
order is tested by `testPlaceholderOrderBetweenEngineAndConnectorPasses` (line
503).
- **F6 (invalid regex):** `compilePattern` (`JdbcSinkFactory.java:361`)
wraps `PatternSyntaxException` into `JDBC-12`, and it runs from
`resolveMultiTablePrimaryKeys` during `createSink`, so the user sees the
connector error code, not a raw regex exception. Tests:
`testResolveMultiTablePrimaryKeysInvalidRegexFails` (line 399) and
`testCompilePatternInvalidFails` (line 419). One small gap: these assert the
exception type rather than the `JDBC-12` code. That is a Low polish item, not a
blocker.
- **F7 (IT):** `JdbcMysqlMultipleTablesIT` runs a real MySQL-to-JDBC
multi-table job with `jdbc_mysql_source_and_sink_with_multi_table_config.conf`
and asserts the primary key columns that end up in the created sink tables
(`mtc_table1` gets the composite key, `mtc_table2` a single key). The config
also declares a catch-all `^mtc_.*$` last, so the job fails if declaration
order were not preserved: this pins F2 end to end on a real job. To be precise
about the remaining gap, both tables in the IT match a pattern, so the "one
matched and one unmatched table" combination is covered only at unit level
(`testResolveMultiTablePrimaryKeysUnmatched`, `JdbcSinkFactoryTest.java:307`,
and the legacy-fallback factory tests), not in the IT. Adding a third source
table that matches no pattern and asserting it keeps the fallback `id` key
would close that, and I would treat it as a recommended, non-blocking addition.
- **F8 (naming):** the key is `multi_table_config`
(`JdbcSinkOptions.java:113`) with the nested `primary_keys`, spelled the same
way in code, docs and tests. A search of the head for the old
`multi-table_config` form finds nothing.
**CI on `e761f0c5b` (fork run `38199352694`, completed):** 81 jobs passed,
11 skipped, 2 failed: `all-connectors-it-2 (11)` and `(17)`. Both fail on the
same two tests, `CassandraIT#testCassandra` (line 134) and
`#testCassandraMultiTableSource` (line 144, `expected: <0> but was: <1>`,
container index 4). This PR touches no Cassandra files (0 of its 15 files), and
every JDBC IT job, including the one with `JdbcMysqlMultipleTablesIT`, is
green. I could not tell from the logs whether the Cassandra failure also
reproduces on `dev`, so I am not claiming it is a known flake; the diff simply
cannot reach it. The branch is 8 commits behind `dev` (`diverged`, ahead 26,
behind 8).
Suggested next step for @luozihen: re-run only the failed jobs from the fork
Actions page ("Re-run failed jobs"). Syncing `dev` is optional, and please do
not change Cassandra tests in this PR.
My source-side conclusion is unchanged: no blocker; the optional items are
the F3 schema check, an unmatched table in the IT (F7), and asserting the
`JDBC-12` code in the invalid-regex tests. Thanks again for the careful
follow-through!
--
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]
