luis4a0 commented on code in PR #13047: URL: https://github.com/apache/gluten/pull/13047#discussion_r4092021840
########## backends-velox/src/test/scala/org/apache/gluten/backendsapi/velox/VeloxValidatorApiSuite.scala: ########## @@ -0,0 +1,149 @@ +/* + * 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.gluten.backendsapi.velox + +import org.apache.gluten.backendsapi.velox.VeloxValidatorApi._ + +import org.apache.spark.sql.catalyst.expressions.{BoundReference, BRound, Literal, Round} +import org.apache.spark.sql.types._ + +import org.scalatest.funsuite.AnyFunSuite + +import scala.util.Properties + +class VeloxValidatorApiSuite extends AnyFunSuite { + private val validator = new VeloxValidatorApi + + test("bround compatibility constants match the qualified native contract") { + assert(MIN_BROUND_SCALE == -400) + assert(MAX_BROUND_SCALE == 400) + assert(MIN_BROUND_FLOATING_JAVA_VERSION == "21") + } + + test("floating bround with nonzero scale requires the qualified JVM conversion") { + Seq(FloatType, DoubleType).foreach { + dataType => + Seq(-3, 2).foreach { + scale => + val expression = + new BRound(BoundReference(0, dataType, nullable = true), Literal(scale)) + assert( + validator.doExprValidate("bround", expression) == + Properties.isJavaAtLeast(MIN_BROUND_FLOATING_JAVA_VERSION)) Review Comment: Addressed in https://github.com/apache/gluten/commit/aee918260a5b95049f35f6f144a0de30f02a6b3f. Every BROUND construction in this validator suite now passes `ansiEnabled = false` explicitly, including the cache, NULL/zero-scale, boundary and nonconstant-scale cases. The paired ROUND control also supplies its mode explicitly. This makes the test setup independent of the constructor's ambient SQLConf default without changing the validator's eligibility rules. All 12 API tests and the unchanged 9 BROUND integration tests pass on both Java 17 and Java 21. ########## gluten-ut/spark41/src/test/scala/org/apache/spark/sql/GlutenBRoundSuite.scala: ########## @@ -0,0 +1,326 @@ +/* + * 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.BRound +import org.apache.spark.sql.catalyst.optimizer.{ConstantFolding, NullPropagation} +import org.apache.spark.sql.internal.SQLConf +import org.apache.spark.sql.types._ + +import scala.jdk.CollectionConverters._ +import scala.util.Properties + +class GlutenBRoundSuite extends GlutenSQLTestsTrait { + private def withInput(schema: StructType, rows: Seq[Row])(f: => Unit): Unit = { + withSQLConf( + SQLConf.ADAPTIVE_EXECUTION_ENABLED.key -> "false", + GlutenConfig.GLUTEN_ANSI_FALLBACK_ENABLED.key -> "false") { + withTempPath { + path => + withTempView("bround_input") { + spark + .createDataFrame(rows.asJava, schema) + .coalesce(1) + .write + .parquet(path.getCanonicalPath) + spark.read.parquet(path.getCanonicalPath).createOrReplaceTempView("bround_input") + f + } + } + } + } + + private def isBroundFullyNative(df: DataFrame): Boolean = { + val plan = df.queryExecution.executedPlan + val hasNativeCall = plan.exists { + case project: ProjectExecTransformer => + project.projectList.exists(_.exists(_.isInstanceOf[BRound])) + case _ => false + } + val hasSparkCall = plan.exists { + case _: ProjectExecTransformer => false + case node => node.expressions.exists(_.exists(_.isInstanceOf[BRound])) + } + hasNativeCall && !hasSparkCall + } + + private def assertNativeBround(df: DataFrame): Unit = { + assert( + isBroundFullyNative(df), + s"BROUND was not executed natively:\n${df.queryExecution.executedPlan}") + } + + private def checkBround(query: String, expectNative: Boolean = true): Unit = { + val (expectedType, expected) = withSQLConf(GlutenConfig.GLUTEN_ENABLED.key -> "false") { + val df = sql(query) + (df.schema, df.collect().toSeq) + } + val df = sql(query) + assert( + isBroundFullyNative(df) == expectNative, + s"Unexpected BROUND execution path:\n${df.queryExecution.executedPlan}") + assert(df.schema == expectedType) + val actual = df.collect().toSeq + def exactRows(rows: Seq[Row]): Map[Seq[Any], Int] = { + rows + .map(_.toSeq.map { + case value: Double => java.lang.Double.doubleToRawLongBits(value) + case value: Float => java.lang.Float.floatToRawIntBits(value) + case value => value + }) + .groupMapReduce(identity)(_ => 1)(_ + _) + } Review Comment: Keeping the raw-bit comparison intentionally for this regression. Spark 4.1.1's BROUND implementation returns NaN inputs directly rather than performing rounding arithmetic on them, in both interpreted and generated execution: https://github.com/apache/spark/blob/v4.1.1/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/mathExpressions.scala#L1603-L1616 https://github.com/apache/spark/blob/v4.1.1/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/mathExpressions.scala#L1663-L1678 The corresponding native path also returns nonfinite input unchanged: https://github.com/facebookincubator/velox/blob/255fa7b95810060581f5b4789e818c54fb63a370/velox/functions/sparksql/BRound.cpp#L188-L193 This integration fixture uses canonical `Double.NaN` and its FLOAT conversion, not a collection of arbitrary payload encodings. Normalizing every NaN would weaken the deliberately exact Spark-versus-native passthrough check. The unchanged integration suite passes on Java 17 and Java 21, and the existing registered native quiet-NaN payload regression also passes. This is a targeted compatibility assertion, not a claim that IEEE-754 requires identical payloads for every operation on every platform. ########## backends-velox/src/test/scala/org/apache/gluten/backendsapi/velox/VeloxBRoundTransformerSuite.scala: ########## @@ -0,0 +1,65 @@ +/* + * 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.gluten.backendsapi.velox + +import org.apache.gluten.expression.LiteralTransformer +import org.apache.gluten.substrait.SubstraitContext + +import org.apache.spark.sql.catalyst.expressions.{BRound, Literal} +import org.apache.spark.sql.types.Decimal + +import org.scalatest.funsuite.AnyFunSuite + +class VeloxBRoundTransformerSuite extends AnyFunSuite { + private val api = new VeloxSparkPlanExecApi + + test("integral bround carries the expression's ANSI mode in a native literal") { + Seq(Literal(127.toByte), Literal(32767.toShort), Literal(Int.MaxValue), Literal(Long.MaxValue)) + .foreach { + value => + Seq(false, true).foreach { + ansi => + val scale = Literal(-1) + val original = new BRound(value, scale, ansi) + val transformed = api.genBRoundTransformer( + "bround", + Seq(LiteralTransformer(value), LiteralTransformer(scale)), + original) + val function = + transformed.doTransform(new SubstraitContext).toProtobuf.getScalarFunction + assert(function.getArgumentsCount == 3) + val mode = function.getArguments(2).getValue.getLiteral + assert(mode.hasBoolean) + assert(mode.getBoolean == ansi) + } + } + } + + test("floating and decimal bround retain their two-argument signatures") { + Seq(Literal(2.5f), Literal(2.5d), Literal(Decimal("2.5"))).foreach { + value => + val scale = Literal(1) + val original = new BRound(value, scale) + val transformed = api.genBRoundTransformer( + "bround", + Seq(LiteralTransformer(value), LiteralTransformer(scale)), + original) + val function = transformed.doTransform(new SubstraitContext).toProtobuf.getScalarFunction + assert(function.getArgumentsCount == 2) + } Review Comment: Addressed in https://github.com/apache/gluten/commit/aee918260a5b95049f35f6f144a0de30f02a6b3f. The floating/decimal transformer test now constructs BROUND with an explicit `ansiEnabled` argument and exercises both `false` and `true`, asserting that both retain the two-argument native signature. Integral coverage already supplies and checks both explicit modes. The 12 validator/transformer tests pass on Java 17 with ambient ANSI enabled and Java 21 with ambient legacy mode. No production behavior changed. -- 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]
