sunchao commented on code in PR #56575:
URL: https://github.com/apache/spark/pull/56575#discussion_r3807660938
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/stringExpressions.scala:
##########
@@ -2690,29 +2690,40 @@ case class Substring(str: Expression, pos: Expression,
len: Expression)
since = "2.3.0",
group = "string_funcs")
// scalastyle:on line.size.limit
-case class Right(str: Expression, len: Expression) extends RuntimeReplaceable
- with ImplicitCastInputTypes with BinaryLike[Expression] {
-
- override lazy val replacement: Expression = If(
- IsNull(str),
- Literal(null, str.dataType),
- If(
- LessThanOrEqual(len, Literal(0)),
- Literal(UTF8String.EMPTY_UTF8, str.dataType),
- new Substring(str, UnaryMinus(len, failOnError = false))
- )
- )
+object Right extends DelegateFunction {
+ override val name: String = "right"
override def inputTypes: Seq[AbstractDataType] =
- Seq(
- StringTypeWithCollation(supportsTrimCollation = true),
- IntegerType
- )
- override def left: Expression = str
- override def right: Expression = len
- override protected def withNewChildrenInternal(
- newLeft: Expression, newRight: Expression): Expression = {
- copy(str = newLeft, len = newRight)
+ Seq(StringTypeWithCollation(supportsTrimCollation = true), IntegerType)
+
+ // At build time `str` is the not-yet-coerced argument (wrapped in an
`ImplicitCastInput` marker
+ // that delegates `dataType` to its child), so `str.dataType` is the *input*
type, which is not
+ // necessarily a string yet -- e.g. `right(12345, 2)` has an `IntegerType`
child the implicit cast
+ // will turn into a string. Use it for the null/empty branch literals only
when it is already a
+ // string-family type, so a CHAR(N)/VARCHAR(N) result (under
+ // `spark.sql.preserveCharVarcharTypeInfo`) or a non-default collation is
preserved through the
+ // `If` branch unification; otherwise fall back to plain `StringType`, the
type the implicit cast
+ // produces. Typing a UTF8String literal with a non-string type would be
invalid.
+ override def lower(args: Seq[Expression]): Expression = {
+ val str = args(0)
+ val len = args(1)
+ val litType = str.dataType match {
+ case _: StringType | _: CharType | _: VarcharType => str.dataType
+ case _ => StringType
+ }
+ // Keep both arguments single-use while the analyzer extracts window
expressions. The length
+ // is bound inside the non-null branch so right's null short-circuit is
preserved.
+ With(str) { case Seq(strRef) =>
Review Comment:
[P2] Preserve analysis-time evaluation of constant right calls
`VALUES (right('abc', 1))` previously produced one row containing `c`, but
now fails with
`INVALID_INLINE_TABLE.CANNOT_EVALUATE_EXPRESSION_IN_INLINE_TABLE`. The new
definition starts with `With`, which is unevaluable and non-foldable.
Inline-table analysis checks `prepareForEval(e).foldable`, and that helper only
unwraps `RuntimeReplaceable`; the new delegate is therefore rejected before the
optimizer can rewrite its definition. This is independent of the nested-`With`
optimizer crash. I reproduced the analysis failure and verified that the
previous foldable `If`/`Substring` definition still passes the same
inline-table validation. Please preserve this constant-expression behavior and
add a `VALUES` regression.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/jsonExpressions.scala:
##########
@@ -178,72 +178,79 @@ object GetJsonObject {
}
/**
- * Extracts multiple simple object-key and array-index paths from a JSON
string in one parse. This
- * is an internal expression used to share sibling [[GetJsonObject]]
expressions; unsupported and
- * prefix-conflicting JSON paths remain as independent GetJsonObject
expressions.
+ * Builds the internal expression that extracts multiple simple object-key and
array-index paths
+ * from a JSON string in one parse, used to share sibling [[GetJsonObject]]
expressions; unsupported
+ * and prefix-conflicting paths remain as independent `GetJsonObject`
expressions.
+ *
+ * It is inserted by `OptimizeCsvJsonExprs` (after analysis, so its inputs are
resolved), and is the
+ * optimizer-constructed showcase for [[DelegateExpression]]: instead of
hand-written
+ * eval/doGenCode, it builds a typed delegate directly -- the high-level call
+ * `multi_get_json_object(json, p1, ..., pn)` stays visible via `inputs`,
while the `definition`
+ * delegates evaluation to [[MultiGetJsonObjectEvaluator]] through an
`Invoke`. The delegate stays
+ * in logical plans and is lowered to its definition before physical planning.
*/
-case class MultiGetJsonObject(
- json: Expression,
- fallbackPaths: Seq[String])
- extends UnaryExpression
- with ExpectsInputTypes {
-
- // OptimizeCsvJsonExprs caps shared path depth to keep evaluator recursion
stack-safe.
- require(fallbackPaths.nonEmpty)
-
- override def child: Expression = json
-
- override def inputTypes: Seq[AbstractDataType] =
- Seq(StringTypeWithCollation(supportsTrimCollation = true))
-
- override lazy val dataType: DataType = StructType(fallbackPaths.indices.map
{ index =>
- StructField(s"_$index", StringType, nullable = true)
- })
-
- override def nullable: Boolean = true
-
- // This internal unary expression always returns null when its JSON child is
null.
- override def nullIntolerant: Boolean = true
-
- override def prettyName: String = "multi_get_json_object"
-
- final override val nodePatterns: Seq[TreePattern] = Seq(GET_JSON_OBJECT)
-
- @transient
- private lazy val simplePaths = fallbackPaths.map { path =>
- GetJsonObject.simplePath(UTF8String.fromString(path)).getOrElse {
- throw new IllegalArgumentException(s"Unsupported shared JSON path:
$path")
+object MultiGetJsonObject {
+ val name: String = "multi_get_json_object"
+
+ def apply(json: Expression, fallbackPaths: Seq[String]): DelegateExpression
= {
+ // OptimizeCsvJsonExprs caps shared path depth to keep evaluator recursion
stack-safe.
+ require(fallbackPaths.nonEmpty)
+ val resultType = StructType(fallbackPaths.indices.map { index =>
+ StructField(s"_$index", StringType, nullable = true)
+ })
+ val utf8Paths = fallbackPaths.map(UTF8String.fromString)
+ val simplePaths = utf8Paths.map { path =>
+ GetJsonObject.simplePath(path).getOrElse {
+ throw new IllegalArgumentException(s"Unsupported shared JSON path:
$path")
+ }
}
+ val evaluator = MultiGetJsonObjectEvaluatorHolder(utf8Paths, simplePaths)
+ // `propagateNull = true` reproduces the old null-intolerant behavior:
null json -> null result.
+ val definition = Invoke(
+ evaluator,
+ "evaluate",
+ resultType,
+ Seq(json),
+ Seq(json.dataType),
+ returnNullable = true)
+ // `inputs` keeps the high-level call visible: the json plus one string
literal per path.
+ val pathInputs = utf8Paths.map(Literal(_, StringType))
+ DelegateExpression(name, json +: pathInputs, definition)
}
- override def stateful: Boolean = true
+ /** Recovers `(json, fallbackPaths)` from a delegate produced by `apply`. */
+ def unapply(e: Expression): Option[(Expression, Seq[String])] = e match {
+ case d: DelegateExpression if d.name == name =>
+ val paths = d.inputs.tail.map {
+ case Literal(p: UTF8String, _: StringType) => p.toString
+ case other => throw new IllegalStateException(s"Unexpected path input:
$other")
+ }
+ Some((d.inputs.head, paths))
+ case _ => None
+ }
- @transient
- private lazy val evaluator = MultiGetJsonObjectEvaluator(
- fallbackPaths.map(UTF8String.fromString),
- simplePaths)
+ def isInstance(e: Expression): Boolean = unapply(e).isDefined
- override def eval(input: InternalRow): Any = {
- evaluator.evaluate(json.eval(input).asInstanceOf[UTF8String])
+ def pathsOf(e: Expression): Seq[String] = unapply(e) match {
+ case Some((_, paths)) => paths
+ case None => throw new IllegalArgumentException(s"Not a
multi_get_json_object: $e")
}
+}
- override protected def doGenCode(ctx: CodegenContext, ev: ExprCode):
ExprCode = {
- val refEvaluator = ctx.addReferenceObj("evaluator", evaluator)
- val jsonEval = json.genCode(ctx)
- val resultType = CodeGenerator.javaType(dataType)
- ev.copy(code = code"""
- |${jsonEval.code}
- |boolean ${ev.isNull} = ${jsonEval.isNull};
- |$resultType ${ev.value} = ${CodeGenerator.defaultValue(dataType)};
- |if (!${ev.isNull}) {
- | ${ev.value} = ($resultType)
$refEvaluator.evaluate(${jsonEval.value});
- | ${ev.isNull} = ${ev.value} == null;
- |}
- |""".stripMargin)
- }
+/** Holds one mutable JSON evaluator per fresh expression copy. */
+private case class MultiGetJsonObjectEvaluatorHolder(
+ paths: Seq[UTF8String],
+ simplePaths: Seq[Seq[GetJsonObject.SimpleJsonPathSegment]])
+ extends LeafExpression with CodegenFallback {
- override protected def withNewChildInternal(newChild: Expression):
MultiGetJsonObject =
- copy(json = newChild)
+ @transient private lazy val evaluator = MultiGetJsonObjectEvaluator(paths,
simplePaths)
+
+ override def dataType: DataType =
ObjectType(classOf[MultiGetJsonObjectEvaluator])
+ override def nullable: Boolean = false
+ override def stateful: Boolean = true
+ override def eval(input: InternalRow): Any = evaluator
+ override protected def withNewChildrenInternal(
Review Comment:
[P1] Keep the leaf copy override public
`LeafLike.withNewChildrenInternal` is public, so this `protected` override
reduces its visibility and prevents Catalyst from compiling. The exact-head
[SBT precompile
job](https://github.com/cloud-fan/spark/actions/runs/32165746499/job/95805272577)
reports that the override has weaker access privileges and should be public.
Please remove `protected` while retaining `copy()`; the fresh-holder behavior
is still needed.
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/DelegateExpressionSuite.scala:
##########
@@ -0,0 +1,243 @@
+/*
+ * 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.sql.catalyst.expressions
+
+import org.apache.spark.SparkFunSuite
+import org.apache.spark.sql.AnalysisException
+import org.apache.spark.sql.catalyst.InternalRow
+import org.apache.spark.sql.catalyst.analysis.{RemoveInputTypeMarkers,
UnresolvedAttribute}
+import org.apache.spark.sql.catalyst.analysis.TypeCheckResult.DataTypeMismatch
+import org.apache.spark.sql.catalyst.expressions.objects.Invoke
+import org.apache.spark.sql.types.{AbstractDataType, AnyDataType, IntegerType,
StringType, StructField, StructType}
+import org.apache.spark.unsafe.types.UTF8String
+
+/**
+ * Validates [[DelegateExpression]] transparency (eval + codegen, via
`checkEvaluation` which runs
+ * both paths) and that [[DelegateFunction]] supports all three input-type
contracts.
+ */
+class DelegateExpressionSuite extends SparkFunSuite with ExpressionEvalHelper {
+
+ // ---- transparency: every behavior delegates to `definition` ----
+
+ test("delegates eval and codegen to its definition (foldable)") {
+ val expr = DelegateExpression("inc", Seq(Literal(10)), Add(Literal(10),
Literal(1)))
+ checkEvaluation(expr, 11)
+ }
+
+ test("delegates eval and codegen with a non-foldable input") {
+ val ref = BoundReference(0, IntegerType, nullable = true)
+ val expr = DelegateExpression("inc", Seq(ref), Add(ref, Literal(1)))
+ checkEvaluation(expr, 11, InternalRow(10))
+ checkEvaluation(expr, null, InternalRow(null))
+ }
+
+ test("delegates type/nullability/foldability/determinism and canonicalizes
to its definition") {
+ val ref = BoundReference(0, IntegerType, nullable = true)
+ val expr = DelegateExpression("inc", Seq(ref), Add(ref, Literal(1)))
+ assert(expr.dataType == IntegerType)
+ assert(expr.nullable)
+ assert(!expr.foldable)
+ assert(expr.deterministic)
+ assert(expr.canonicalized == Add(ref, Literal(1)).canonicalized)
+ assert(DelegateExpression("inc", Seq(Literal(10)), Add(Literal(10),
Literal(1))).foldable)
+ }
+
+ // ---- input-type contracts ----
+
+ private object CastFn extends DelegateFunction {
+ override val name = "castfn"
+ override def inputTypes: Seq[AbstractDataType] = Seq(StringType)
+ override def implicitCast: Boolean = true
+ override def lower(args: Seq[Expression]): Expression = args.head
+ }
+
+ private object CheckFn extends DelegateFunction {
+ override val name = "checkfn"
+ override def inputTypes: Seq[AbstractDataType] = Seq(IntegerType)
+ override def implicitCast: Boolean = false
+ override def lower(args: Seq[Expression]): Expression = args.head
+ }
+
+ private object AnyFn extends DelegateFunction {
+ override val name = "anyfn"
+ // no inputTypes -> accepts any type
+ override def lower(args: Seq[Expression]): Expression = args.head
+ }
+
+ // The input-type contracts are the analysis-time `build` path, which
inserts the markers.
+ private def buildDef(fn: DelegateFunction, args: Expression*): Expression =
+ fn.build(fn.name, args).asInstanceOf[DelegateExpression].definition
+
+ test("contract 1 (implicit cast): args wrapped in ImplicitCastInput (cast
happens via " +
+ "the standard coercion rule)") {
+ val shim = buildDef(CastFn, Literal(1)).asInstanceOf[ImplicitCastInput]
+ assert(shim.expectedType == StringType)
+ // The shim IS an ImplicitCastInputTypes node, so TypeCoercion will cast
its child.
+ assert(shim.isInstanceOf[ImplicitCastInputTypes])
+ }
+
+ test("contract 2 (type check only): args wrapped in TypeCheckInput, mismatch
rejected, " +
+ "no cast") {
+ val ok = buildDef(CheckFn, Literal(1)).asInstanceOf[TypeCheckInput]
+ assert(ok.checkInputDataTypes().isSuccess)
+ // A Long is NOT cast down to Int -- it is rejected.
+ val bad = buildDef(CheckFn, Literal(1L)).asInstanceOf[TypeCheckInput]
+ assert(bad.checkInputDataTypes().isFailure)
+ assert(!bad.isInstanceOf[ImplicitCastInputTypes])
+ }
+
+ test("contract 3 (any type): no inputTypes -> no shim, arg passed through
unchanged") {
+ assert(buildDef(AnyFn, Literal(1L)) == Literal(1L))
+ }
+
+ test("nullIntolerant is delegated to the definition") {
+ // An Invoke with the default propagateNull = true is null-intolerant; a
bare Literal is not.
+ val invoke = Invoke(Literal("x"), "toString", StringType)
+ assert(invoke.nullIntolerant)
+ assert(DelegateExpression("f", Seq(Literal("x")), invoke).nullIntolerant,
+ "the wrapper should report its null-intolerant definition's
null-intolerance")
+ assert(!DelegateExpression("g", Seq(Literal(1)),
Literal(1)).nullIntolerant)
+ }
+
+ test("build validates argument count against the inputTypes arity") {
+ // CheckFn declares one typed input, so build rejects any other arity with
WRONG_NUM_ARGS rather
+ // than indexing past the args (too few) or silently ignoring extras (too
many).
+ Seq(Seq.empty[Expression], Seq(Literal(1), Literal(2))).foreach { args =>
+ val e = intercept[AnalysisException](CheckFn.build(CheckFn.name, args))
+ assert(e.getCondition == "WRONG_NUM_ARGS.WITHOUT_SUGGESTION")
+ }
+ // AnyFn has no inputTypes -> it is variadic and `lower` owns the arg
handling, so no arity
+ // check.
+ assert(AnyFn.build(AnyFn.name, Seq(Literal(1),
Literal(2))).isInstanceOf[DelegateExpression])
+ }
+
+ test("a surviving marker reports transparently: delegate-call sql and the
real argument index") {
+ // A marker only survives when its type check failed, and `CheckAnalysis`
(bottom-up) reports it
+ // before the enclosing delegate. It must therefore look like the
high-level call, not an
+ // internal shim: its `sql` is the delegate call, and its type-check error
carries the true
+ // argument position rather than the marker's only-child index 0 ("first").
+ val marker = ImplicitCastInput(
+ Literal(1L), StringType, funcName = "castfn", argIndex = 1, callSql =
"castfn(x, 1)")
+ assert(marker.sql == "castfn(x, 1)")
+ marker.checkInputDataTypes() match {
+ case m: DataTypeMismatch =>
+ assert(m.messageParameters("paramIndex") == "second",
+ s"expected the real argument index, got
${m.messageParameters("paramIndex")}")
+ case other => fail(s"expected a DataTypeMismatch, got $other")
+ }
+ // With no supplied context (direct construction), `sql` falls back to the
plain node rendering.
+ assert(
+ ImplicitCastInput(Literal(1L), StringType).sql ==
s"implicitcastinput(${Literal(1L).sql})")
+ }
+
+ test("RemoveInputTypeMarkers keeps a failed type-check marker for
CheckAnalysis to report") {
+ // A resolved marker has served its purpose and is unwrapped to its child
...
+ val okDelegate = CheckFn.build(CheckFn.name, Seq(Literal(1)))
+
assert(!RemoveInputTypeMarkers.removeMarkers(okDelegate).exists(_.isInstanceOf[TypeCheckInput]),
+ "a resolved TypeCheckInput should be unwrapped")
+ // ... but a type-mismatched (unresolved) marker is left in place, so its
ExpectsInputTypes
+ // failure stays visible to CheckAnalysis instead of exposing a resolved
child of a wrong type.
+ val badDelegate = CheckFn.build(CheckFn.name, Seq(Literal(1L)))
+ val cleaned = RemoveInputTypeMarkers.removeMarkers(badDelegate)
+ assert(cleaned.exists(_.isInstanceOf[TypeCheckInput]),
+ s"a failed TypeCheckInput must be preserved for CheckAnalysis, got
$cleaned")
+ }
+
+ private object MixedFn extends DelegateFunction {
+ override val name = "mixedfn"
+ override def inputTypes: Seq[AbstractDataType] = Seq(StringType,
AnyDataType)
+ override def lower(args: Seq[Expression]): Expression = CreateArray(args)
+ }
+
+ test("input-type contract is per argument: AnyDataType position opts out of
shimming") {
+ val args = buildDef(MixedFn, Literal(1),
Literal(2)).asInstanceOf[CreateArray].children
+ assert(args(0).isInstanceOf[ImplicitCastInput]) // StringType -> shimmed
+ assert(args(1) == Literal(2)) // AnyDataType -> raw
+ }
+
+ test("apply (direct construction) inserts no markers; args must already be
typed") {
+ // Unlike `build`, `apply` is construct-anywhere and never produces
input-type markers.
+ assert(CastFn(Literal("x")).definition == Literal("x"))
+ assert(
+ MixedFn(Literal("s"), Literal(2)).definition ==
CreateArray(Seq(Literal("s"), Literal(2))))
+ }
+
+ test("apply rejects unresolved arguments") {
+ intercept[IllegalArgumentException](CastFn(UnresolvedAttribute("x")))
+ }
+
+ // ---- definition is a real child (the safety property the whole design
rests on) ----
+
+ test("transform reaches into the definition and withNewChildren replaces
it") {
+ val ref = BoundReference(0, IntegerType, nullable = true)
+ val expr = DelegateExpression("inc", Seq(ref), Add(ref, Literal(1)))
+ // tree traversal descends into `definition`
+ val bumped = expr.transform { case Literal(1, IntegerType) => Literal(2) }
+ assert(bumped == DelegateExpression("inc", Seq(ref), Add(ref, Literal(2))))
+ // withNewChildren swaps the single child, which is the definition
+ val replaced =
expr.withNewChildren(Seq(Literal(99))).asInstanceOf[DelegateExpression]
+ assert(replaced.definition == Literal(99))
+ assert(replaced.inputs == Seq(ref)) // inputs are metadata, untouched
+ }
+
+ test("references come from the definition, not from inputs") {
+ val a = AttributeReference("a", IntegerType)()
+ val b = AttributeReference("b", IntegerType)()
+ // `b` appears only as display metadata; the real child references `a`
+ val expr = DelegateExpression("f", Seq(b), Add(a, Literal(1)))
+ assert(expr.references == AttributeSet(a))
+ }
+
+ test("sql and prettyName reflect the high-level call") {
+ val expr = DelegateExpression("myfunc", Seq(Literal(1), Literal("x")),
Literal(0))
+ assert(expr.prettyName == "myfunc")
+ assert(expr.sql == "myfunc(1, 'x')")
+ }
+
+ test("DelegateFunction.unapply round-trips apply") {
+ assert(CastFn.unapply(CastFn(Literal("x"))).contains(Seq(Literal("x"))))
+ assert(AnyFn.unapply(Literal(1)).isEmpty)
+ }
+
+ // ---- input-type markers are transient, unevaluable, and transparent in
type ----
+
+ test("input-type markers are Unevaluable and delegate type/nullability to
their child") {
+ val marker = ImplicitCastInput(BoundReference(0, IntegerType, nullable =
true), StringType)
+ assert(marker.dataType == IntegerType) // delegates to child until
coercion casts it
Review Comment:
[P2] Update the marker test for the new fallback type
The latest `InputTypeMarker.dataType` returns
`expectedType.defaultConcreteType` when the child's type check fails. This
marker has an integer child but expects `StringType`, so its data type is now
`StringType`, not `IntegerType`. After removing only the production compile
blocker, the existing `DelegateExpressionSuite` fails here with `StringType did
not equal IntegerType`. Please update the assertion and test description to
match the new contract; a separate matching-type case can retain coverage that
valid markers expose their child's type.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/stringExpressions.scala:
##########
@@ -2690,29 +2690,40 @@ case class Substring(str: Expression, pos: Expression,
len: Expression)
since = "2.3.0",
group = "string_funcs")
// scalastyle:on line.size.limit
-case class Right(str: Expression, len: Expression) extends RuntimeReplaceable
- with ImplicitCastInputTypes with BinaryLike[Expression] {
-
- override lazy val replacement: Expression = If(
- IsNull(str),
- Literal(null, str.dataType),
- If(
- LessThanOrEqual(len, Literal(0)),
- Literal(UTF8String.EMPTY_UTF8, str.dataType),
- new Substring(str, UnaryMinus(len, failOnError = false))
- )
- )
+object Right extends DelegateFunction {
+ override val name: String = "right"
override def inputTypes: Seq[AbstractDataType] =
- Seq(
- StringTypeWithCollation(supportsTrimCollation = true),
- IntegerType
- )
- override def left: Expression = str
- override def right: Expression = len
- override protected def withNewChildrenInternal(
- newLeft: Expression, newRight: Expression): Expression = {
- copy(str = newLeft, len = newRight)
+ Seq(StringTypeWithCollation(supportsTrimCollation = true), IntegerType)
+
+ // At build time `str` is the not-yet-coerced argument (wrapped in an
`ImplicitCastInput` marker
+ // that delegates `dataType` to its child), so `str.dataType` is the *input*
type, which is not
+ // necessarily a string yet -- e.g. `right(12345, 2)` has an `IntegerType`
child the implicit cast
+ // will turn into a string. Use it for the null/empty branch literals only
when it is already a
+ // string-family type, so a CHAR(N)/VARCHAR(N) result (under
+ // `spark.sql.preserveCharVarcharTypeInfo`) or a non-default collation is
preserved through the
+ // `If` branch unification; otherwise fall back to plain `StringType`, the
type the implicit cast
+ // produces. Typing a UTF8String literal with a non-string type would be
invalid.
+ override def lower(args: Seq[Expression]): Expression = {
+ val str = args(0)
+ val len = args(1)
+ val litType = str.dataType match {
+ case _: StringType | _: CharType | _: VarcharType => str.dataType
+ case _ => StringType
+ }
+ // Keep both arguments single-use while the analyzer extracts window
expressions. The length
+ // is bound inside the non-null branch so right's null short-circuit is
preserved.
+ With(str) { case Seq(strRef) =>
+ If(
+ IsNull(strRef),
+ Literal(null, litType),
+ With(len) { case Seq(lenRef) =>
+ If(
+ LessThanOrEqual(lenRef, Literal(0)),
+ Literal(UTF8String.EMPTY_UTF8, litType),
+ new Substring(strRef, UnaryMinus(lenRef, failOnError = false)))
Review Comment:
[P1] Preserve outer references when rewriting the nested With
The inner `With(len)` contains `strRef` from the outer `With`. For a `With`
in a conditional branch, `RewriteWithExpression` builds a map containing only
that inner node's definitions, then replaces every `CommonExpressionRef` using
`refToExpr(ref.id)`. It therefore looks up the outer string reference in a map
containing only the length reference. After fixing the compilation issue,
optimizing `SELECT right('abc', 1) AS r` reproducibly throws
`NoSuchElementException: key not found: CommonExpressionId(0,false)`. Could we
make the substitution scope-aware, or otherwise avoid this cross-scope shape,
while preserving null short-circuiting and single-use window inputs?
--
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]
