LuciferYang commented on code in PR #13204:
URL: https://github.com/apache/gluten/pull/13204#discussion_r4186345388
##########
gluten-substrait/src/main/scala/org/apache/gluten/execution/FileSourceScanExecTransformer.scala:
##########
@@ -188,9 +191,34 @@ abstract class FileSourceScanExecTransformerBase(
s"Unsupported matching schema column names " +
s"by field ids in native scan.")
}
+ if (fileFormat == ReadFileFormat.TextReadFormat) {
+ csvFieldDelimiter match {
+ case Some(d) if isSingleAscii(d) =>
+ case Some(d) =>
+ return ValidationResult.failed(
+ "Only a single ASCII character field delimiter is supported in
native text scan, " +
+ s"got '$d'.")
+ case None =>
+ return ValidationResult.failed(
+ s"Cannot decode the CSV field delimiter '$rawCsvFieldDelimiter'.")
+ }
+ }
Review Comment:
Every other check in this `doValidateInternal` returns early with
`ValidationResult.failed(...)`; the new one follows the same shape so the
method reads the same top to bottom.
##########
gluten-substrait/src/main/scala/org/apache/gluten/execution/FileSourceScanExecTransformer.scala:
##########
@@ -188,9 +191,34 @@ abstract class FileSourceScanExecTransformerBase(
s"Unsupported matching schema column names " +
s"by field ids in native scan.")
}
+ if (fileFormat == ReadFileFormat.TextReadFormat) {
+ csvFieldDelimiter match {
+ case Some(d) if isSingleAscii(d) =>
+ case Some(d) =>
+ return ValidationResult.failed(
+ "Only a single ASCII character field delimiter is supported in
native text scan, " +
+ s"got '$d'.")
+ case None =>
+ return ValidationResult.failed(
+ s"Cannot decode the CSV field delimiter '$rawCsvFieldDelimiter'.")
+ }
+ }
super.doValidateInternal()
}
+ // Spark's CSVOptions takes `sep` over the legacy `delimiter` alias.
+ private def rawCsvFieldDelimiter: String =
+ relation.options.getOrElse("sep", relation.options.getOrElse("delimiter",
","))
+
+ // The delimiter after decoding escapes such as `\t`, as CSVOptions does.
None when it cannot be
+ // decoded, for which Spark raises an error.
+ private def csvFieldDelimiter: Option[String] =
Review Comment:
It runs on the driver a few times per scan node and decodes a one- or
two-character string, so caching would not buy anything measurable. Keeping it
a `def` avoids adding state to the case class.
--
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]
