Copilot commented on code in PR #13204:
URL: https://github.com/apache/gluten/pull/13204#discussion_r4185509216
##########
gluten-substrait/src/main/scala/org/apache/gluten/execution/FileSourceScanExecTransformer.scala:
##########
@@ -188,9 +191,24 @@ abstract class FileSourceScanExecTransformerBase(
s"Unsupported matching schema column names " +
s"by field ids in native scan.")
}
+ if (fileFormat == ReadFileFormat.TextReadFormat &&
!csvFieldDelimiter.exists(isSingleAscii)) {
+ return ValidationResult.failed(
+ "Only a single ASCII character field delimiter is supported in native
text scan.")
+ }
super.doValidateInternal()
}
+ // The delimiter Spark's CSVOptions reads with: `sep` wins over the legacy
`delimiter` alias,
+ // and escapes such as `\t` are decoded. None when it cannot be decoded.
+ private def csvFieldDelimiter: Option[String] =
+ Try(
+ CSVExprUtils.toDelimiterStr(
+ relation.options.getOrElse("sep",
relation.options.getOrElse("delimiter", ",")))).toOption
Review Comment:
`getOrElse` will return `null` if the key exists with a null value (it
treats “present with null” as present), which then turns into a failed
`Try(...)` and `None`, causing validation failure/fallback instead of applying
Spark’s default delimiter or the alias fallback. If null option values are
possible in practice (you already handle this for `header`), it would be safer
to treat null as “not set” here (e.g., by reading via
`relation.options.get("sep").flatMap(Option(_))` and only falling back when the
value is non-null).
##########
backends-velox/src/test/scala/org/apache/gluten/execution/FileSourceScanOptionsSuite.scala:
##########
@@ -0,0 +1,81 @@
+/*
+ * 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.spark.network.util.JavaUtils
+import org.apache.spark.sql.catalyst.expressions.AttributeReference
+import org.apache.spark.sql.catalyst.util.CaseInsensitiveMap
+import org.apache.spark.sql.execution.datasources.{HadoopFsRelation,
InMemoryFileIndex}
+import org.apache.spark.sql.execution.datasources.csv.CSVFileFormat
+import org.apache.spark.sql.types.{StringType, StructField, StructType}
+
+import org.apache.hadoop.fs.Path
+
+import java.nio.file.Files
+
+class FileSourceScanOptionsSuite extends VeloxWholeStageTransformerSuite {
+ override protected val resourcePath: String = ""
+ override protected val fileFormat: String = "csv"
+
+ private lazy val dir = Files.createTempDirectory("csv-options").toFile
+
+ override def afterAll(): Unit = {
+ try JavaUtils.deleteRecursively(dir)
+ finally super.afterAll()
+ }
+
+ private def scan(options: Map[String, String]):
FileSourceScanExecTransformer = {
+ val schema = StructType(Seq(StructField("a", StringType, nullable = true)))
+ val relation = HadoopFsRelation(
+ new InMemoryFileIndex(spark, Seq(new Path(dir.toURI)), Map.empty, None),
+ StructType(Nil),
+ schema,
+ None,
+ new CSVFileFormat(),
+ CaseInsensitiveMap(options))(spark)
+ val attr = AttributeReference("a", StringType)()
+ FileSourceScanExecTransformer(relation, None, Seq(attr), schema, Nil,
None, None, Nil, None)
+ }
+
+ private def scanProperties(options: Map[String, String]): Map[String,
String] =
+ scan(options).getProperties
+
+ test("csv sep option maps to the native field delimiter") {
+ assert(scanProperties(Map("sep" -> "\t"))("field_delimiter") == "\t")
+ // Spark decodes an escaped delimiter, so a backslash followed by t is a
tab.
+ assert(scanProperties(Map("sep" -> "\\t"))("field_delimiter") == "\t")
+ }
+
+ test("sep wins over delimiter when both are set, like Spark") {
+ assert(scanProperties(Map("sep" -> "\t", "delimiter" ->
";"))("field_delimiter") == "\t")
+ }
+
Review Comment:
The new logic also supports the legacy `delimiter` alias when `sep` is
absent, and applies a default delimiter (`,`) when neither is provided. Adding
assertions for (1) `Map("delimiter" -> ";")` mapping to `field_delimiter=";"`
and (2) empty options mapping to `field_delimiter=","` would cover the
remaining branches of the newly introduced precedence/default behavior.
##########
gluten-substrait/src/main/scala/org/apache/gluten/execution/FileSourceScanExecTransformer.scala:
##########
@@ -188,9 +191,24 @@ abstract class FileSourceScanExecTransformerBase(
s"Unsupported matching schema column names " +
s"by field ids in native scan.")
}
+ if (fileFormat == ReadFileFormat.TextReadFormat &&
!csvFieldDelimiter.exists(isSingleAscii)) {
+ return ValidationResult.failed(
+ "Only a single ASCII character field delimiter is supported in native
text scan.")
+ }
super.doValidateInternal()
}
+ // The delimiter Spark's CSVOptions reads with: `sep` wins over the legacy
`delimiter` alias,
+ // and escapes such as `\t` are decoded. None when it cannot be decoded.
+ private def csvFieldDelimiter: Option[String] =
+ Try(
+ CSVExprUtils.toDelimiterStr(
+ relation.options.getOrElse("sep",
relation.options.getOrElse("delimiter", ",")))).toOption
Review Comment:
`CSVExprUtils` is an internal Spark catalyst API and may not be
stable/available across all Spark versions Gluten builds against. To reduce
cross-version risk, consider either (a) using a more stable Spark-facing
utility (if available in your supported versions), or (b) implementing a
minimal local delimiter unescaping/validation logic (e.g., handling common
escapes like `\t`, `\r`, `\n`, `\\`) so the code doesn’t depend on catalyst
internals.
--
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]
