luis4a0 commented on code in PR #13282:
URL: https://github.com/apache/gluten/pull/13282#discussion_r4269741362
##########
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:
Updated the diagnostic to state the accepted contract directly: a folded
INTEGER scale in `[0, 400]` or NULL. This covers out-of-range positive values
as well as negative and unsupported scale expressions, without changing
eligibility. Commit:
https://github.com/apache/gluten/commit/c8cef1c47c048aaa312bbe8786f75db41e01de9e
##########
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:
Removed the redundant conversion by reusing the typed arguments already
prepared by `validateScalarFunction`, following the existing
`validateExtractExpr(params)` pattern. The caller already validates/converts
every argument before dispatch, so direct-field-only inference would not avoid
that existing work and would unnecessarily reject supported computed children.
Added a VARCHAR-to-TINYINT cast regression to verify that the computed result
type controls eligibility. Commit:
https://github.com/apache/gluten/commit/c8cef1c47c048aaa312bbe8786f75db41e01de9e
Applied the same cleanup to the decimal companion, with VARCHAR-to-DECIMAL
regressions for negative and NULL scales:
https://github.com/apache/gluten/commit/1f5c19a7a7e7f9bd62f305903e98438d9a4305b2
--
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]
