luis4a0 commented on code in PR #13282: URL: https://github.com/apache/gluten/pull/13282#discussion_r4281933640
########## 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: This repeats the dependency/merge gate already tracked at https://github.com/apache/gluten/pull/13282#discussion_r4269051013 . The temporary validation hook must be removed when a qualified normal dependency contains the upstream integral implementation. Closing this duplicate discussion only; the original thread remains open to track that required pre-merge step. ########## 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: Checked the complete call/conversion path: this is a private helper with one caller. `validateScalarFunction` appends one successfully converted expression per Substrait argument and dispatches only after that loop completes. Failed child validation returns before dispatch; conversion failures throw and are not caught in a way that continues to `validateRound`. Successful converter branches return constructed expressions, including non-null expression objects for NULL literals. The existing `arguments.size() < 2` guard therefore also protects `params[0]` under this invariant. Caller: https://github.com/apache/gluten/blob/7e4a3a64f6c6e2f6ba38cd00f3a3cd666b09f54b/cpp/velox/substrait/SubstraitToVeloxPlanValidator.cc#L205-L225 Converter dispatch: https://github.com/apache/gluten/blob/7e4a3a64f6c6e2f6ba38cd00f3a3cd666b09f54b/cpp/velox/substrait/SubstraitToVeloxExpr.cc#L693-L712 Documented the non-null, argument-aligned precondition at the helper declaration in https://github.com/apache/gluten/commit/7e4a3a64f6c6e2f6ba38cd00f3a3cd666b09f54b . No silent fallback for an unreachable parameter state was added. ########## 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: Corrected the ScalaDoc to describe JVM-side integral ROUND eligibility and captured ANSI preservation independently of optional native plan validation: https://github.com/apache/gluten/commit/7e4a3a64f6c6e2f6ba38cd00f3a3cd666b09f54b . Applied the corresponding decimal-scale clarification to the decimal companion as well: https://github.com/apache/gluten/commit/28d77cb4ddd683c1f48059d84d6f876350cffc11 . These are documentation-only changes; eligibility and execution behavior are unchanged. -- 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]
