Copilot commented on code in PR #13103:
URL: https://github.com/apache/gluten/pull/13103#discussion_r4283938510
##########
cpp/velox/substrait/SubstraitToVeloxPlanValidator.cc:
##########
@@ -127,6 +127,19 @@ bool SubstraitToVeloxPlanValidator::validateRound(
return false;
}
+ const auto& valueType = params[0]->type();
Review Comment:
`validateRound` dereferences `params[0]` without checking `params.size()`.
If the converted-params vector is ever empty/mismatched (e.g., due to earlier
conversion failure or a caller bug), this will crash instead of returning
`false`. Add an explicit `params.size()` guard (ideally `>= 2` since ROUND is
binary) before accessing `params[0]` (and/or any future `params[1]`).
##########
cpp/velox/substrait/SubstraitToVeloxPlanValidator.h:
##########
@@ -148,8 +148,11 @@ class SubstraitToVeloxPlanValidator {
/// Validate aggregate rel.
bool validateAggRelFunctionType(const ::substrait::AggregateRel&
substraitAgg);
- /// Validate the round scalar function.
- bool validateRound(const ::substrait::Expression::ScalarFunction&
scalarFunction, const RowTypePtr& inputType);
+ /// Validate the round scalar function using its already-converted arguments.
+ /// The caller supplies one non-null expression for each Substrait argument.
Review Comment:
The updated comment suggests validation is performed using the
already-converted arguments, but the implementation still inspects the raw
Substrait argument literal for the scale (and only uses `params[0]` for type).
Consider clarifying the docstring to reflect that scale validation is performed
against the Substrait literal encoding (NULL/i32), while the value type is
derived from `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:
Hardcoding a specific upstream Velox PR id in a repo script will
unexpectedly redirect all consumers (including CI/devs who don’t intend to test
that PR) to fetch a non-default Velox source. Since the PR description calls
this a temporary developer hook, this should be reverted back to an empty
default (or gated behind an explicit env var/flag) before merge, so normal
builds remain deterministic and don’t depend on a specific upstream PR.
##########
backends-velox/src/main/scala/org/apache/gluten/backendsapi/velox/VeloxValidatorApi.scala:
##########
@@ -38,12 +39,32 @@ 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 scale = round.scale
+ val supported =
+ if (scale.dataType != IntegerType || !scale.foldable) {
+ false
+ } else {
+ scale.eval(EmptyRow) match {
+ case null => true
+ case value: Int => value >= -400 && value <= 400
+ case _ => false
+ }
+ }
Review Comment:
Calling `scale.eval(EmptyRow)` during validation can throw (e.g.,
constant-foldable casts that fail, or other foldable expressions with
evaluation errors). That would make planning/validation fail hard instead of
cleanly returning `false` and letting Spark execute (and report the error) on
the fallback path. Wrap the eval in a safe try/catch (e.g., NonFatal) and treat
exceptions as `supported = false` (optionally logging at debug).
--
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]
