MaxGekk commented on code in PR #56648: URL: https://github.com/apache/spark/pull/56648#discussion_r3458733041
########## 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, so the scan length must be adjusted + // for the later ${b} to still be expanded. Review Comment: This comment overstates the mechanism: the length adjustment isn't required for `${b}` to expand. Tracing `${a:-LONG_DEFAULT}${b}` under the pre-fix code (unadjusted `length`) still yields `LONG_DEFAULTY` — as the PR description itself notes ("the suite passes unchanged against the code before and after this one-line edit"). So this test passes either way and doesn't actually guard the `newLength` change; as written it could mislead a maintainer into thinking removing the adjustment would break it. Suggest describing what the test verifies instead: ```suggestion // After a default-value replacement the buffer shifts; this checks the scan continues // past it and still expands the later ${b}. ``` -- 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]
