Skip to content

[SPARK-39469][SQL] Infer date type for CSV schema inference #36871

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Jonathancui123 wants to merge 21 commits into from
Closed

[SPARK-39469][SQL] Infer date type for CSV schema inference #36871

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
5048253
infer date type in csv
Jonathancui123 Jun 15, 2022
5058d92
fix
Jonathancui123 Jun 15, 2022
f16e5e1
parse dates from timestamp column
Jonathancui123 Jun 15, 2022
c2b5fdc
cleanup
Jonathancui123 Jun 15, 2022
9514c2c
test date and timestamp same format
Jonathancui123 Jun 15, 2022
be7aabd
e2e test WIP
Jonathancui123 Jun 16, 2022
db3b442
e2e test data
Jonathancui123 Jun 16, 2022
7d98686
fix e2e date test
Jonathancui123 Jun 17, 2022
966bdb6
added inferDate flag
Jonathancui123 Jun 21, 2022
638064b
Updated documentation and fixed tests
Jonathancui123 Jun 22, 2022
50a91a6
added new error for inferDate with legacy parser and fixed feedback
Jonathancui123 Jun 22, 2022
d71558d
Update CSVInferSchema.scala
Jonathancui123 Jun 23, 2022
601dfc8
Update CSVOptions.scala
Jonathancui123 Jun 23, 2022
5aa4ab6
change inferDate error type and improve docs
Jonathancui123 Jun 23, 2022
2282c59
fix error class
Jonathancui123 Jun 23, 2022
2484b77
fix sqlState code and typo
Jonathancui123 Jun 24, 2022
762e0d8
Update CSVInferSchema.scala
Jonathancui123 Jun 24, 2022
2c93af5
updated docs and test parsing dates in TimestampNTZ column
Jonathancui123 Jun 28, 2022
41fa8eb
Fix case where dateFormat is not specified
Jonathancui123 Jun 30, 2022
e1170d0
allow legacy parser with inferDate
Jonathancui123 Jul 12, 2022
1e8f938
Revert "allow legacy parser with inferDate"
Jonathancui123 Jul 19, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Updated documentation and fixed tests
  • Loading branch information
