minni31 commented on code in PR #13229:
URL: https://github.com/apache/gluten/pull/13229#discussion_r4526065594
##########
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:
Thanks for flagging the dependency requirement. The current default Velox
dependency already includes native `right`: both `dft-2026_11_06` and
`ibm-2026_11_06` resolve to `IBM/velox` commit
`6deac80271569277bf54b6d04547620c17438348`, which [registers `right(varchar,
integer)`](https://github.com/IBM/velox/blob/6deac80271569277bf54b6d04547620c17438348/velox/functions/sparksql/registration/RegisterString.cpp#L161-L162).
The PR description now clarifies that the prerequisite is present in those
default pins rather than still pending. I retained the positive native-offload
assertions; no additional dependency update or availability gate is needed for
those defaults. A custom older Velox build may still lack the function.
--
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]
