DanielLeens commented on PR #12015:
URL: https://github.com/apache/seatunnel/pull/12015#issuecomment-5852837915
@SEZ9 thanks for the quick follow-up. Two things to close out.
**On the "cut off" comment** — I re-fetched my previous comment directly
through the GitHub API (not the rendered web page) and it is complete on the
server side; nothing was truncated in storage. My best guess is the web UI's
long-comment fold ("Show more") only rendered the visible portion for you. For
the record, here is the tail that answers (a) and (b) directly:
> ...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.
I went back to the source again to answer your two specific questions
precisely:
**(a) Does the exact-match pattern win over the catch-all declared last?**
Yes.
`JdbcSinkFactoryTest#testResolveMultiTablePrimaryKeysFromHoconFirstMatchWins`
(`JdbcSinkFactoryTest.java:347-384`) declares the HOCON map as `^table2$`, then
`^table1$`, then the catch-all `^table.*$` last. If iteration order were not
preserved (e.g. if it fell back to alphabetical or hash order), the catch-all
would win for both tables since it sorts first and matches both. The test
asserts `table1 -> [c_int, c_integer]` and `table2 -> [c_mediumint]`, i.e. the
specific pattern wins in both cases, not `[c_smallint]` from the catch-all. So
non-alphabetical declaration order is honored end to end from a parsed `Config`
through `ReadonlyConfig` into `resolveMultiTablePrimaryKeys`.
**(b) Does the order guarantee hold through the shade round trip, not just
in the factory unit test?** Yes, and it's actually verified at three
independent layers, not only the one you quoted:
1. `JdbcSinkFactoryTest.java:347-384` — proves order survives `HOCON string
-> ConfigFactory.parseString -> ReadonlyConfig.fromConfig ->
resolveMultiTablePrimaryKeys`, as above.
2. `ConfigShadeTest#testDecryptPreservesSpecialCharacterKeys`
(`ConfigShadeTest.java:439-468`) — exercises
`ConfigShadeUtils.decryptConfig(...)` directly (this is the method that wraps
`processConfig`, the JSON-round-trip shade step) on a config containing two
`multi_table_config.primary_keys` regex keys, and asserts both regex keys and
their list values survive the round trip unchanged.
3. `JdbcMysqlMultipleTablesIT#testMysqlJdbcMultipleTableE2e` runs the real
engine job-submission pipeline end to end, using the same non-alphabetical,
catch-all-last pattern set (`^mtc_table2$`, `^mtc_table1$`, `^mtc_.*$` — see
the test resource config), and asserts the primary keys read back from the
database catalog match the specific patterns (`mtc_table1 -> [c_int,
c_integer]`, `mtc_table2 -> [c_mediumint]`), not the catch-all. Since
`ConfigShadeUtils.processConfig` runs unconditionally at job submission
(`ConfigShadeUtils.java:143` -> `:152`), this E2E test necessarily exercises
the shade round trip too, so it closes the gap between "unit-tested in
isolation" and "actually holds in the full submission path."
So F2/F5 is covered end to end across the factory unit test, the shade unit
test, and the E2E test — I'd consider it closed with solid evidence, not just a
single narrow assertion.
**On the F1 regression-test ask** — good catch, but it's already there and I
should have pointed to it explicitly instead of only citing the positive case.
`ConfigShadeTest#testDecryptPreservesOrdinaryConfigShapes`
(`ConfigShadeTest.java:470-496`) is exactly the negative-case regression test
you're asking for: it builds a config with an unquoted `split.size` key (which
HOCON expands into a nested object) and a quoted literal `"dfs.replication"`
key inside a nested map, runs it through `ConfigShadeUtils.decryptConfig(...)`,
and asserts the `env`/`source`/`sink` blocks come out byte-for-byte equal (via
`root().unwrapped()`) to the input. Paired with the positive regex-key case in
`testDecryptPreservesSpecialCharacterKeys` (`ConfigShadeTest.java:439-468`),
both the "must not change" and "must now work" halves of the JSON round trip
are covered. I don't think a new test is needed here; the existing pair already
asserts exactly what you described.
With that, I don't have any open items on F1/F2/F5 either. Still Ready to
merge from my side, pending CI and a write-capable maintainer for the 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]
