Copilot commented on code in PR #13204:
URL: https://github.com/apache/gluten/pull/13204#discussion_r4186199812
##########
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:
`return` inside the `match` makes control flow harder to follow and is
generally discouraged in Scala. Consider rewriting this block to produce a
`ValidationResult` expression (e.g., compute an `Option[ValidationResult]` for
the failure case and `getOrElse(super.doValidateInternal())`) so the method
remains expression-oriented without early returns.
##########
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:
`rawCsvFieldDelimiter`/`csvFieldDelimiter` are `def`s, so delimiter decoding
can run multiple times per instance (e.g., during validation and again when
building properties). Consider making the decoded delimiter a `lazy val` (or
otherwise caching it) to avoid repeated decoding work and keep behavior
consistent across call sites.
--
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]