@Jonathancui123
Jonathancui123 committed Jul 12, 2022
commit 638064bb847396c749b0cf81df33c2ce03d3af46
6 changes: 6 additions & 0 deletions docs/sql-data-sources-csv.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,12 @@ Data source options of CSV can be set via:
<td>Infers the input schema automatically from data. It requires one extra pass over the data. CSV built-in functions ignore this option.</td>
<td>read</td>
</tr>
<tr>
<td><code>inferDate</code></td>
<td>false</td>
<td>Whether or not to infer columns that satisfy the <code>dateFormat</code> option as <code>Date</code>. Requires <code>inferSchema</code> to be <code>true</code>. Legacy date formats in Timestamp columns cannot be parsed with this option.</td>
<td>read</td>
</tr>
<tr>
<td><code>enforceSchema</code></td>
<td>true</td>
Expand Down
14 changes: 12 additions & 2 deletions sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/csv/CSVInferSchema.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,8 +141,18 @@ class CSVInferSchema(val options: CSVOptions) extends Serializable {
case _: DecimalType => tryParseDecimal(field)
case DoubleType => tryParseDouble(field)
case DateType => tryParseDateTime(field)
Copy link
Member

@gengliangwang gengliangwang Jun 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems changing the method tryParseDouble should be enough

Copy link
Contributor Author

@Jonathancui123 Jonathancui123 Jun 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this change is necessary:
Consider the column of a TimestampType followed by a DateType entry. We would expect this column to be inferred as a TimestampType column.
typeSoFar will be Timestamp when inferField is called on the second entry which is DateType. We need logic in inferField to try and parse DateType even when typeSoFar is Timestamp

case TimestampNTZType => tryParseTimestampNTZ(field)
case TimestampType => tryParseTimestamp(field)
case TimestampNTZType =>
if (options.inferDate) {
tryParseDateTime(field)
} else {
tryParseTimestampNTZ(field)
}
case TimestampType =>
if (options.inferDate) {
tryParseDateTime(field)
} else {
tryParseTimestamp(field)
}
case BooleanType => tryParseBoolean(field)
case StringType => StringType
case other: DataType =>
Expand Down
10 changes: 7 additions & 3 deletions sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/csv/UnivocityParserSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,12 @@ package org.apache.spark.sql.catalyst.csv

import java.math.BigDecimal
import java.text.{DecimalFormat, DecimalFormatSymbols}
import java.time.{ZoneId, ZoneOffset}
import java.time.{ZoneOffset}
import java.util.{Locale, TimeZone}

import org.apache.commons.lang3.time.FastDateFormat
import org.apache.spark.SparkFunSuite

import org.apache.spark.SparkFunSuite
import org.apache.spark.sql.catalyst.InternalRow
import org.apache.spark.sql.catalyst.plans.SQLHelper
import org.apache.spark.sql.catalyst.util.DateTimeConstants._
Expand Down Expand Up @@ -367,11 +367,15 @@ class UnivocityParserSuite extends SparkFunSuite with SQLHelper {
"timestampNTZFormat" -> "dd-MM-yyyy HH:mm", "dateFormat" -> "dd_MM_yyyy"),
Copy link
Member

@HyukjinKwon HyukjinKwon Jun 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One test we might need would be:

timestampFormat" -> "dd/MM/yyyy HH:mm and dateFormat -> dd/MM/yyyy to make sure timestamps are not parsed as date types without conflicting.

Copy link
Contributor

@bersprockets bersprockets Jun 16, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to make sure timestamps are not parsed as date types without conflicting.

That's actually what happens:

Before this PR:

scala> val csvInput = Seq("0,2012-01-01 12:00:00", "1,2021-07-01 15:00:00").toDS()
csvInput: org.apache.spark.sql.Dataset[String] = [value: string]

scala> val df = spark.read.option("inferSchema", "true").csv(csvInput)
df: org.apache.spark.sql.DataFrame = [_c0: int, _c1: timestamp]

scala> df.printSchema
root
 |-- _c0: integer (nullable = true)
 |-- _c1: timestamp (nullable = true)

scala>

After this PR:

scala> val csvInput = Seq("0,2012-01-01 12:00:00", "1,2021-07-01 15:00:00").toDS()
csvInput: org.apache.spark.sql.Dataset[String] = [value: string]

scala> val df = spark.read.option("inferSchema", "true").csv(csvInput)
df: org.apache.spark.sql.DataFrame = [_c0: int, _c1: date]

scala> df.printSchema
root
 |-- _c0: integer (nullable = true)
 |-- _c1: date (nullable = true)

scala>

It looks like some tests fail too, like CSVInferSchemaSuite, and CSVv1Suite, possibly others (I ran these two suites on my laptop. For some reason, the github actions didn't run tests for this PR. Maybe @Jonathancui123 needs to turn them on in his fork?).

We should probably 1. add either SQL configuration or an option e.g., infersDate

I think you would need something like that: when set, the date formatter could use the slower, more strict method of parsing (so "2012-01-01 12:00:00" wouldn't parse as a date).

Edit: To do a strict parsing, one might need to use ParsePosition and check that the whole date/time value string was consumed. Even after setting lenient=false SimpleDateFormat.parse didn't complain about extra characters that weren't consumed.

Copy link
Contributor Author

@Jonathancui123 Jonathancui123 Jun 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I addressed inference mistakes in the following code snippet and comment

false, "UTC")
val dateString = "08_09_2001"
val expected = date(2001, 9, 8, 0, 0, 0, 0, ZoneOffset.UTC)
val expected = dataType match {
case TimestampType | TimestampNTZType => date(2001, 9, 8, 0, 0, 0, 0, ZoneOffset.UTC)
case DateType => days(2001, 9, 8)
}
val parser = new UnivocityParser(new StructType(), timestampsOptions)
assert(parser.makeConverter("d", dataType).apply(dateString) == expected)
}
checkDate(TimestampType)
checkDate(TimestampNTZType)
checkDate(DateType)
}
}