Copilot commented on code in PR #13282:
URL: https://github.com/apache/gluten/pull/13282#discussion_r4269051087
##########
backends-velox/src/main/scala/org/apache/gluten/backendsapi/velox/VeloxValidatorApi.scala:
##########
@@ -38,12 +39,28 @@ 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 retains Spark execution for negative or
unqualified scales.")
Review Comment:
The debug message is misleading: `supported` is also false for out-of-range
positive literal scales (e.g., 401 / Int.MaxValue), not just 'negative or
unqualified'. Update the message to reflect all rejected cases (e.g., negative,
out-of-range, or non-literal/non-int32 scales) to make debugging validation
decisions accurate.
##########
cpp/velox/operators/functions/overlay/RegisterFunctionOverlay.cc:
##########
@@ -25,14 +25,10 @@ using namespace facebook;
namespace gluten {
namespace {
-// Spark's round differs from Velox's in the handling of negative decimals and
-// floating point rounding semantics.
+// Keep only the floating-point overlay; integral ROUND uses Velox's session
ANSI handling.
void registerRoundFunction() {
- velox::functions::registerUnaryNumeric<RoundFunction>({"round"});
- velox::registerFunction<RoundFunction, int8_t, int8_t, int32_t>({"round"});
- velox::registerFunction<RoundFunction, int16_t, int16_t, int32_t>({"round"});
- velox::registerFunction<RoundFunction, int32_t, int32_t, int32_t>({"round"});
- velox::registerFunction<RoundFunction, int64_t, int64_t, int32_t>({"round"});
+ velox::registerFunction<RoundFunction, double, double>({"round"});
+ velox::registerFunction<RoundFunction, float, float>({"round"});
velox::registerFunction<RoundFunction, double, double, int32_t>({"round"});
velox::registerFunction<RoundFunction, float, float, int32_t>({"round"});
}
Review Comment:
This comment suggests integral `ROUND` broadly 'uses Velox', but integral
execution is still constrained elsewhere (e.g.,
negative/unqualified/out-of-range scales fall back to Spark). Consider
tightening the comment to reflect the actual contract: only the integral
overlay is removed, and native integral execution remains gated by validation
rules to preserve Spark semantics where needed.
##########
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 = exprConverter_->toVeloxExpr(arguments[0].value(),
inputType)->type();
+ if (!valueType->isDecimal() &&
+ (valueType->isTinyint() || valueType->isSmallint() ||
valueType->isInteger() || valueType->isBigint())) {
+ return true;
+ }
+ }
Review Comment:
Validation is now converting `arguments[0]` into a Velox expression just to
determine its type. That adds overhead during plan validation and can also
cause validation-time failures if `toVeloxExpr` throws for an otherwise
fallback-only child expression. Prefer deriving the type from the Substrait
argument when it is a direct selection (i.e., field reference) and returning
`false` (or falling through) when the type can’t be determined cheaply, rather
than forcing a full conversion during validation.
##########
docs/velox-backend-scalar-function-support.md:
##########
@@ -297,7 +297,7 @@ The status applies to `spark.sql.ansi.enabled=false`. When
ANSI mode is enabled,
| randn | Randn | S | |
| random | Rand | S | |
| rint | Rint | S | |
-| round | Round | S | |
+| round | Round | PS | Integral scales in
[0, 400] and NULL use upstream Velox. NULL scales skip child evaluation.
Negative and unqualified extreme integral scales retain Spark's captured ANSI
behavior. |
Review Comment:
The phrase 'Negative and unqualified extreme integral scales' is ambiguous
(it reads like only 'extreme' scales that are 'unqualified'). Consider
rewording to explicitly list the categories, e.g., 'Negative scales,
out-of-range scales, and non-literal (unqualified) scales fall back to Spark to
preserve captured ANSI behavior.' This will better match the validator logic
and reduce confusion for users.
##########
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:
Hardcoding `UPSTREAM_VELOX_PR_ID` changes the default build behavior to
always pull a specific upstream PR, which is labeled 'Developer use only' in
the script comment and can unexpectedly affect CI/reproducibility. Prefer
leaving this empty by default and setting it via environment/CI configuration
(or gating it behind an explicit opt-in flag) so normal builds don’t silently
depend on a PR ref.
--
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]
