Copilot commented on code in PR #13047: URL: https://github.com/apache/gluten/pull/13047#discussion_r4082318526
########## 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: Comparing floats/doubles via raw bits makes the test very strict for `NaN` values: IEEE-754 allows multiple NaN payload bit patterns, and different implementations (or intermediate steps) can legally produce different NaN payloads while still being semantically NaN. To reduce brittleness, consider normalizing NaNs (e.g., mapping any NaN to a single canonical bit pattern or sentinel) while still preserving strictness for non-NaN values (including `-0.0`). ########## 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: This test constructs `new BRound(value, scale)` without explicitly setting `ansiEnabled`. In Spark, the default for `ansiEnabled` is typically derived from `SQLConf` (often thread-local), which can make the unit test sensitive to ambient configuration set by other tests. Prefer constructing `BRound` with an explicit `ansiEnabled` value (or wrapping the test in a known `SQLConf`) to keep the test deterministic. ########## 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: Similar to the transformer suite, these tests construct `BRound` without explicitly setting `ansiEnabled`. Even though `doExprValidate` doesn’t currently depend on `ansiEnabled`, the expression’s default may still be pulled from `SQLConf`, which can introduce hidden coupling to ambient test state. Consider passing an explicit `ansiEnabled` value in all `BRound` constructions in this suite for stability. -- 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]
