DanielLeens commented on PR #12810:
URL: https://github.com/apache/seatunnel/pull/12810#issuecomment-6305090510
Thanks for the quick reply, @NganWave, and for being open about pausing.
Splitting the work to keep each PR focused is a sensible call, and since this
is a draft there is no pressure on timing.
On Issue 2 I would like to double-check the root cause with you, because I
re-read the code against `dev` and I think the re-typing comes from this PR
rather than from phase 1, so it will not go away by tightening the phase-1
parser alone:
- On `dev`, `ConfigBuilder.processVariable` and `processVariablesList`
(lines 373-420 on `dev`) put the replaced text back as a plain `String`
(`parentMap.put(variableKey, variableString)`). A value such as `field =
"[${name}]"` with `-i name=x` therefore stays the text `[x]` today.
- In this PR the same two methods gained a new call,
`ConfigValueUtils.parseValue(replacedValue)` whenever the text changed
(`ConfigBuilder.java:392` and `:427` at head `d3053a87`). That call is what
turns `[x]` into the list `["x"]`. On `dev`, `parseValue` is only used for `-i`
values in `substituteUserConfig` (line 273), which is a different surface.
- The cases in Issue 2 do not depend on lenient bracket handling. `[x]` is
balanced and valid, so it is parsed as a list even by a strict parser; and
`parseValue` unwraps a value that starts and ends with `"` in its first branch,
before any bracket logic runs. So making `ParameterSplitter`/`parseValue`
stricter for unbalanced input would fix a different problem, but would not
change these results.
So I think Issue 2 is still open for phase 2. The smallest fix I can see
stays the same as in my last review: call `parseValue` only when the original
value is exactly one placeholder (the `"""${k:{...}}"""` form that the docs and
the E2E use), and keep the plain `String` result for mixed text such as
`"[${name}]"`, `"${p:[0-9]}"` or a quote-wrapped SQL fragment. If you do find a
real phase-1 defect while working on it, fixing it in its own PR first is a
good plan, and I would be glad to take a look.
When you resume, the other open items from my last review still apply: the
contradictory `ConfigShadeTest.testPlaceholders` (it fails all four unit-test
lanes, so it is the first thing to fix), the test strictness points, and the
two documentation items. Thanks again for the steady work on this!
--
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]
