Copilot commented on code in PR #13204:
URL: https://github.com/apache/gluten/pull/13204#discussion_r4185863538
##########
gluten-substrait/src/main/scala/org/apache/gluten/execution/FileSourceScanExecTransformer.scala:
##########
@@ -24,6 +24,7 @@ import org.apache.gluten.utils.FileIndexUtil
import org.apache.spark.Partition
import org.apache.spark.sql.catalyst.TableIdentifier
+import org.apache.spark.sql.catalyst.csv.CSVExprUtils
Review Comment:
`org.apache.spark.sql.catalyst.csv.CSVExprUtils` is a Spark
catalyst-internal API and may not be stable/available across all Spark versions
Gluten supports. To reduce cross-version breakage risk, consider routing
delimiter decoding through an existing Gluten Spark-shim layer (if present) or
using a more version-stable parsing path (e.g., Spark’s `CSVOptions` via shims)
rather than directly importing catalyst internals.
##########
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()
}
Review Comment:
This failure reason is used both when the delimiter is
multi-character/non-ASCII and when the delimiter cannot be decoded (i.e.,
`csvFieldDelimiter` is `None`). It would be more actionable to include the raw
configured value (when present) and distinguish “cannot decode delimiter
escape” vs “decoded delimiter is not a single ASCII character”.
##########
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:
`relation.options.getOrElse("sep", ...)` can return `null` if the key is
present with a null value; this makes `toDelimiterStr` throw (caught by `Try`),
yielding `None` and causing validation to fail (fallback) rather than behaving
like “option not set” (defaulting to comma). Consider treating null as absent
before decoding so `sep=null` / `delimiter=null` uses the default `","` instead
of forcing fallback.
##########
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(
Review Comment:
`org.apache.spark.sql.catalyst.csv.CSVExprUtils` is a Spark
catalyst-internal API and may not be stable/available across all Spark versions
Gluten supports. To reduce cross-version breakage risk, consider routing
delimiter decoding through an existing Gluten Spark-shim layer (if present) or
using a more version-stable parsing path (e.g., Spark’s `CSVOptions` via shims)
rather than directly importing catalyst internals.
##########
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:
The new delimiter parsing path has an important edge case when `sep` or
`delimiter` is explicitly set to `null` (or otherwise undecodable). Adding a
unit test that asserts the intended behavior (default to comma vs fallback)
would prevent regressions and clarify the contract for null/invalid delimiter
inputs.
--
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]
