srowen commented on a change in pull request #29516:
URL: https://github.com/apache/spark/pull/29516#discussion_r475158855
##########
File path:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/csv/CSVExprUtils.scala
##########
@@ -25,16 +25,21 @@ object CSVExprUtils {
* This is currently being used in CSV reading path and CSV schema inference.
*/
def filterCommentAndEmpty(iter: Iterator[String], options: CSVOptions):
Iterator[String] = {
- iter.filter { line =>
- line.trim.nonEmpty && !line.startsWith(options.comment.toString)
+ if (options.isCommentSet) {
+ val commentPrefix = options.comment.toString
+ iter.filter { line =>
+ line.trim.nonEmpty && !line.startsWith(commentPrefix)
+ }
+ } else {
+ iter.filter(_.trim.nonEmpty)
}
}
def skipComments(iter: Iterator[String], options: CSVOptions):
Iterator[String] = {
if (options.isCommentSet) {
val commentPrefix = options.comment.toString
iter.dropWhile { line =>
- line.trim.isEmpty || line.trim.startsWith(commentPrefix)
+ line.trim.isEmpty || line.startsWith(commentPrefix)
Review comment:
I think it's correct to _not_ trim the string that's checked to see if
it starts with a comment, which is a slightly separate issue. `\u0000` can't be
used as a comment char, but other non-printable chars _could_.
##########
File path:
sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/csv/CSVSuite.scala
##########
@@ -1902,25 +1902,26 @@ abstract class CSVSuite extends QueryTest with
SharedSparkSession with TestCsvDa
test("SPARK-25387: bad input should not cause NPE") {
val schema = StructType(StructField("a", IntegerType) :: Nil)
- val input = spark.createDataset(Seq("\u0000\u0000\u0001234"))
+ val input = spark.createDataset(Seq("\u0001\u0000\u0001234"))
Review comment:
I think this test was wrong in 2 ways. First it relied on, actually,
ignoring lines starting with `\u0000`, which is the very bug we're fixing. You
can see below it's asserting there is no result at all, when there should be
_some_ result.
##########
File path:
sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/csv/CSVSuite.scala
##########
@@ -1902,25 +1902,26 @@ abstract class CSVSuite extends QueryTest with
SharedSparkSession with TestCsvDa
test("SPARK-25387: bad input should not cause NPE") {
val schema = StructType(StructField("a", IntegerType) :: Nil)
- val input = spark.createDataset(Seq("\u0000\u0000\u0001234"))
+ val input = spark.createDataset(Seq("\u0001\u0000\u0001234"))
checkAnswer(spark.read.schema(schema).csv(input), Row(null))
checkAnswer(spark.read.option("multiLine",
true).schema(schema).csv(input), Row(null))
- assert(spark.read.csv(input).collect().toSet == Set(Row()))
+ assert(spark.read.schema(schema).csv(input).collect().toSet ==
Set(Row(null)))
}
test("SPARK-31261: bad csv input with `columnNameCorruptRecord` should not
cause NPE") {
val schema = StructType(
StructField("a", IntegerType) :: StructField("_corrupt_record",
StringType) :: Nil)
- val input = spark.createDataset(Seq("\u0000\u0000\u0001234"))
+ val input = spark.createDataset(Seq("\u0001\u0000\u0001234"))
checkAnswer(
spark.read
.option("columnNameOfCorruptRecord", "_corrupt_record")
.schema(schema)
.csv(input),
- Row(null, null))
- assert(spark.read.csv(input).collect().toSet == Set(Row()))
+ Row(null, "\u0001\u0000\u0001234"))
Review comment:
The other problem I think is that this was asserting there is no corrupt
record -- no result at all -- when I think clearly the test should result in a
single row with a corrupt record.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
