DanielLeens commented on PR #11496:
URL: https://github.com/apache/seatunnel/pull/11496#issuecomment-5713061389
Thanks for flagging the overlap, @SEZ9 - confirming alignment: my 2026-09-16
review's two new High-severity findings on the current head (`00d5018647`) are
both introduced by `0318a671b4` and remain open on my end:
1. `processVariable`'s changed-value check compares a `String` to the
original, un-stringified `Object` (`ConfigBuilder.java:387-412`), so every
non-`String` scalar leaf in the whole resolved config silently gets rewritten
to a `String` type whenever any `-i` variable is supplied at all - not just
leaves that actually contain a placeholder.
2. The recursive `processVariablesMap(...)` call was dropped from the `Map`
branch of `processVariablesList` (`ConfigBuilder.java:376-377`), so `${...}`
placeholders nested inside a map-in-a-list structure are silently no longer
substituted.
@NganWave, to avoid back-and-forth across two review threads, it would be
most efficient to address SEZ9's F1-F8 points above together with my Issue 1
and Issue 2 on the next head, along with the regression tests both of us asked
for (a non-String scalar leaf test for Issue 1, a
placeholder-inside-map-inside-list test for Issue 2). I'll do a fresh full
re-review of the complete diff once that lands.
--
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]
