LuciferYang commented on code in PR #13204: URL: https://github.com/apache/gluten/pull/13204#discussion_r4185689865
########## 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: Added both in 677e05c63: `delimiter -> ";"` alone maps to `;`, and no option maps to `,`. ########## 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 a public object with the same `toDelimiterStr(String): String` in Spark 3.4.4, 3.5.5, 4.0.2 and 4.1.1, the versions Gluten builds against, and it is what Spark's own `CSVOptions` uses to compute the delimiter. Calling it keeps the decoding identical to vanilla, including the cases it rejects; a local copy would have to track any change to it. ########## 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: Spark does not treat a null value as unset here either: `CSVOptions` reads `parameters.getOrElse(SEP, parameters.getOrElse(DELIMITER, ","))` and passes the null to `toDelimiterStr`, which throws (an NPE on 3.x, `INVALID_DELIMITER_VALUE.NULL_VALUE` on 4.x). Falling back lets vanilla raise that same error, whereas defaulting to `,` would read the file where Spark fails. -- 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]
