DanielLeens commented on PR #12015:
URL: https://github.com/apache/seatunnel/pull/12015#issuecomment-5846333267
Thanks for narrowing it down to these four, @SEZ9 — I went and re-checked
each one directly against the source at `00acc23a582` rather than relying on
the description prose.
**F1** — Confirmed unchanged. `ReadonlyConfig#toConfig()`
(`ReadonlyConfig.java:76`) is still exactly `ConfigFactory.parseMap(confData)`.
The two actual changes are:
- `ConfigShadeUtils.processConfig` (`ConfigShadeUtils.java:224-226`) now
rebuilds the post-decrypt map with
`ConfigFactory.parseString(JsonUtils.toJsonString(configMap),
ConfigParseOptions.defaults().setSyntax(ConfigSyntax.JSON))` instead of
`ConfigFactory.parseMap(configMap)`. Since `configMap` is already the result of
a JSON parse, this keeps every key literal on the round trip instead of
re-interpreting it as a HOCON path — that is what lets a regex key such as
`^t_nova_.*$` survive the encrypt/decrypt step. It runs for every job, but it
only changes behavior for keys that previously failed to parse as a HOCON path
(or that relied on being re-expanded as one at this specific point); I don't
see a caller in this repo that does the latter.
- `MultiTableFailureHelper.mergeOptions()`
(`MultiTableFailureHelper.java:64-71`) no longer goes through
`Config#withFallback()`. It now does `merged.putAll(fallback.getSourceMap());
merged.putAll(primary.getSourceMap())` and wraps the result with
`ReadonlyConfig.fromMap()`. The Javadoc directly above it now states: "The
merge is shallow/top-level: on a key collision, primary's value replaces
fallback's value wholesale, and nested objects are not merged recursively."
That's the exact caveat we pinned down a few rounds back, now written into the
code rather than left implicit.
So the compatibility impact on other `toConfig()` callers is nil — that
method never changed. The only shared-code shift is `mergeOptions()`'s merge
strategy, and every current caller (`MultipleTableJobConfigParser`, the
Spark/Flink `SinkExecuteProcessor`s,
`withMultiTableFailurePolicy`/`withFailedTables`) merges disjoint namespaces,
so the shallow-vs-deep distinction is a no-op for them today.
**F2/F5** — This is exactly covered end to end.
`JdbcSinkFactoryTest#testResolveMultiTablePrimaryKeysFromHoconFirstMatchWins`
(`JdbcSinkFactoryTest.java:348-380`) parses a real HOCON string containing:
```
"^table2$" = ["c_mediumint"]
"^table1$" = ["c_int", "c_integer"]
"^table.*$" = ["c_smallint"]
```
declared in that non-alphabetical order — the catch-all `^table.*$` is
declared last even though it would sort first — builds a `ReadonlyConfig` via
`ReadonlyConfig.fromConfig(...)` from the parsed `Config`, and calls the
production `factory.resolveMultiTablePrimaryKeys(...)` directly, not a
hand-built map. It asserts `table1` resolves to `["c_int", "c_integer"]` and
`table2` to `["c_mediumint"]` — the first-declared pattern wins for both, not
the catch-all. That is the full chain you asked for (HOCON string →
`ReadonlyConfig` → option → `toCompiledPatternMap`), so I'd consider F2/F5
resolved.
**F4** — Also written down now.
`docs/en/introduction/configuration/sink-options-placeholders.md:145-148` (and
the `docs/zh` counterpart) states: "The two passes run in a fixed order. The
engine-level replacement described above runs first and only [rewrites
top-level String/single-element-String-List values, so the]
`multi_table_config.primary_keys` map reaches the sink untouched. The JDBC sink
then expands `${primary_key}` / `${unique_key}` inside that map once per
matched table, while the sink is created." That matches what both of us
independently traced through
`TablePlaceholderProcessor`/`expandPrimaryKeyPlaceholder` earlier in this
thread.
**F7** — `ReadableConfigTest#testToConfigPreservesDottedKeyExpansion`
(`ReadableConfigTest.java:383-391`) builds a map with a single dotted key
`a.b`, round-trips it through `ReadonlyConfig.fromMap(...).toConfig()`, and
asserts `config.getInt("a.b") == 1` — the old HOCON path-expansion behavior for
dotted keys is intact for every caller of `toConfig()`.
`JdbcMysqlMultipleTablesIT#testMysqlJdbcMultipleTableE2e` runs the actual
`multi_table_config.primary_keys` scenario end to end through the engine: it
drops the dedicated sink tables so the sink has to create them itself, executes
`jdbc_mysql_source_and_sink_with_multi_table_config.conf`, and then asserts the
primary keys read back from the database catalog (not from the job's own
config) — `mtc_table1` → `["c_int", "c_integer"]`, `mtc_table2` →
`["c_mediumint"]` — with a comment noting the catch-all pattern is declared
last on purpose. So this also re-proves first-match-wins at the engine level,
not only in the factory unit tests.
With F1, F2/F5, F4 and F7 now independently verified against the diff, and
F3/F6/F8 already confirmed in my previous pass, I don't have anything further
open on my side. Ready to merge from my read; still waiting on a write-capable
maintainer for the actual merge action.
--
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]
