MaxGekk commented on PR #56648:
URL: https://github.com/apache/spark/pull/56648#issuecomment-4768482045
Nice addition - the new `StringSubstitutorSuite` significantly improves
direct coverage for a parser that sits on the error-message path.
I have a few test-gap suggestions that would make the suite more complete:
1. **Custom syntax knobs are not covered yet**
`StringSubstitutor` exposes configurable `prefix`, `suffix`, `escape`,
and `valueDelimiter`, but all tests use defaults. Could we add at least one
test with non-default values (e.g. `%{x}` and a custom delimiter)? This
protects constructor-level behavior.
2. **Mixed unresolved/resolved placeholders in one string**
With `enableUndefinedVariableException = false`, it would be good to pin
that scanning continues after an unresolved placeholder:
- input: `" "`
- expected: `" X"`
This specifically guards scan progression.
3. **Resolver value = `null` behavior**
Current implementation does `value.toString`; if resolver has `("x" ->
null)`, that likely NPEs. A test here would document intended behavior (throw
vs treat as undefined), rather than leaving it accidental.
4. **Placeholders inside default values**
Consider pinning `` semantics. It’s a subtle parser behavior whether ``
is expanded in that flow; a dedicated test would avoid ambiguity/regressions.
Optional nice-to-have:
- `` (empty var name without default),
- extra escape edge cases (`24578x`, etc.),
- stronger assertion on cycle-detection message prefix.
Overall direction looks good; these are mainly to lock in less-obvious
corners.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
