LuciferYang commented on code in PR #58226:
URL: https://github.com/apache/spark/pull/58226#discussion_r3997037813
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/csv/CSVOptions.scala:
##########
@@ -124,7 +124,7 @@ class CSVOptions(
val extension = {
val ext = parameters.getOrElse(EXTENSION, "csv")
- if (ext.size != 3 && !ext.forall(_.isLetter)) {
+ if (ext.isEmpty || !ext.forall(_.isLetter)) {
Review Comment:
The option has shipped since 4.0.0 and every branch through `branch-4.3`
carries the same `&&` line, so the empty string and every 3-character suffix
containing a non-letter (`ab1`, `mp3`) still write there and only start failing
in 4.4.0, and `docs/sql-migration-guide.md` says nothing about it.
The user-facing-change section covers writes only, but reads are affected
too: the check runs when `CSVOptions` is constructed, and a catalog table's
`OPTIONS` go straight into `CSVFileFormat.getCsvOptions`. So a csv table
created on an older release with `OPTIONS (extension 'ab1')` starts failing
`SELECT * FROM t`, which `ALTER TABLE ... SET SERDEPROPERTIES` can undo.
A bullet under "Upgrading from Spark SQL 4.3 to 4.4" covering both sides
would be enough. Read-side rejection is not new in itself, since `csv.gz`
already fails a read on master, so there is no need to make the val lazy.
##########
common/utils/src/main/resources/error/error-conditions.json:
##########
@@ -4846,7 +4846,7 @@
},
"EXTENSION" : {
"message" : [
- "Invalid extension: <invalidValue>. Extension is limited to exactly
3 letters (e.g. csv, tsv, etc...)"
+ "Invalid extension: <invalidValue>. Extension must be non-empty and
contain only letters."
Review Comment:
The `EXTENSION` message now reads "non-empty and contain only letters", but
`docs/sql-data-sources-csv.md:66` still says "Limited to letters. Length must
equal 3." Nothing under `docs/` is generated from `error-conditions.json`, and
no Python, R or Scala writer doc mentions the option, so that row is the only
description a user ever sees and it now contradicts the error.
That line is already wrong on master, which accepts `abcd` too, so this PR
did not break it. But this PR is where the rule gets settled, so fixing the row
here is the cheapest option. A separate comment asks for one more clause in the
same row.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/csv/CSVOptions.scala:
##########
@@ -124,7 +124,7 @@ class CSVOptions(
val extension = {
val ext = parameters.getOrElse(EXTENSION, "csv")
- if (ext.size != 3 && !ext.forall(_.isLetter)) {
+ if (ext.isEmpty || !ext.forall(_.isLetter)) {
Review Comment:
Letters-only does not block the values that actually break a round trip: on
read the decompression codec is chosen purely from the file-name suffix, while
the rationale in this thread stops at filename safety.
`deflate`, `gz`, `snappy`, `zst`, `zstd` and `gzip` are all letters, so both
the old and the new predicate let them through: `extension=gz` without
`compression` writes plain text under a `.gz` name, and reading it back
inflates it as gzip and throws an IOException during schema inference. The only
codec suffixes this change does reject, `bz2` and `lz4`, are the ones carrying
a digit, so that rejection is incidental. It is also the reason not to switch
to `isLetterOrDigit`, which would put those two back.
The trap is already on master and does not need solving here. Worth one
clause in the same `docs/sql-data-sources-csv.md:66` row the other comment
updates: a suffix that collides with a compression codec makes the output
unreadable unless `compression` is set too.
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/csv/CSVSuite.scala:
##########
@@ -3095,6 +3095,41 @@ abstract class CSVSuite
}
}
+ test("SPARK-58946: reject empty or non-letter file extensions") {
+ Seq("", "ab1", "a/b").foreach { ext =>
+ withTempPath { path =>
+ checkError(
+ exception = intercept[SparkIllegalArgumentException] {
+ spark.range(1).write.option("extension",
ext).csv(path.getAbsolutePath)
+ },
+ condition = "INVALID_PARAMETER_VALUE.EXTENSION",
+ parameters = Map(
+ "functionName" -> "`csv`",
+ "parameter" -> "`extension`",
+ "invalidValue" -> s"`$ext`"))
+ }
+ }
+ }
+
+ test("SPARK-58946: allow alphabetic file extensions of arbitrary length") {
Review Comment:
`allow alphabetic file extensions of arbitrary length` is almost a verbatim
copy of the SPARK-50616 test just below it (`CSVSuite.scala:3133`): same three
timestamp strings, same `repartition(1)`, same `Files.list` plus
`assert(files.size == 1)`. Fold `tsv` into the new `Seq`, name SPARK-50616 in
the test name and drop the old test: it asserts nothing the new one does not,
and that removes an assertion that would otherwise have to stay in sync.
The three timestamp strings are carried over from the timestamp test above
and have nothing to do with the suffix; any single row would do, as long as
`repartition(1)` stays, since `assert(files.size == 1)` rests on it. None of
this needs doing in this PR.
--
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]
