Copilot commented on code in PR #13229:
URL: https://github.com/apache/gluten/pull/13229#discussion_r4521305472
##########
backends-velox/src/main/scala/org/apache/gluten/backendsapi/velox/VeloxSparkPlanExecApi.scala:
##########
@@ -1350,6 +1351,75 @@ class VeloxSparkPlanExecApi extends SparkPlanExecApi
with Logging {
}
}
+ /**
+ * Spark replaces `right(str, len)` with
+ * `If(IsNull(str), null, If(len <= 0, "", Substring(str, -len,
Int.MaxValue)))` before physical
+ * planning. Convert that shape to `If(IsNull(str), null, right(str, len))`
so that Velox runs the
+ * Spark `right` function directly instead of `if` and `substring`.
+ */
+ private object SparkRightShape {
+ private def unwrapKnownNotNull(expression: Expression): Expression =
expression match {
+ case KnownNotNull(child) => unwrapKnownNotNull(child)
+ case other => other
+ }
+
+ def unapply(expression: Expression): Option[(Expression, Expression, If)]
= expression match {
+ case outer @ If(
+ IsNull(nullCheckedStr),
+ Literal(null, _),
+ If(
+ LessThanOrEqual(nonPositiveLen, Literal(0, IntegerType)),
+ Literal(empty: UTF8String, StringType),
+ Substring(substringStr, UnaryMinus(substringLen, _),
Literal(Int.MaxValue, _))
+ )
+ )
+ if empty == UTF8String.EMPTY_UTF8 &&
+ unwrapKnownNotNull(nullCheckedStr).semanticEquals(
+ unwrapKnownNotNull(substringStr)) &&
+ nonPositiveLen.semanticEquals(substringLen) =>
+ Some((nullCheckedStr, nonPositiveLen, outer))
+ case _ => None
+ }
+ }
+
+ override def extraExpressionConverter(
+ substraitExprName: String,
+ expr: Expression,
+ attributeSeq: Seq[Attribute]): Option[ExpressionTransformer] = expr
match {
+ case SparkRightShape(str, len, original) =>
+ // `substraitExprName` is the outer `if`. If any function in Spark's
replacement or in the
+ // converted expression is blacklisted, keep Spark's replacement, which
Gluten converts or
+ // falls back as usual.
+ val blacklist = GlutenConfig.get.expressionBlacklist
+ val usedFunctions =
+ Seq(
+ substraitExprName,
+ ExpressionNames.RIGHT,
+ ExpressionNames.IS_NULL,
+ ExpressionNames.SUBSTRING)
+ if (usedFunctions.exists(blacklist.contains)) {
+ None
+ } else {
+ val strTransformer =
ExpressionConverter.replaceWithExpressionTransformer(str, attributeSeq)
+ val lenTransformer =
ExpressionConverter.replaceWithExpressionTransformer(len, attributeSeq)
+ Some(
+ IfTransformer(
+ substraitExprName,
+ GenericExpressionTransformer(
+ ExpressionNames.IS_NULL,
+ Seq(strTransformer),
+ IsNull(str)),
+ LiteralTransformer(Literal.create(null, original.dataType)),
+ GenericExpressionTransformer(
+ ExpressionNames.RIGHT,
+ Seq(strTransformer, lenTransformer),
+ Right(str, len)),
Review Comment:
On a Velox build without the prerequisite native `right` function, this
unconditional rewrite makes previously native projections fail validation and
fall back to Spark. The new `checkNativeRight(expected = true)` assertions also
fail on such builds, a dependency gap acknowledged in the PR description.
Include the prerequisite dependency update/backport, or gate both the rewrite
and positive offload assertions on native function availability.
--
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]
