DanielLeens commented on PR #11559:
URL: https://github.com/apache/seatunnel/pull/11559#issuecomment-5649789744
Pushed `779004c073`, which fixes the root cause, and `4d75844b4a`, which
rebases-by-merge onto current `dev` (140+ commits, the blocker from round 14).
**The fix** — you had it exactly right:
`ConfigParseOptions.PATH_TOKEN_SEPARATOR = "->"` (not `.`) is the real segment
separator for this project's shaded `Config#getString`/`hasPath`/`getValue` and
the other path-expression accessors. Every dotted literal
`MultipleTableJobConfigParser` passed to those methods —
`schema-change.behavior`, `state.backend`, `state.ttl`, `join.type`,
`join.fields`, `state.max-concurrent-snapshots`,
`resource.max-concurrent-snapshots`, and both
`resource.max-*-state-bytes-per-subtask` keys — was therefore resolved as one
literal key containing a dot, not a nested path. Since
`validateDynamicLookupModeConfig`'s `schema-change.behavior` check runs first
and unconditionally, this one broken check masked every other validation path,
which is exactly the "expected true, was false" pattern across the suite.
Rather than hardcoding `"->"` into the validator's business logic, I added
`navigateDynamicLookupParent`/`hasDynamicLookupPath`/`dynamicLookupLeafKey`
plus thin `getDynamicLookupString`/`StringList`/`Int` wrappers that split the
caller's dotted literal themselves and walk `Config#getConfig` one segment at a
time — the same pattern this file already uses correctly for `factConfig =
config.getConfig("fact")`. This sidesteps the separator quirk entirely instead
of depending on an internal detail of the shaded fork, and every path this
validator checks is a fixed key its own schema declares (none contain a literal
dot), so splitting on `.` is unambiguous here.
**The test you asked for** — every existing dynamic-lookup test already
parses through the real shaded HOCON parser (`ConfigFactory.parseString`, not a
hand-built `Config`), so once the fix lands,
`testDynamicLookupLeftJoinMakesDimensionProjectionNullable` (the one
positive-path case) becomes a direct end-to-end regression guard for this exact
bug class — it can't pass unless every nested dotted check in the chain
resolves correctly. I also added
`testDynamicLookupRejectsUnsupportedStateBackendValue`, which fills a gap none
of the five existing tests covered: a value that's present at its correctly
nested path but semantically invalid. A value-only assertion couldn't
previously distinguish "path not found" from "path found, wrong value" — both
produced *a* `JobDefineCheckException` — so this test asserts the
value-specific message and would fail loudly with the old "requires
'state.backend'" absent-path message if this regresses.
**Rebase** — `4d75844b4a` merges current `dev` (via `git merge`, no
force-push). Merge-tree confirmed clean before I did it; the actual merge
auto-resolved every file, no conflicts. Fresh fork Build is running on this
head now (run `34729103406`).
Point 3 (KafkaSourceReader deserialization) is still awaiting your
confirmation of which line/commit you're seeing as unguarded — happy to
re-check the moment you point me at it.
--
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]
