Copilot commented on code in PR #13223:
URL: https://github.com/apache/gluten/pull/13223#discussion_r4268439600
##########
backends-velox/src/main/scala/org/apache/gluten/backendsapi/velox/VeloxValidatorApi.scala:
##########
@@ -42,8 +43,13 @@ class VeloxValidatorApi extends ValidatorApi {
import VeloxValidatorApi._
/** For velox backend, key validation is on native side. */
- override def doExprValidate(substraitExprName: String, expr: Expression):
Boolean =
- true
+ override def doExprValidate(substraitExprName: String, expr: Expression):
Boolean = {
+ expr match {
+ case formatString: FormatString =>
+ supportsFormatString(formatString)
+ case _ => true
Review Comment:
This only validates `FormatString`. Spark’s `printf(...)` is commonly
represented by a separate Catalyst expression (e.g., `Printf`), which would
currently fall through to `true` and could be incorrectly treated as natively
supported even when the pattern is dynamic/unsupported. Consider
matching/validating the `printf` expression too (or using `substraitExprName`
to apply the same literal-pattern validation for both `format_string` and
`printf`).
##########
backends-velox/src/test/scala/org/apache/gluten/execution/VeloxFormatStringSuite.scala:
##########
@@ -0,0 +1,212 @@
+/*
+ * 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 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 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)
+ }
+ }
+
Review Comment:
The suite asserts `printf` can execute natively for a supported literal, but
there’s no coverage that `printf` correctly falls back for the same
unsupported/dynamic cases covered for `format_string` (e.g., dynamic pattern,
unsupported conversions, excessive width, malformed specifiers). Adding at
least one `printf` fallback assertion would help ensure the validator/converter
handles both functions consistently.
##########
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]
+ var index = 0
+ while (index < format.length) {
Review Comment:
`ArrayBuffer` is used here but no import is shown in this diff. Unless it
already exists elsewhere in the file, this won’t compile. Add `import
scala.collection.mutable.ArrayBuffer` (or fully-qualify
`scala.collection.mutable.ArrayBuffer`).
--
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]
