DanielLeens commented on PR #12015:
URL: https://github.com/apache/seatunnel/pull/12015#issuecomment-6406899455
Hi @luozihen, @hutiefang76, @SEZ9, thanks for pushing this to a concrete
answer, and @luozihen, I owe you a correction.
I re-checked the shaded parser on `dev`:
`ConfigParseOptions.PATH_TOKEN_SEPARATOR` is `"->"`, and
`PathParser.splitTokenOnPeriod` / `addPathText` split only on that token. So in
this repository a plain dotted key such as `shade.identifier` or `job.mode` is
not expanded by `parseMap`; it stays one literal key, and
`env.hasPath("shade.identifier")` is true both before and after the literal
rebuild. That means the premise of my earlier "Issue 1 (High)" (that
`0c43f10dd` stops `env`'s dotted shade keys from being found) was wrong, and my
+1 on the expansion concern was wrong for the same reason. Sorry for the
detour. I also misread
`ReadableConfigTest#testToConfigPreservesDottedKeyExpansion`: it only asserts
`getInt("a.b")`, which passes for a literal key too, so it does not pin
expansion.
What does still differ, per your table, is narrower: `a->b` (expands today,
would become one literal key) and keys containing quote characters (normalized
today, kept as-is). Those can matter for existing REST JSON users, so I agree
with the plan you described: quote only the keys that fail path parsing and
leave everything that parses today untouched, with the REST-shaped case
(`RestUtil.buildConfig` -> `ConfigShadeUtils.decryptConfig`) asserting
`shade.identifier`, `job.mode`, `a.b`, `a->b` and a regex key. With that in
place I will re-review the new head and withdraw the compatibility blocker.
Please also fix the comment in `ConfigBuilderTest` that describes the "before"
shape.
Two practical notes: please settle ownership of `ConfigBuilder.of(Map)` /
`ConfigShadeUtils` with @hutiefang76, since #12443 already carries the
quote-only-invalid helper and its `Build` is green on its current head (I am
approving it separately); reusing that helper, or rebasing on it, avoids two
competing conversions. And the red `Build` here is the `LLMBooleanOutputTest`
failure in `seatunnel-transforms-v2`, fixed on `dev` by #12938 and absent from
this branch, which is 33 commits behind, so syncing `dev` with your next push
will clear 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]
