luozihen commented on PR #12015:
URL: https://github.com/apache/seatunnel/pull/12015#issuecomment-5724793526
Thanks for the thorough pass — seven of these eight items were already
addressed in the earlier rounds, so instead of re-touching working code I've
pointed each one at the current source, and added the one thing that was
genuinely missing (a test for the placeholder pass ordering in F4). Everything
below is on the new head:C15F9FD.
F1 — ReadonlyConfig#toConfig() compatibility. toConfig() is unchanged: it is
still ConfigFactory.parseMap(confData)
(seatunnel-api/src/main/java/org/apache/seatunnel/api/configuration/ReadonlyConfig.java:76),
and ReadonlyConfig.java is not part of this PR's diff at all. The only
config-rebuild change here is in ConfigShadeUtils.processConfig, which replaces
ConfigFactory.parseMap(configMap) with a JSON round-trip; without it a regex
key such as ^t_nova_.*$ is re-read as a HOCON path and the job fails with
ConfigException$BadPath before the sink ever sees the option. The map being
re-encoded holds the values of an already-parsed config, so JSON re-encoding
keeps each key literal rather than applying path expansion a second time.
MultiTableFailureHelper#mergeOptions() no longer calls
toConfig()/withFallback() either — it merges the two option maps directly, with
the shallow-merge contract documented on the method. The previous dotted-key
expansion of toConfig() is pinned by Read
ableConfigTest#testToConfigPreservesDottedKeyExpansion.
F7 — tests. Both halves exist.
ReadableConfigTest#testToConfigPreservesDottedKeyExpansion asserts that an a.b
key still expands through the dotted path. The engine-level E2E is
JdbcMysqlMultipleTablesIT#testMysqlJdbcMultipleTableE2e with
jdbc_mysql_source_and_sink_with_multi_table_config.conf: it drops the sink
tables, lets the sink auto-create them, then asserts the resolved per-table
primary keys and row counts, and it runs on the Zeta/Flink/Spark containers. It
uses the JDBC source rather than MySQL-CDC deliberately — everything this
option touches is resolved sink-side in createSink(), so the source connector
type does not change the path under test. If you would like the CDC-sourced
variant as well I can add it, but I would suggest tracking it as a follow-up,
since another @TestTemplate in the CDC suite adds a full container x JDK matrix
run.
F2 / F5 — ordering. toCompiledPatternMap copies the configured patterns into
an explicit LinkedHashMap<Pattern, List<String>>, so ordering is guaranteed by
construction rather than by whatever map the parser hands over;
JdbcSinkFactoryTest#testResolveMultiTablePrimaryKeysFromHoconFirstMatchWins
parses a real HOCON string whose patterns overlap in non-alphabetical
declaration order (^table2$, ^table1$, then a catch-all ^table.*$ that would
win if the order were lost) and asserts the first declared match wins. The
en/zh docs state the same rule. I kept declaration-order semantics rather than
"fail fast when more than one pattern matches", because overlapping patterns
are the intended usage here (a broad default plus narrower overrides).
F3 — identifier safety. validatePrimaryKeyColumns rejects blank names and
names containing a comma before they are joined into PRIMARY_KEYS, raising
JDBC-12, covered by
JdbcSinkFactoryTest#testFactoryContextWithMultiTableConfigInvalidColumnFails.
Those two are the only inputs that cannot be represented safely: the resolved
names are interpolated through the dialect's quoteIdentifier when SQL is
generated (JdbcDialect#getDeleteStatement, MysqlDialect#quoteIdentifier,
MysqlCreateTableSqlBuilder#buildPrimaryKeySql), so quoting covers everything
else, while a comma collides with the delimiter of the comma-joined
primary_keys value and a blank name produces an empty list element. That
reasoning is now recorded on the method's Javadoc.
F4 — pass order. Documented and tested.
docs/{en,zh}/introduction/configuration/sink-options-placeholders.md state the
fixed order explicitly, and
JdbcSinkFactoryTest#testPlaceholderOrderBetweenEngineAndConnectorPasses pins
it: it runs the real engine pass first and asserts that a top-level
primary_keys = ["${primary_key}"] is expanded there, that the nested
multi_table_config.primary_keys map is still untouched afterwards, and that the
connector expands it during createSink().
F6 — regex handling. Patterns are compiled once in the factory
(compilePattern, memoized in the bounded COMPILED_PATTERN_CACHE, 256 entries)
and an invalid pattern raises JDBC-12 with the offending pattern in the
message; covered by testResolveMultiTablePrimaryKeysInvalidRegexFails,
testCompilePatternInvalidFails and testCompilePatternCachesCompiledPattern.
F8 — naming. Confirmed multi_table_config in JdbcSinkOptions, both docs, the
E2E config and all tests; the last hyphenated dummy key in ConfigShadeTest was
renamed as well.
One item from the previous round that is not on your list, but worth
knowing: 6043c1507 also fixed the name the patterns are matched against.
createSink() previously matched against the already-remapped sink table name,
so combining this option with table / tablePrefix / tableSuffix silently
defeated the mapping and fell back to catalog metadata; it now keeps the
upstream CatalogTable and matches on that, with
JdbcSinkFactoryTest#testFactoryContextWithMultiTableConfigMatchesUpstreamTableName
as the regression test.
--
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]
