Copilot commented on code in PR #13223:
URL: https://github.com/apache/gluten/pull/13223#discussion_r4292446998
##########
backends-velox/src/main/scala/org/apache/gluten/backendsapi/velox/VeloxValidatorApi.scala:
##########
@@ -104,6 +110,151 @@ class VeloxValidatorApi extends ValidatorApi {
}
object VeloxValidatorApi {
+ private val nativeFormatFlags = Set('-', '+', ' ', '0')
+ // Match Velox's per-specifier output limit to prevent excessive allocations.
+ private val maxNativeFormatWidth = 1 << 20
+
+ private def supportsFormatString(formatString: FormatString): Boolean = {
+ val format = formatString.children.headOption match {
+ case Some(Literal(value: UTF8String, dataType))
+ if dataType.isInstanceOf[StringType] =>
+ value.toString
+ case _ =>
+ return false
+ }
+ val conversions = parseNativeFormatString(format).getOrElse {
+ return false
+ }
+ conversions.length <= formatString.children.length - 1 &&
+ conversions
+ .zip(formatString.children.tail)
+ .forall {
+ case (conversion, argument) =>
+ supportsNativeFormatType(conversion, argument.dataType)
+ }
+ }
+
+ private def parseNativeFormatString(format: String): Option[Seq[Char]] = {
+ val conversions = ArrayBuffer.empty[Char]
Review Comment:
`ArrayBuffer` is referenced unqualified here; if there isn't already a
file-level import for `scala.collection.mutable.ArrayBuffer`, this will not
compile. Consider either adding an explicit import near the top of the file or
qualifying the usage (`scala.collection.mutable.ArrayBuffer.empty[Char]`) so
the dependency is self-contained and resilient to future refactors.
##########
backends-velox/src/test/scala/org/apache/gluten/execution/VeloxFormatStringSuite.scala:
##########
@@ -0,0 +1,223 @@
+/*
+ * 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.execution
+
+import org.apache.gluten.test.FallbackUtil
+
+import org.apache.spark.sql.{DataFrame, Row}
+
+class VeloxFormatStringSuite extends VeloxWholeStageTransformerSuite {
+
+ override protected val resourcePath: String = "/tpch-data-parquet"
+ override protected val fileFormat: String = "parquet"
+
+ private def withFormatValues(f: => Unit): Unit = {
+ withTempView("format_values") {
+ withTempPath {
+ path =>
+ spark.range(0, 3).write.parquet(path.getAbsolutePath)
+
spark.read.parquet(path.getAbsolutePath).createOrReplaceTempView("format_values")
+ f
+ }
+ }
+ }
+
+ private def assertFallback(sql: String): Unit = {
+ val dataFrame = spark.sql(sql)
+ dataFrame.collect()
+ assert(
+ FallbackUtil.hasFallback(dataFrame.queryExecution.executedPlan),
+ s"Expected format_string/printf fallback:
${dataFrame.queryExecution.executedPlan}")
+ }
+
+ private def assertFallbackWithoutExecution(sql: String): Unit = {
+ val executedPlan = spark.sql(sql).queryExecution.executedPlan
+ assert(
+ FallbackUtil.hasFallback(executedPlan),
+ s"Expected format_string/printf fallback: $executedPlan")
+ }
+
+ private def assertNative(dataFrame: DataFrame): Unit = {
+ val executedPlan = dataFrame.queryExecution.executedPlan
+ assert(
+ !FallbackUtil.hasFallback(executedPlan),
+ s"Expected native format_string execution: $executedPlan")
+ assert(
+ executedPlan.find(_.isInstanceOf[ProjectExecTransformer]).isDefined,
+ s"Expected a native project for format_string: $executedPlan")
+ }
+
+ test("format_string and printf share native integer formatting") {
+ withFormatValues {
+ runQueryAndCompare("""
+ |SELECT
+ | format_string('id=%04d', CAST(id AS INT)),
+ | printf('id=%04d', CAST(id AS INT))
+ |FROM format_values
+ |""".stripMargin)(assertNative)
+ }
+ }
+
+ test("format_string offloads verified integral conversions") {
+ withFormatValues {
+ runQueryAndCompare("""
+ |SELECT format_string(
+ | '%d|%05d|%o|%x|%X',
+ | CAST(id AS INT),
+ | CAST(id AS INT),
+ | CAST(id AS INT),
+ | CAST(id AS INT),
+ | CAST(id AS INT))
+ |FROM format_values
+ |""".stripMargin)(assertNative)
+ }
+ }
+
+ test("format_string offloads strings, booleans, integrals, and nulls with
percent-s") {
+ withFormatValues {
+ runQueryAndCompare("""
+ |SELECT format_string(
+ | '%s|%s|%s|%s',
+ | CAST(id AS STRING),
+ | id % 2 = 0,
+ | id,
+ | CASE WHEN id = 1 THEN NULL ELSE CAST(id AS
STRING) END)
+ |FROM format_values
+ |""".stripMargin)(assertNative)
+ }
+ }
+
+ test("format_string offloads padded nullable BIGINT values") {
+ withFormatValues {
+ runQueryAndCompare("""
+ |SELECT format_string(
+ | '%+05d|%05X|%-5d',
+ | CASE WHEN id = 1 THEN CAST(NULL AS BIGINT) ELSE
id END,
+ | CASE WHEN id = 1 THEN CAST(NULL AS BIGINT) ELSE
id END,
+ | CASE WHEN id = 1 THEN CAST(NULL AS BIGINT) ELSE
id END)
+ |FROM format_values
+ |""".stripMargin)(assertNative)
+ }
+ }
+
+ Seq("format_string", "printf").foreach {
+ functionName =>
+ test(s"$functionName falls back outside the verified compatibility
subset") {
+ withFormatValues {
+ Seq(
+ "CASE WHEN id = 0 THEN '%d' ELSE '%x' END, id",
+ "'%b', id = 0",
+ "'%c', CAST(id + 65 AS INT)",
+ "'%g', CAST(id AS DOUBLE) + 0.25D",
+ "'%.2f', CAST(id AS DOUBLE) + 0.25D",
+ "'%.3e', CAST(id AS DOUBLE) + 0.25D",
+ "'%.3E', CAST(id AS DOUBLE) + 0.25D",
+ "'%1$s', CAST(id AS STRING)",
+ "'%10s', CAST(id AS STRING)",
+ "'%s', CAST(id AS DOUBLE)",
+ "'%f', CAST(id AS DOUBLE) + 0.25D"
+ ).foreach {
+ arguments => assertFallback(s"SELECT $functionName($arguments)
FROM format_values")
+ }
+ }
+ }
+ }
+
+ test("format_string handles null arguments and uppercase integral
conversions") {
+ withFormatValues {
+ val dataFrame =
+ runQueryAndCompare("""
+ |SELECT format_string(
+ | '%d|%X',
+ | CASE WHEN id = 1 THEN NULL ELSE CAST(id AS
INT) END,
+ | CASE WHEN id = 1 THEN NULL ELSE CAST(id AS
INT) END)
+ |FROM format_values
+ |""".stripMargin)(assertNative)
+ checkAnswer(dataFrame, Seq(Row("0|0"), Row("null|NULL"), Row("2|2")))
+ }
+ }
+
+ test("format_string accepts the maximum native width") {
+ withFormatValues {
+ val dataFrame =
+ runQueryAndCompare("""
+ |SELECT length(format_string('%1048576d', CAST(id
AS INT)))
+ |FROM format_values
+ |WHERE id = 0
+ |""".stripMargin)(assertNative)
+ checkAnswer(dataFrame, Row(1048576))
Review Comment:
This test forces construction of a 1,048,576-character string, which can add
noticeable CPU/memory pressure and increase CI flakiness (GC pressure, executor
memory) even though it’s only one row. Consider making this a boundary-only
test that avoids materializing the full string (e.g., validate native planning
for `%1048576d` without execution, and keep an executed test at a smaller but
still meaningful width), while retaining the `%1048577d` fallback test as the
“excessive width” guard.
##########
backends-velox/src/test/scala/org/apache/gluten/execution/VeloxFormatStringSuite.scala:
##########
@@ -0,0 +1,223 @@
+/*
+ * 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.execution
+
+import org.apache.gluten.test.FallbackUtil
+
+import org.apache.spark.sql.{DataFrame, Row}
+
+class VeloxFormatStringSuite extends VeloxWholeStageTransformerSuite {
+
+ override protected val resourcePath: String = "/tpch-data-parquet"
+ override protected val fileFormat: String = "parquet"
+
+ private def withFormatValues(f: => Unit): Unit = {
+ withTempView("format_values") {
+ withTempPath {
+ path =>
+ spark.range(0, 3).write.parquet(path.getAbsolutePath)
+
spark.read.parquet(path.getAbsolutePath).createOrReplaceTempView("format_values")
+ f
+ }
+ }
+ }
+
+ private def assertFallback(sql: String): Unit = {
+ val dataFrame = spark.sql(sql)
+ dataFrame.collect()
+ assert(
+ FallbackUtil.hasFallback(dataFrame.queryExecution.executedPlan),
+ s"Expected format_string/printf fallback:
${dataFrame.queryExecution.executedPlan}")
+ }
+
+ private def assertFallbackWithoutExecution(sql: String): Unit = {
+ val executedPlan = spark.sql(sql).queryExecution.executedPlan
+ assert(
+ FallbackUtil.hasFallback(executedPlan),
+ s"Expected format_string/printf fallback: $executedPlan")
+ }
+
+ private def assertNative(dataFrame: DataFrame): Unit = {
+ val executedPlan = dataFrame.queryExecution.executedPlan
+ assert(
+ !FallbackUtil.hasFallback(executedPlan),
+ s"Expected native format_string execution: $executedPlan")
+ assert(
+ executedPlan.find(_.isInstanceOf[ProjectExecTransformer]).isDefined,
+ s"Expected a native project for format_string: $executedPlan")
+ }
+
+ test("format_string and printf share native integer formatting") {
+ withFormatValues {
+ runQueryAndCompare("""
+ |SELECT
+ | format_string('id=%04d', CAST(id AS INT)),
+ | printf('id=%04d', CAST(id AS INT))
+ |FROM format_values
+ |""".stripMargin)(assertNative)
+ }
+ }
+
+ test("format_string offloads verified integral conversions") {
+ withFormatValues {
+ runQueryAndCompare("""
+ |SELECT format_string(
+ | '%d|%05d|%o|%x|%X',
+ | CAST(id AS INT),
+ | CAST(id AS INT),
+ | CAST(id AS INT),
+ | CAST(id AS INT),
+ | CAST(id AS INT))
+ |FROM format_values
+ |""".stripMargin)(assertNative)
+ }
+ }
+
+ test("format_string offloads strings, booleans, integrals, and nulls with
percent-s") {
+ withFormatValues {
+ runQueryAndCompare("""
+ |SELECT format_string(
+ | '%s|%s|%s|%s',
+ | CAST(id AS STRING),
+ | id % 2 = 0,
+ | id,
+ | CASE WHEN id = 1 THEN NULL ELSE CAST(id AS
STRING) END)
+ |FROM format_values
+ |""".stripMargin)(assertNative)
+ }
+ }
+
+ test("format_string offloads padded nullable BIGINT values") {
+ withFormatValues {
+ runQueryAndCompare("""
+ |SELECT format_string(
+ | '%+05d|%05X|%-5d',
+ | CASE WHEN id = 1 THEN CAST(NULL AS BIGINT) ELSE
id END,
+ | CASE WHEN id = 1 THEN CAST(NULL AS BIGINT) ELSE
id END,
+ | CASE WHEN id = 1 THEN CAST(NULL AS BIGINT) ELSE
id END)
+ |FROM format_values
+ |""".stripMargin)(assertNative)
+ }
+ }
+
+ Seq("format_string", "printf").foreach {
+ functionName =>
+ test(s"$functionName falls back outside the verified compatibility
subset") {
+ withFormatValues {
+ Seq(
+ "CASE WHEN id = 0 THEN '%d' ELSE '%x' END, id",
+ "'%b', id = 0",
+ "'%c', CAST(id + 65 AS INT)",
+ "'%g', CAST(id AS DOUBLE) + 0.25D",
+ "'%.2f', CAST(id AS DOUBLE) + 0.25D",
+ "'%.3e', CAST(id AS DOUBLE) + 0.25D",
+ "'%.3E', CAST(id AS DOUBLE) + 0.25D",
+ "'%1$s', CAST(id AS STRING)",
+ "'%10s', CAST(id AS STRING)",
+ "'%s', CAST(id AS DOUBLE)",
+ "'%f', CAST(id AS DOUBLE) + 0.25D"
+ ).foreach {
+ arguments => assertFallback(s"SELECT $functionName($arguments)
FROM format_values")
+ }
+ }
+ }
+ }
Review Comment:
These fallback tests assert that a fallback happened, but they don’t
leverage `runQueryAndCompare`, so they don’t explicitly validate result parity
against vanilla Spark for these unsupported/dynamic cases (despite the PR
description calling out “native and fallback execution tests that compare
results”). Consider rewriting the helper to accept a `DataFrame` so you can
call `runQueryAndCompare(...)(df => assertFallback(df))`, ensuring both (1)
fallback is exercised and (2) results are compared consistently by the existing
harness.
--
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]
