stevomitric commented on code in PR #58226:
URL: https://github.com/apache/spark/pull/58226#discussion_r3971117760
##########
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.size != 3 || !ext.forall(_.isLetter)) {
Review Comment:
To be precise about the constraint though, the only technical requirement is
that the value is filename-safe, since it's concatenated into the output name
and it's the `forall(_.isLetter)` clause, not the length check, that actually
blocks the dangerous a/b case.
IMO "Length must equal 3" adds no safety, it just rejects harmless suffixes
like .data. So I don't think .abcdef is technically wrong, only .a/b case and
an empty one.
Your proposed `ext.isEmpty || !ext.forall(_.isLetter)` variant looks like
the right call to me, i.e., keep letters-only and drop the length rule,
updating the doc + error message to "limited to letters.".
What do others think here? cc @uros-b
--
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]
