This is an automated email from the ASF dual-hosted git repository.
LuciferYang pushed a commit to branch branch-4.x
in repository https://gitbox.apache.org/repos/asf/spark.git
The following commit(s) were added to refs/heads/branch-4.x by this push:
new 09473c8e3335 [SPARK-57532][CORE][TEST] Add a test suite for
StringSubstitutor and tidy its default-value length bookkeeping
09473c8e3335 is described below
commit 09473c8e3335e26a1b83270ff56ecbaaa9f003b0
Author: YangJie <[email protected]>
AuthorDate: Wed Jun 24 13:17:55 2026 +0800
[SPARK-57532][CORE][TEST] Add a test suite for StringSubstitutor and tidy
its default-value length bookkeeping
### What changes were proposed in this pull request?
`StringSubstitutor` (added in SPARK-52990) is the hand-rolled `${name}` /
`${name:-default}` interpolation parser behind every error-message format, but
it has no direct test coverage. This PR adds `StringSubstitutorSuite` to
exercise it on its own.
The suite covers basic substitution, no-placeholder/empty/null input,
non-string values, undefined variables (both the throwing default and the
leave-as-is mode), default values (resolved, defaulted, empty, and empty
variable name), substitution that grows the buffer, escaped placeholders under
`preserveEscapes` true/false, nested expansion off/on, cycle detection, an
unclosed placeholder, custom `prefix`/`suffix`/`escape`/`valueDelimiter`, scan
progression past an unresolved placehol [...]
While adding the suite I also made two small production tweaks. First, the
default-value branch of `substitute` computed `newLength` (the scan length
adjusted for the replacement) but then recursed with the original, unadjusted
`length`, leaving `newLength` dead; the resolved-value branch right above it
already recurses with `newLength`, so this makes the two branches symmetric.
That change is behavior-preserving: a default value is always shorter than the
`${name:-default}` placehold [...]
### Why are the changes needed?
`StringSubstitutor` is on the path of all error-message formatting yet was
only tested indirectly. A dedicated suite documents and locks in its behavior,
including the less-obvious cases (escapes, defaults, nesting, cycles, custom
syntax). The `newLength` cleanup removes a confusing dead local, and
`String.valueOf` removes a latent `NullPointerException` on a null resolver
value.
### Does this PR introduce _any_ user-facing change?
No. The `newLength` cleanup is behavior-preserving. The `String.valueOf`
change only affects a `null` resolver value (previously a
`NullPointerException`, now the string `"null"`), which is not reachable from
the sole caller `ErrorClassesJSONReader`: it already maps null message
parameters to `"null"` before constructing the substitutor.
### How was this patch tested?
New unit tests. `build/sbt 'common-utils/testOnly *StringSubstitutorSuite'`
passes (18 cases).
### Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Claude Opus 4.8)
Closes #56648 from LuciferYang/SPARK-stringsubstitutor-tests.
Authored-by: YangJie <[email protected]>
Signed-off-by: yangjie01 <[email protected]>
(cherry picked from commit aa5108fe426437241c96404821d399d87edab2f4)
Signed-off-by: yangjie01 <[email protected]>
---
.../scala/org/apache/spark/StringSubstitutor.scala | 6 +-
.../org/apache/spark/StringSubstitutorSuite.scala | 160 +++++++++++++++++++++
2 files changed, 164 insertions(+), 2 deletions(-)
diff --git
a/common/utils/src/main/scala/org/apache/spark/StringSubstitutor.scala
b/common/utils/src/main/scala/org/apache/spark/StringSubstitutor.scala
index 6513c7dc7e8e..d8e59270092f 100644
--- a/common/utils/src/main/scala/org/apache/spark/StringSubstitutor.scala
+++ b/common/utils/src/main/scala/org/apache/spark/StringSubstitutor.scala
@@ -88,7 +88,9 @@ class StringSubstitutor(
resolver.get(varName) match {
case Some(value) =>
- var replacement = value.toString
+ // Use String.valueOf so a null value renders as "null"
(matching how the sole
+ // caller, ErrorClassesJSONReader, sanitizes null parameters)
instead of NPEing.
+ var replacement = String.valueOf(value)
if (enableSubstitutionInVariables) {
val newPrior = priorVariables.clone() += varName
val tempBuf = new StringBuilder(replacement)
@@ -109,7 +111,7 @@ class StringSubstitutor(
buf.replace(prefixPos, suffixPos + suffixLen, value)
val changeInLength = value.length - (suffixPos + suffixLen -
prefixPos)
val newLength = length + changeInLength
- substitute(buf, prefixPos + value.length, length,
priorVariables)
+ substitute(buf, prefixPos + value.length, newLength,
priorVariables)
return true
case None =>
if (enableUndefinedVariableException) {
diff --git
a/common/utils/src/test/scala/org/apache/spark/StringSubstitutorSuite.scala
b/common/utils/src/test/scala/org/apache/spark/StringSubstitutorSuite.scala
new file mode 100644
index 000000000000..549e356d1101
--- /dev/null
+++ b/common/utils/src/test/scala/org/apache/spark/StringSubstitutorSuite.scala
@@ -0,0 +1,160 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.spark
+
+import org.scalatest.funsuite.AnyFunSuite // scalastyle:ignore funsuite
+
+class StringSubstitutorSuite extends AnyFunSuite { // scalastyle:ignore
funsuite
+
+ test("basic substitution") {
+ val sub = new StringSubstitutor(Map("a" -> "X", "b" -> "Y"))
+ assert(sub.replace("${a}") === "X")
+ assert(sub.replace("pre${a}post") === "preXpost")
+ assert(sub.replace("${a}${b}") === "XY")
+ assert(sub.replace("${a}-${b}") === "X-Y")
+ }
+
+ test("no placeholders, empty and null input") {
+ val sub = new StringSubstitutor(Map("a" -> "X"))
+ assert(sub.replace("hello") === "hello")
+ assert(sub.replace("") === "")
+ assert(sub.replace(null) === null)
+ }
+
+ test("non-string values are rendered with toString") {
+ val sub = new StringSubstitutor(Map("n" -> 42, "b" -> true))
+ assert(sub.replace("${n}/${b}") === "42/true")
+ }
+
+ test("undefined variable throws by default") {
+ val sub = new StringSubstitutor(Map.empty[String, Any])
+ val e = intercept[IllegalArgumentException](sub.replace("${x}"))
+ assert(e.getMessage.contains("x"))
+ }
+
+ test("undefined variable is left as-is when exceptions are disabled") {
+ val sub =
+ new StringSubstitutor(Map.empty[String, Any],
enableUndefinedVariableException = false)
+ assert(sub.replace("a${x}b") === "a${x}b")
+ }
+
+ test("default value is used only when the variable is undefined") {
+ assert(new StringSubstitutor(Map.empty[String, Any]).replace("${x:-def}")
=== "def")
+ assert(new StringSubstitutor(Map("x" -> "X")).replace("${x:-def}") === "X")
+ // Empty variable name: the delimiter is at index 0, so the text after it
is the default.
+ assert(new StringSubstitutor(Map.empty[String,
Any]).replace("${:-fallback}") === "fallback")
+ }
+
+ test("a default-value substitution is followed by a further substitution") {
+ // After a default-value replacement the buffer shifts; this checks the
scan continues
+ // past it and still expands the later ${b}.
+ val sub = new StringSubstitutor(Map("b" -> "Y"))
+ assert(sub.replace("${a:-LONG_DEFAULT}${b}") === "LONG_DEFAULTY")
+ }
+
+ test("a resolved value longer than its placeholder is followed by a further
substitution") {
+ // The replacement is longer than its placeholder (a growing buffer); the
later ${b} must
+ // still be expanded.
+ val sub = new StringSubstitutor(Map("a" -> "LONG_VALUE", "b" -> "Y"))
+ assert(sub.replace("${a}${b}") === "LONG_VALUEY")
+ }
+
+ test("escaped placeholder is not substituted") {
+ // preserveEscapes = true (default): the escape is kept and the
placeholder is not expanded,
+ // but the scan resumes, so a later resolvable placeholder is still
expanded.
+ assert(new StringSubstitutor(Map("a" -> "X")).replace("$${a}") === "$${a}")
+ assert(new StringSubstitutor(Map("b" -> "Y")).replace("$${a}${b}") ===
"$${a}Y")
+ // preserveEscapes = false: the escape is removed and the placeholder is
still not expanded;
+ // again the scan resumes for a later placeholder.
+ assert(new StringSubstitutor(Map("a" -> "X"), preserveEscapes =
false).replace("$${a}")
+ === "${a}")
+ assert(new StringSubstitutor(Map("b" -> "Y"), preserveEscapes =
false).replace("$${a}${b}")
+ === "${a}Y")
+ }
+
+ test("nested references are expanded only when enabled") {
+ val resolver = Map[String, Any]("a" -> "${b}", "b" -> "Y")
+ assert(new StringSubstitutor(resolver).replace("${a}") === "${b}")
+ assert(new StringSubstitutor(resolver, enableSubstitutionInVariables =
true).replace("${a}")
+ === "Y")
+ }
+
+ test("cyclic references are detected when nested expansion is enabled") {
+ val sub = new StringSubstitutor(
+ Map("a" -> "${b}", "b" -> "${a}"), enableSubstitutionInVariables = true)
+ val e = intercept[IllegalStateException](sub.replace("${a}"))
+ assert(e.getMessage.startsWith("Infinite loop in property interpolation
of"))
+ assert(e.getMessage.contains("a") || e.getMessage.contains("b"))
+ }
+
+ test("an unclosed placeholder is left as-is") {
+ val sub = new StringSubstitutor(Map("a" -> "X"))
+ assert(sub.replace("text ${unclosed") === "text ${unclosed")
+ // A resolved placeholder earlier in the string does not stop the scan
from reaching and
+ // leaving the unclosed tail.
+ assert(sub.replace("${a} and ${unclosed") === "X and ${unclosed")
+ }
+
+ test("an empty default value collapses the placeholder to the empty string")
{
+ val sub = new StringSubstitutor(Map.empty[String, Any])
+ assert(sub.replace("prefix${x:-}suffix") === "prefixsuffix")
+ // A later placeholder is still expanded after the empty-default
replacement.
+ assert(sub.replace("${x:-}${y:-Z}") === "Z")
+ }
+
+ test("custom prefix, suffix, escape, and value delimiter") {
+ val sub = new StringSubstitutor(
+ Map("x" -> "X"), prefix = "%{", suffix = "}", escape = '%',
valueDelimiter = "|")
+ assert(sub.replace("%{x}") === "X")
+ assert(sub.replace("pre%{x}post") === "preXpost")
+ // The custom value delimiter supplies the default for an undefined
variable.
+ assert(sub.replace("%{y|def}") === "def")
+ // The custom escape char guards the custom prefix (preserveEscapes
defaults to true).
+ assert(sub.replace("%%{x}") === "%%{x}")
+ }
+
+ test("scan continues past an unresolved placeholder to a later resolved
one") {
+ // With exceptions disabled, an unresolved placeholder is left untouched
but does not stop the
+ // scan from reaching and expanding a later resolvable placeholder.
+ val sub = new StringSubstitutor(Map("x" -> "X"),
enableUndefinedVariableException = false)
+ assert(sub.replace("${undefined} ${x}") === "${undefined} X")
+ }
+
+ test("a null resolver value renders as the string \"null\"") {
+ // The sole caller (ErrorClassesJSONReader) sanitizes a null parameter to
"null", so a null
+ // value is rendered the same way here rather than NPEing on toString or
being treated as
+ // undefined.
+ val resolver = Map[String, Any]("x" -> null)
+ assert(new StringSubstitutor(resolver).replace("${x}") === "null")
+ assert(new StringSubstitutor(resolver).replace("a${x}b") === "anullb")
+ // The variable is resolved (to "null"), so its default value is not used.
+ assert(new StringSubstitutor(resolver).replace("${x:-def}") === "null")
+ }
+
+ test("placeholders inside a default value are not expanded") {
+ // The first '}' closes the placeholder, so the default value is the
literal text up to it and
+ // the inner ${x} is not treated as a nested reference.
+ val sub = new StringSubstitutor(Map("x" -> "X"))
+ assert(sub.replace("${undefined:-${x}}") === "${x}")
+ }
+
+ test("an empty placeholder with no default is undefined") {
+ val e = intercept[IllegalArgumentException](
+ new StringSubstitutor(Map.empty[String, Any]).replace("${}"))
+ assert(e.getMessage.contains("Undefined variable"))
+ }
+}
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
