DanielLeens commented on PR #12810:
URL: https://github.com/apache/seatunnel/pull/12810#issuecomment-6411509120
Thanks @NganWave for your patience on this one, and thanks @SEZ9 for the
detailed review. Since the head is still `2786f03f12ab` (no new commit since my
last review), I re-read the relevant code at that exact head so the author gets
one consolidated list, and I want to confirm which of the new points hold up.
Static reading only; nothing was built or run locally.
**Overlap with my review (+1 @SEZ9, same root causes):**
- SEZ9 Issue 2 (raw defaults / input slices in error messages) is my Issue
5: `PlaceholderUtils.java` conflicting-default and unterminated errors still
render raw text, and `SeaTunnel.java` logs the message before desensitization.
- SEZ9 Issue 5 (quoted default keeps its quotes) is my Issue 1: the default
is emitted verbatim (`defaultValue.trim()` from the `substring` at the colon,
appended as `replacement`), while the `-i` path unwraps.
**New points from SEZ9 that I verified against the head (please treat these
as additions to the list):**
1. **Quoted `-i` value is re-typed after being unwrapped (SEZ9 Issue 1,
High).** `processPlaceholders` does `StringUtils.unwrap(userConfigMap.get(key),
"\"")`, and then `ConfigBuilder.processVariable` (the
`hasOnlyOnePlaceholder(...) && isStructured(replacedValue)` branch near line
435) calls `parseStructured` on the already-unwrapped text. So `-i
option='"[a-z]"'` with `option = "${option}"` becomes the list `["a-z"]`, which
is exactly the case the migration note says should stay a string. Typing that
leaf from the raw `-i` text (which still carries the quotes) and only using
`isStructured` for system-property/default values would fix it.
2. **`isStructured` now needs a depth-1 `:` or `=` (SEZ9 Issue 3).** In
`ConfigValueUtils.isStructured` the key/value flag is only set when
`stack.size() == 1`, and it returns `!isObject || hasKeyValue`. A valid HOCON
object such as `{fields{id=int}}` has its separators at depth 2, so it now
stays a String; on `dev` the check was bracket balance only. This changes
phase-one `-i` behavior and is not listed in the incompatible-changes entry.
3. **Default-consistency check also hits system placeholders (SEZ9 Issue
4).** The `defaultConfigMap` checks run before the
`isSystemPlaceholder.test(key)` guard. On `dev` there is no such check at all
(`git grep` for `defaultConfigMap` / `Inconsistent placeholder` on the base
finds nothing), so a multi-sink config that mixes `${table_name}` and
`${table_name:fallback}` would newly fail at load time, even though those keys
cannot be set with `-i`. Moving the system-placeholder test first and passing
the text through untouched would avoid that.
4. Smaller items from SEZ9 (Issue 7 narrower catch and wording, Issue 8 the
EN doc links pointing at `zh` and the unclosed `***`) look right as well; they
pair naturally with my Issue 3/4 doc and Javadoc cleanups.
None of this changes my verdict: the structured-default design is sound, and
the blockers are the ones already listed (my Issues 1, 5, 6 plus items 1-3
above). When you push the next commit I will re-review that head in full. No
sync with `dev` is needed just for the branch being behind. Thanks again for
the steady progress!
--
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]
