DanielLeens commented on PR #12015:
URL: https://github.com/apache/seatunnel/pull/12015#issuecomment-6359807234
Thanks @luozihen for the thorough follow-up, and thanks @SEZ9 and
@hutiefang76 for helping converge on the key contract. The head is still
`d9bb1574f0dfee9f6673533dff2616522a76e6c4`, the one my latest review covers, so
that review stands; this is a clarification, not a new full review. Static read
only, nothing was executed locally.
**1. The first row of the semantics table does not match the shaded parser
source on this head.** The table says `job.mode` / `shade.identifier` are kept
as literal keys by `ConfigFactory.parseMap`. I traced the code path in this
repo and it points the other way:
- `ConfigImpl.fromPathMap`
(`seatunnel-config-shade/.../impl/ConfigImpl.java:189-193`) calls
`PropertiesParser.fromPathMap`.
- `PropertiesParser.fromPathMap` (`PropertiesParser.java`, the map overload)
builds `Path.newPath(key)` for every entry, and `Path.newPath` is
`PathParser.parsePath` (`Path.java:233-235`), which splits on `.`.
- Nested map values are converted with `FromMapMode.KEYS_ARE_PATHS`
(`PropertiesParser.java:175-177`), so a key inside `env` such as
`shade.identifier` is also split into `shade` -> `identifier`. This is also why
the regex key `^t_nova_.*$` fails with `BadPath` in the first place.
- The existing `ReadableConfigTest#testToConfigPreservesDottedKeyExpansion`
(`ReadableConfigTest.java:383-390`) pins the same thing for `toConfig()`.
I could not find a patch to this path in `seatunnel-config-shade` (I
searched for #8975 and found nothing), so unless you can point me at the exact
class that changes this, I read the expansion as the current behavior. That is
why I flagged Issue 1: with the literal rebuild in `0c43f10dd`, an `env` body
carrying `shade.identifier` would no longer be found by
`hasPath("shade.identifier")` in `ConfigShadeUtils`.
This is a factual question we can settle by test rather than by argument,
and you already offered the right one. Please add the REST-shaped test exactly
as you described (map -> `RestUtil.buildConfig` ->
`ConfigShadeUtils.decryptConfig`) with `env` containing `shade.identifier`,
`job.mode`, `a.b` and a regex key, and assert which keys still expand and which
stay literal. If it is green on the literal rebuild, I will withdraw Issue 1
and say so; if it is red, it tells you which of the two options below to take.
**2. Which option to take.** Both are fine from my side. I agree with @SEZ9
and your own lean: keep existing keys behaving exactly as today and only rescue
keys that cannot be parsed as a path today (quote-only-invalid). Only one PR
should change `ConfigBuilder.of(Map)` / `ConfigShadeUtils.processConfig`;
please agree the owner with @hutiefang76 and drop the duplicate commit in the
other PR. If the owner is #12443, drop `0c43f10dd` here and restore the
limitation wording in the two Jdbc docs; the JDBC feature and validation stay
in this PR.
**3. CI, to correct one point in the thread.** The unit-test failure is not
the `Open udf failed` log line (that is logged output). All four `unit-test`
lanes fail on
`LLMBooleanOutputTest.readsLegacyFactoryWrapperWithValidationDisabled` in
`seatunnel-transforms-v2`, which this PR does not touch; the fix is #12938
(`cc54bb43`), and this head does not contain it (verified: it is not an
ancestor of the head). So re-triggering alone will not turn it green; syncing
the latest `dev` and re-running will. The dead link (the InterSystems IRIS
driver URL) is on `dev` already and not in your diff, so I agree with @SEZ9 not
to block on it.
Next step: sync `dev`, add the REST-shaped test, settle the owner of the
shared conversion, then push; I will do a fresh pass on the new head. My
earlier change request is still the visible state on this PR; Daniel has no
write-level review controls, so once the blockers are cleared a maintainer with
write access needs to do the final approve or dismiss.
--
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]
