Copilot commented on code in PR #13103:
URL: https://github.com/apache/gluten/pull/13103#discussion_r4522802058
##########
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:
`validateRound` now indexes `params[0]` without validating `params.size()`.
If argument conversion fails upstream but `arguments` still has 2 entries, this
can read out-of-bounds and crash the validator. Add a guard like `if
(params.size() < 2) return false;` before accessing `params[0]` (and ideally
ensure `params[i]` is non-null per the header contract).
##########
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 function comment/header says `validateRound` validates using
already-converted arguments, but the decimal-path still inspects the raw
protobuf literal (`arguments[1].value().literal()`). This couples validation
logic to Substrait encoding details and makes it harder to evolve argument
conversion (e.g., handling casts/arithmetic consistently via `params[1]`).
Consider validating the scale using `params[1]` (e.g., allow constant null /
constant integer within bounds, reject non-constants) and keep protobuf
inspection as a fallback only if required.
##########
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:
These debug-mode assertions currently require full Spark parity
(`checkRound` compares schema + results against vanilla Spark) even for scales
explicitly treated as unsupported by the validator (e.g., ±401). This can make
the suite brittle and at odds with the config documentation stating unsupported
operations may fail at runtime when native validation is disabled. Consider
adjusting this test to (a) only assert native planning occurs, and (b) either
accept a runtime failure for out-of-range scales (intercept/verify error) or
avoid enforcing result parity for those out-of-range cases.
--
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]
