Copilot commented on code in PR #13103:
URL: https://github.com/apache/gluten/pull/13103#discussion_r4523397743
##########
cpp/velox/substrait/SubstraitToVeloxPlanValidator.cc:
##########
@@ -141,7 +141,7 @@ bool SubstraitToVeloxPlanValidator::flattenDualLevel(const
TypePtr& type, std::v
bool SubstraitToVeloxPlanValidator::validateRound(
const ::substrait::Expression::ScalarFunction& scalarFunction,
- const RowTypePtr& inputType) {
+ const std::vector<core::TypedExprPtr>& params) {
Review Comment:
`params[0]` is accessed without validating `params.size()`. If
`validateScalarFunction` ever calls `validateRound` with an incomplete `params`
vector (e.g., due to earlier conversion failure or arity mismatch), this will
be an out-of-bounds access. Add an explicit `params.size() >= 2` (or at least
`>= 1`) check alongside the existing Substrait `arguments` checks before
indexing.
##########
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();
Review Comment:
`params[0]` is accessed without validating `params.size()`. If
`validateScalarFunction` ever calls `validateRound` with an incomplete `params`
vector (e.g., due to earlier conversion failure or arity mismatch), this will
be an out-of-bounds access. Add an explicit `params.size() >= 2` (or at least
`>= 1`) check alongside the existing Substrait `arguments` checks before
indexing.
##########
gluten-substrait/src/main/scala/org/apache/gluten/expression/DecimalRoundTransformer.scala:
##########
@@ -20,15 +20,16 @@ import org.apache.gluten.backendsapi.BackendsApiManager
import org.apache.gluten.exception.GlutenNotSupportException
import org.apache.spark.sql.catalyst.expressions._
-import org.apache.spark.sql.types.{DataType, DecimalType}
+import org.apache.spark.sql.types.{DataType, DecimalType, IntegerType}
case class DecimalRoundTransformer(
substraitExprName: String,
child: ExpressionTransformer,
original: Round)
extends BinaryExpressionTransformer {
- val toScale: Int = original.scale.eval(EmptyRow).asInstanceOf[Int]
+ private val scaleValue = original.scale.eval(EmptyRow)
+ val toScale: Int = if (scaleValue == null) 0 else
scaleValue.asInstanceOf[Int]
Review Comment:
`original.scale.eval(EmptyRow)` can throw for foldable-but-invalid
expressions (e.g., ANSI cast failures). Eagerly evaluating at transformer
construction time can surface planning-time errors instead of allowing Spark
fallback. Consider wrapping the eval in a try/catch and converting failures
into a non-support signal (e.g., throwing a `GlutenNotSupportException` /
returning an unsupported transformer) so the engine can fall back rather than
failing during plan construction.
##########
gluten-ut/common/src/test/scala/org/apache/spark/sql/GlutenDecimalRoundSuiteBase.scala:
##########
@@ -0,0 +1,193 @@
+/*
+ * 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"))
+ })
Review Comment:
This assertion hard-codes a substring match on the native error message
(`"Decimal overflow"`), which is brittle across Velox/Gluten versions and
potentially across execution paths. If there is a stable error class/code
exposed for native failures, prefer asserting on that; otherwise consider
loosening this to a less specific predicate (or centralizing the expected
message token) to reduce test flakiness.
##########
gluten-substrait/src/main/scala/org/apache/gluten/expression/DecimalRoundTransformer.scala:
##########
@@ -43,5 +44,6 @@ case class DecimalRoundTransformer(
}
override def left: ExpressionTransformer = child
- override def right: ExpressionTransformer = LiteralTransformer(toScale)
+ override def right: ExpressionTransformer =
+ LiteralTransformer(Literal.create(scaleValue, IntegerType))
Review Comment:
`original.scale.eval(EmptyRow)` can throw for foldable-but-invalid
expressions (e.g., ANSI cast failures). Eagerly evaluating at transformer
construction time can surface planning-time errors instead of allowing Spark
fallback. Consider wrapping the eval in a try/catch and converting failures
into a non-support signal (e.g., throwing a `GlutenNotSupportException` /
returning an unsupported transformer) so the engine can fall back rather than
failing during plan construction.
--
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]
