Copilot commented on code in PR #13103:
URL: https://github.com/apache/gluten/pull/13103#discussion_r4282072854
##########
cpp/velox/substrait/SubstraitToVeloxPlanValidator.cc:
##########
@@ -127,6 +127,19 @@ bool SubstraitToVeloxPlanValidator::validateRound(
return false;
}
+ const auto& valueType = params[0]->type();
+ if (valueType->isDecimal()) {
+ const auto& scale = arguments[1].value().literal();
+ if (scale.has_null() && scale.null().has_i32()) {
+ return true;
+ }
+ if (scale.has_i32() && scale.i32() >= -400 && scale.i32() <= 400) {
+ return true;
+ }
+ LOG_VALIDATION_MSG("Decimal ROUND requires an INTEGER scale in [-400, 400]
or NULL.");
+ return false;
+ }
Review Comment:
`params[0]` is dereferenced without verifying `params.size() >= 1` (and more
generally, consistency with `arguments.size()`). While `arguments.size() < 2`
is checked, a mismatch between `arguments` and `params` would cause an
out-of-bounds access/crash here. Add an explicit `params.size()` guard (e.g.,
require `params.size() >= 2`) before indexing into `params`.
##########
ep/build-velox/src/get-velox.sh:
##########
@@ -25,7 +25,7 @@ RUN_SETUP_SCRIPT=ON
ENABLE_ENHANCED_FEATURES=OFF
# Developer use only for testing Velox PR.
-UPSTREAM_VELOX_PR_ID=""
+UPSTREAM_VELOX_PR_ID="19141"
Review Comment:
Hard-coding a non-empty `UPSTREAM_VELOX_PR_ID` changes the default build
behavior for everyone (including CI and local dev), making builds depend on a
specific upstream PR. This should remain empty by default (as the comment
indicates) and be set only via an explicit developer/CI override (e.g.,
environment variable, command-line argument, or a separate CI-only patch).
##########
backends-velox/src/main/scala/org/apache/gluten/backendsapi/velox/VeloxValidatorApi.scala:
##########
@@ -38,12 +39,26 @@ import io.substrait.proto.SimpleExtensionDeclaration
import scala.collection.JavaConverters._
import scala.collection.mutable.ArrayBuffer
-class VeloxValidatorApi extends ValidatorApi {
+class VeloxValidatorApi extends ValidatorApi with Logging {
import VeloxValidatorApi._
- /** For velox backend, key validation is on native side. */
- override def doExprValidate(substraitExprName: String, expr: Expression):
Boolean =
- true
+ /**
+ * Enforce decimal ROUND scale compatibility on the JVM independently of
optional native plan
+ * validation.
+ */
+ override def doExprValidate(substraitExprName: String, expr: Expression):
Boolean = expr match {
+ case round: Round if round.child.dataType.isInstanceOf[DecimalType] =>
+ val supported = round.scale match {
+ case Literal(null, IntegerType) => true
+ case Literal(scale: Int, IntegerType) => scale >= -400 && scale <= 400
+ case _ => false
+ }
+ if (!supported) {
+ logDebug("Decimal ROUND requires a folded INTEGER scale in [-400, 400]
or NULL.")
+ }
+ supported
+ case _ => true
+ }
Review Comment:
This validator only accepts `round.scale` when it is syntactically a
`Literal`, but the intended rule (and the log message) is about
*folded/foldable* scales. A foldable expression like `CAST(NULL AS INT)` or a
constant cast/arith expression can still be compatible, yet this code will
reject it unless Spark has already rewritten it into a `Literal`. To make
offload decisions robust across Spark versions/rules, treat
`round.scale.foldable` as supported by evaluating it and checking for typed
NULL or an `Int` in `[-400, 400]`, instead of requiring a `Literal` shape.
--
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]
