LuciferYang commented on code in PR #13204:
URL: https://github.com/apache/gluten/pull/13204#discussion_r4186043296
##########
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:
Same answer as above: `CSVExprUtils.toDelimiterStr` exists with the same
signature in every Spark version Gluten builds against (3.4 to 4.1), and it is
the function `CSVOptions` itself calls, so the decoding stays identical to
vanilla Spark. A shim would only wrap the same call four times.
##########
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:
Same answer as above: `CSVExprUtils.toDelimiterStr` exists with the same
signature in every Spark version Gluten builds against (3.4 to 4.1), and it is
the function `CSVOptions` itself calls, so the decoding stays identical to
vanilla Spark. A shim would only wrap the same call four times.
##########
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:
Done in 7a15ea634. The reason now says either that the delimiter cannot be
decoded, with the configured value, or that it is not a single ASCII character,
with the decoded value.
##########
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:
Added in 7a15ea634: `sep -> null` fails validation with the "Cannot decode"
reason.
##########
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:
Treating null as unset would diverge from Spark: `CSVOptions` passes the
null straight to `toDelimiterStr`, which throws. Falling back keeps that
behavior. The new test pins it.
--
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]
