Copilot commented on code in PR #13282:
URL: https://github.com/apache/gluten/pull/13282#discussion_r4269878336
##########
cpp/velox/substrait/SubstraitToVeloxPlanValidator.cc:
##########
@@ -127,6 +127,15 @@ bool SubstraitToVeloxPlanValidator::validateRound(
return false;
}
+ const auto& scale = arguments[1].value().literal();
+ if (scale.has_null() && scale.null().has_i32()) {
+ const auto& valueType = params[0]->type();
+ if (!valueType->isDecimal() &&
+ (valueType->isTinyint() || valueType->isSmallint() ||
valueType->isInteger() || valueType->isBigint())) {
+ return true;
+ }
+ }
Review Comment:
`params[0]` is accessed without verifying `params` is non-empty (and that
`params[0]` is non-null). If argument conversion fails upstream but still
reaches `validateRound`, this can become an out-of-bounds access / crash. Add a
defensive check (e.g., `if (params.size() < 1 || !params[0]) return false;`)
before using `params[0]->type()`.
##########
backends-velox/src/main/scala/org/apache/gluten/backendsapi/velox/VeloxValidatorApi.scala:
##########
@@ -38,12 +39,29 @@ 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
+ override def doExprValidate(substraitExprName: String, expr: Expression):
Boolean = expr match {
+ case round: Round =>
+ round.child.dataType match {
+ case ByteType | ShortType | IntegerType | LongType =>
+ // Negative scales can overflow, and Spark captures ANSI mode in the
expression.
+ val supported = round.scale match {
+ case Literal(null, IntegerType) => true
+ case Literal(scale: Int, IntegerType) => scale >= 0 && scale <= 400
+ case _ => false
+ }
+ if (!supported) {
+ logDebug(
+ "Integral ROUND requires a folded INTEGER scale in [0, 400] or
NULL; retaining Spark.")
+ }
+ supported
+ case _ => true
+ }
+ case _ => true
+ }
Review Comment:
The scaladoc on `doExprValidate` now contradicts the implementation: key
validation is no longer solely on the native side since integral ROUND
eligibility is enforced here. Update the comment to reflect that JVM-side
validation is intentionally used to preserve Spark semantics (captured ANSI
mode) and to gate integral-scale offload.
##########
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="19480"
Review Comment:
Hard-coding `UPSTREAM_VELOX_PR_ID` to a specific PR will cause all builds
that source this script to fetch a non-default Velox revision, which can
unexpectedly break reproducibility for developers/CI once that PR changes or
disappears. Keep the default empty and set this via CI/environment (or a
dedicated developer-only override mechanism) when validating against a specific
Velox PR.
--
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]
