luis4a0 commented on code in PR #13103:
URL: https://github.com/apache/gluten/pull/13103#discussion_r4522944235
##########
cpp/velox/substrait/SubstraitToVeloxPlanValidator.cc:
##########
@@ -152,6 +152,19 @@ bool SubstraitToVeloxPlanValidator::validateRound(
return false;
}
+ const auto& valueType = params[0]->type();
+ if (valueType->isDecimal()) {
+ const auto& scale = arguments[1].value().literal();
+ if (scale.has_null() && scale.null().has_i32()) {
+ return true;
+ }
+ if (scale.has_i32() && scale.i32() >= -400 && scale.i32() <= 400) {
+ return true;
+ }
+ LOG_VALIDATION_MSG("Decimal ROUND requires an INTEGER scale in [-400, 400]
or NULL.");
+ return false;
+ }
Review Comment:
The split is intentional. `params[0]->type()` uses the converted value
expression so decimal inputs are recognized through casts and other nontrivial
expressions. The scale policy, however, validates the final serialized
Substrait contract: it must be an INTEGER literal (including typed `null.i32`)
in the qualified range. Inspecting `params[1]` would couple eligibility to
Velox expression subclasses and could broaden accepted plans independently of
what Gluten serialized. Tests cover typed NULL and foldable arithmetic
serialized as literals, plus fallback for nonliteral scales. No change is
needed.
##########
gluten-ut/common/src/test/scala/org/apache/spark/sql/GlutenDecimalRoundSuiteBase.scala:
##########
@@ -0,0 +1,191 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.spark.sql
+
+import org.apache.gluten.config.GlutenConfig
+import org.apache.gluten.execution.ProjectExecTransformer
+
+import org.apache.spark.sql.catalyst.expressions.{Literal, Round}
+import org.apache.spark.sql.catalyst.optimizer.{ConstantFolding,
NullPropagation}
+import org.apache.spark.sql.internal.SQLConf
+import org.apache.spark.sql.types._
+
+import scala.collection.JavaConverters._
+
+abstract class GlutenDecimalRoundSuiteBase extends GlutenSQLTestsTrait {
+ private def withInput(dataType: DecimalType, values: Seq[String])(f: =>
Unit): Unit = {
+ val schema = new StructType().add("value", dataType)
+ val rows = values.map(value => Row(new java.math.BigDecimal(value))) :+
Row(null)
+ withSQLConf(
+ SQLConf.ADAPTIVE_EXECUTION_ENABLED.key -> "false",
+ GlutenConfig.GLUTEN_ANSI_FALLBACK_ENABLED.key -> "false") {
+ withTempPath {
+ path =>
+ spark.createDataFrame(rows.asJava,
schema).coalesce(1).write.parquet(path.toString)
+ withTempView("decimal_round_input") {
+
spark.read.parquet(path.toString).createOrReplaceTempView("decimal_round_input")
+ f
+ }
+ }
+ }
+ }
+
+ private def assertExecution(df: DataFrame, native: Boolean): Unit = {
+ val plan = df.queryExecution.executedPlan
+ val locations = plan.collect {
+ case node if node.expressions.exists(_.exists(_.isInstanceOf[Round])) =>
+ node.isInstanceOf[ProjectExecTransformer]
+ }
+ assert(
+ locations.nonEmpty && locations.forall(_ == native),
+ s"Unexpected ROUND execution:\n$plan")
+ }
+
+ private def checkRound(scale: String, native: Boolean = true): Unit = {
+ val query = s"SELECT round(value, $scale) FROM decimal_round_input"
+ withSQLConf(GlutenConfig.GLUTEN_ENABLED.key -> "false") {
+ val vanilla = sql(query)
+ val expectedSchema = vanilla.schema
+ val expected = vanilla.collect().toSeq
+ withSQLConf(GlutenConfig.GLUTEN_ENABLED.key -> "true") {
+ val df = sql(query)
+ assertExecution(df, native)
+ assert(df.schema == expectedSchema)
+ checkAnswer(df, expected)
+ }
+ }
+ }
+
+ testGluten("decimal ROUND preserves schemas and HALF_UP results across
storage boundaries") {
+ val cases = Seq(
+ (DecimalType(3, 1), Seq("2.5", "-2.5", "0")),
+ (DecimalType(18, 0), Seq("999999999999999999", "-999999999999999999")),
+ (DecimalType(19, 0), Seq("9999999999999999999", "-9999999999999999999")),
+ (DecimalType(38, 1), Seq("9" * 37 + ".9", "-" + "9" * 37 + ".9")),
+ (DecimalType(38, 38), Seq("0.6", "-0.6", "0.5", "-0.5"))
+ )
+ for ((dataType, values) <- cases) {
+ withInput(dataType, values) {
+ for (ansi <- Seq("false", "true")) {
+ withSQLConf(SQLConf.ANSI_ENABLED.key -> ansi) {
+ Seq(-400, -39, -1, 0, 1, 38, 400).foreach(scale =>
checkRound(scale.toString))
+ checkRound("-1 + 0")
+ }
+ }
+ }
+ }
+ }
+
+ testGluten("decimal ROUND handles the 38-digit HALF_UP tie and precision
overflow") {
+ withInput(DecimalType(38, 0), Seq("4" + "9" * 37, "-4" + "9" * 37)) {
+ checkRound("-38")
+ }
+ for ((value, scale) <- Seq(("9" * 38, -1), ("5" + "0" * 37, -38), ("-5" +
"0" * 37, -38))) {
+ withInput(DecimalType(38, 0), Seq(value)) {
+ for (ansi <- Seq("false", "true")) {
+ withSQLConf(SQLConf.ANSI_ENABLED.key -> ansi) {
+ val query = s"SELECT round(value, $scale) FROM decimal_round_input"
+ withSQLConf(GlutenConfig.GLUTEN_ENABLED.key -> "false") {
+ val error = intercept[Exception](sql(query).collect())
+
assert(Iterator.iterate[Throwable](error)(_.getCause).takeWhile(_ !=
null).exists {
+ cause =>
+ Option(cause.getMessage).exists {
+ message =>
+ message.contains("NUMERIC_VALUE_OUT_OF_RANGE") ||
+
message.contains("DECIMAL_PRECISION_EXCEEDS_MAX_PRECISION")
+ }
+ })
+ }
+ val df = sql(query)
+ assertExecution(df, native = true)
+ val error = intercept[Exception](df.collect())
+ assert(Iterator.iterate[Throwable](error)(_.getCause).takeWhile(_
!= null).exists {
+ cause => Option(cause.getMessage).exists(_.contains("Decimal
overflow"))
+ })
+ }
+ }
+ }
+ }
+ }
+
+ testGluten("decimal ROUND NULL scale skips an overflowing child natively") {
+ withInput(DecimalType(38, 0), Seq("9" * 38)) {
+ for (ansi <- Seq("false", "true")) {
+ withSQLConf(
+ SQLConf.ANSI_ENABLED.key -> ansi,
+ GlutenConfig.NATIVE_VALIDATION_ENABLED.key -> "true",
+ SQLConf.OPTIMIZER_EXCLUDED_RULES.key ->
+ s"${ConstantFolding.ruleName},${NullPropagation.ruleName}"
+ ) {
+ val query =
+ "SELECT round(round(value, -1), CAST(NULL AS INT)) FROM
decimal_round_input"
+ val (expectedSchema, expected) =
+ withSQLConf(GlutenConfig.GLUTEN_ENABLED.key -> "false") {
+ val vanilla = sql(query)
+ (vanilla.schema, vanilla.collect().toSeq)
+ }
+ val df = sql(query)
+ assertExecution(df, native = true)
+ assert(df.schema == expectedSchema)
+ checkAnswer(df, expected)
+ }
+ }
+ }
+ }
+
+ testGluten("decimal ROUND debug mode bypasses compatibility fallback") {
+ withInput(DecimalType(3, 1), Seq("2.5", "-2.5", "0")) {
+ withSQLConf(
+ GlutenConfig.NATIVE_VALIDATION_ENABLED.key -> "false",
+ SQLConf.OPTIMIZER_EXCLUDED_RULES.key -> NullPropagation.ruleName) {
+ checkRound("CAST(NULL AS INT)")
+ checkRound("-401")
+ checkRound("401")
Review Comment:
Scales ±401 are outside Gluten’s conservative normal-validation
qualification, but they are supported by the landed Velox implementation across
the full int32 scale domain. This debug-mode test intentionally bypasses the
compatibility policy to exercise that broader native capability, and
schema/result parity is valuable evidence that these specific native-supported
cases remain Spark-compatible. The documentation says unsupported operations
*may* fail when validation is disabled; it does not require accepting failure
for operations that Velox supports. I am keeping the stronger parity assertions.
##########
cpp/velox/substrait/SubstraitToVeloxPlanValidator.cc:
##########
@@ -152,6 +152,19 @@ bool SubstraitToVeloxPlanValidator::validateRound(
return false;
}
+ const auto& valueType = params[0]->type();
+ if (valueType->isDecimal()) {
Review Comment:
This state is excluded by the private caller contract.
`validateScalarFunction` reserves from `arguments().size()` and performs one
`params.emplace_back(...)` for every Substrait argument, returning before
`validateRound` if nested validation fails. The header explicitly documents one
non-null converted expression per argument. Hence `arguments.size() >= 2`
implies `params.size() >= 2` at this call. I am retaining the caller invariant
rather than adding a redundant defensive check.
##########
cpp/velox/substrait/SubstraitToVeloxPlanValidator.cc:
##########
@@ -127,6 +127,19 @@ bool SubstraitToVeloxPlanValidator::validateRound(
return false;
}
+ const auto& valueType = params[0]->type();
Review Comment:
Verified against the current private call path: `validateScalarFunction`
iterates `scalarFunction.arguments()` and appends exactly one converted,
non-null expression to `params` for each argument; if nested expression
validation fails, it returns before dispatching to `validateRound`. Therefore
`params.size() == arguments.size()` at this call, and the existing
`arguments.size() < 2` check guarantees `params[0]` exists. The header now
documents that caller invariant. A second size guard would duplicate an
invariant rather than protect a reachable path, so no code change is needed.
##########
cpp/velox/substrait/SubstraitToVeloxPlanValidator.cc:
##########
@@ -127,6 +127,19 @@ bool SubstraitToVeloxPlanValidator::validateRound(
return false;
}
+ const auto& valueType = params[0]->type();
Review Comment:
Verified against the current private call path: `validateScalarFunction`
creates `params` in a 1:1 loop over `scalarFunction.arguments()` and returns
before dispatch if nested validation fails. The converter contract supplies a
non-null expression for each argument, as documented in the header. Thus the
existing two-argument check also guarantees `params[0]` exists; an
independently mismatched vector is not a reachable caller state. No change is
needed.
--
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]
