[SPARK-39904][SQL] Rename inferDate to prefersDate and clarify semantics of the option in CSV data source #37327

sadikovi · 2022-07-28T05:40:27Z

What changes were proposed in this pull request?

This is a follow-up for #36871.

PR renames inferDate to prefersDate to avoid confusion when dates inference would change the column type and result in confusion when the user meant to infer timestamps.

The patch also updates semantics of the option: prefersDate is allowed to be used during schema inference (inferSchema) as well as user-provided schema where it could be used as a fallback mechanism when parsing timestamps.

Why are the changes needed?

Fixes ambiguity when setting prefersDate to true and clarifies semantics of the option.

Does this PR introduce any user-facing change?

Although it is an option rename, the original PR was merged a few days ago and the config option has not been included in a Spark release.

How was this patch tested?

I added a unit test for prefersDate = true with a user schema.

sadikovi · 2022-07-28T05:40:55Z

cc @cloud-fan @HyukjinKwon @Jonathancui123

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/csv/CSVOptions.scala

Jonathancui123

Thanks for pointing out the gap! @sadikovi Based on the above I think we should:

Still name the option inferDate because the primary impact of the option is that columns with dates will be correctly inferred
Make the change to throw an error when inferSchema is false but inferDate is true. This will clarify that the primary purpose of the inferDate flag is for allowing dates in the inferred schema.

docs/sql-data-sources-csv.md

sadikovi · 2022-07-29T04:02:34Z

Should we keep requirement that inferDate = true needs inferSchema = true? I think we should clarify semantics.

Jonathancui123 · 2022-07-29T05:29:29Z

Should we keep requirement that inferDate = true needs inferSchema = true? I think we should clarify semantics.

@sadikovi I think we should keep the requirement and the new exception type in this PR. The exception will clarify that the primary purpose of the inferDate flag is for allowing dates in the inferred schema. The requirement that inferDate = true needs inferSchema = true makes sense. Without such a requirement, inferDate=true with inferSchema = false is modifying parsing fallback behavior for no reason.

sadikovi · 2022-07-29T05:31:55Z

Yes, that was my thinking too. Okay, I will make a few changes to the PR.

cloud-fan · 2022-07-29T11:54:20Z

The problem is, at the reading phase, we don't know if the schema is inferred or user specified. Even if inferSchema=true, people can still specify schema via spark.read.schema(...), right?

AmplabJenkins · 2022-07-29T16:23:56Z

Can one of the admins verify this patch?

docs/sql-data-sources-csv.md

sadikovi · 2022-08-01T02:14:36Z

@Jonathancui123 @cloud-fan @HyukjinKwon I have renamed the option and clarified the option description. Can you review again? Thanks.

cloud-fan · 2022-08-01T05:28:12Z

docs/sql-data-sources-csv.md

    <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>. When <code>false</code>, columns with dates will be inferred as <code>String</code> (or as <code>Timestamp</code> if it fits the <code>timestampFormat</code>).</td>
+    <td>Attempts to infer string columns that contain dates or timestamps as <code>Date</code> if the values satisfy <code>dateFormat</code> option and failed to be parsed by the respective formatter during schema inference (<code>inferSchema</code>). When used in conjunction with a user-provided schema, attempts parse timestamp columns as dates using <code>dateFormat</code> if they fail to conform to <code>timestampFormat</code>, the parsed values will be cast to timestamp type afterwards.</td>


contain dates or timestamps reads a bit confusing. How about just ... infer string columns as Date ...

How to understand failed to be parsed by the respective formatter?

Let me update.

cloud-fan · 2022-08-01T05:29:22Z

docs/sql-data-sources-csv.md

    <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>. When <code>false</code>, columns with dates will be inferred as <code>String</code> (or as <code>Timestamp</code> if it fits the <code>timestampFormat</code>).</td>
+    <td>Attempts to infer string columns that contain dates or timestamps as <code>Date</code> if the values satisfy <code>dateFormat</code> option and failed to be parsed by the respective formatter during schema inference (<code>inferSchema</code>). When used in conjunction with a user-provided schema, attempts parse timestamp columns as dates using <code>dateFormat</code> if they fail to conform to <code>timestampFormat</code>, the parsed values will be cast to timestamp type afterwards.</td>


Suggested change

<td>Attempts to infer string columns that contain dates or timestamps as <code>Date</code> if the values satisfy <code>dateFormat</code> option and failed to be parsed by the respective formatter during schema inference (<code>inferSchema</code>). When used in conjunction with a user-provided schema, attempts parse timestamp columns as dates using <code>dateFormat</code> if they fail to conform to <code>timestampFormat</code>, the parsed values will be cast to timestamp type afterwards.</td>

<td>Attempts to infer string columns that contain dates or timestamps as <code>Date</code> if the values satisfy <code>dateFormat</code> option and failed to be parsed by the respective formatter during schema inference (<code>inferSchema</code>). When used in conjunction with a user-provided schema, attempts to parse timestamp columns as dates using <code>dateFormat</code> if they fail to conform to <code>timestampFormat</code>, the parsed values will be cast to timestamp type afterwards.</td>

Updated, thanks.

Jonathancui123 · 2022-08-02T19:10:52Z

docs/sql-data-sources-csv.md

    <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>. When <code>false</code>, columns with dates will be inferred as <code>String</code> (or as <code>Timestamp</code> if it fits the <code>timestampFormat</code>).</td>
+    <td>Attempts to infer string columns as <code>Date</code> if the values satisfy <code>dateFormat</code> option and failed to be parsed by the respective formatter during schema inference (<code>inferSchema</code>). When used in conjunction with a user-provided schema, attempts to parse timestamp columns as dates using <code>dateFormat</code> if they fail to conform to <code>timestampFormat</code>, the parsed values will be cast to timestamp type afterwards.</td>


Suggested change

<td>Attempts to infer string columns as <code>Date</code> if the values satisfy <code>dateFormat</code> option and failed to be parsed by the respective formatter during schema inference (<code>inferSchema</code>). When used in conjunction with a user-provided schema, attempts to parse timestamp columns as dates using <code>dateFormat</code> if they fail to conform to <code>timestampFormat</code>, the parsed values will be cast to timestamp type afterwards.</td>

<td>During schema inference (<code>inferSchema</code>), attempts to infer string columns that contain dates or timestamps as <code>Date</code> if the values satisfy the <code>dateFormat</code> option and failed to be parsed by the respective formatter. Attempts to parse timestamp columns as dates using <code>dateFormat</code> if they fail to conform to <code>timestampFormat</code>. The parsed values will be cast to timestamp type afterwards.</td>

Re-order sentence for readability

Removed: "When used in conjunction with a user-provided schema" because parsing timestamp columns as dates happens regardless of whether there is a user-provided schema

Split sentence at comma for readabilityWhen used in conjunction with a user-provided schema

I would still prefer to keep the change for user-provided schema. There is a different behaviour when schema inference or providing one. During the former, string columns would be converted to dates but in the latter they would be kept as timestamp columns.

Let me update and we can discuss further.

I updated the text, essentially kept your suggestions and just inserted "With a user-provided schema". Can you take another look? Thanks.

I see, that makes sense. In my opinion, it is more confusing for the reader. But LGTM

Yes, thanks. I just wanted to clarify the behaviour around user-provided schema as opposed to schema inference to remove any ambiguity that would arise from users.

HyukjinKwon · 2022-08-03T04:07:49Z

Merged to master.

…TimestampNTZ ### What changes were proposed in this pull request? Similar with #37327, this PR renames the JDBC data source option `inferTimestampNTZType` as `preferTimestampNTZ` ### Why are the changes needed? It is simpler and more straightforward. Also, it is consistent with the CSV data source option introduced in #37327, ### Does this PR introduce _any_ user-facing change? No, the TimestampNTZ project is not released yet. ### How was this patch tested? UT Closes #40042 from gengliangwang/inferNTZOption. Authored-by: Gengliang Wang <gengliang@apache.org> Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>

…TimestampNTZ Similar with #37327, this PR renames the JDBC data source option `inferTimestampNTZType` as `preferTimestampNTZ` It is simpler and more straightforward. Also, it is consistent with the CSV data source option introduced in #37327, No, the TimestampNTZ project is not released yet. UT Closes #40042 from gengliangwang/inferNTZOption. Authored-by: Gengliang Wang <gengliang@apache.org> Signed-off-by: Hyukjin Kwon <gurwls223@apache.org> (cherry picked from commit 8194522) Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>

…TimestampNTZ Similar with apache#37327, this PR renames the JDBC data source option `inferTimestampNTZType` as `preferTimestampNTZ` It is simpler and more straightforward. Also, it is consistent with the CSV data source option introduced in apache#37327, No, the TimestampNTZ project is not released yet. UT Closes apache#40042 from gengliangwang/inferNTZOption. Authored-by: Gengliang Wang <gengliang@apache.org> Signed-off-by: Hyukjin Kwon <gurwls223@apache.org> (cherry picked from commit 8194522) Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>

rename the option

cb4147c

github-actions bot added CORE DOCS SQL labels Jul 28, 2022

HyukjinKwon reviewed Jul 28, 2022

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/csv/CSVOptions.scala Outdated Show resolved Hide resolved

HyukjinKwon changed the title ~~[SPARK-39904] Rename inferDate to preferDate and add check for inferSchema = false~~ [SPARK-39904][SQL] Rename inferDate to preferDate and add check for inferSchema = false Jul 28, 2022

Jonathancui123 reviewed Jul 28, 2022

View reviewed changes

docs/sql-data-sources-csv.md Outdated Show resolved Hide resolved

sadikovi added 2 commits August 1, 2022 11:42

revert rename

a865018

update documentation and test

b475d51

HyukjinKwon reviewed Aug 1, 2022

View reviewed changes

docs/sql-data-sources-csv.md Outdated Show resolved Hide resolved

sadikovi changed the title ~~[SPARK-39904][SQL] Rename inferDate to preferDate and add check for inferSchema = false~~ [SPARK-39904][SQL] Rename inferDate to prefersDate and clarify semantics of the option in CSV data source Aug 1, 2022

rename inferDate to prefersDate

6e75a73

HyukjinKwon approved these changes Aug 1, 2022

View reviewed changes

cloud-fan reviewed Aug 1, 2022

View reviewed changes

fix scalastyle

71a095f

cloud-fan reviewed Aug 1, 2022

View reviewed changes

cloud-fan approved these changes Aug 1, 2022

View reviewed changes

update documentation

5b01e20

sadikovi requested a review from Jonathancui123 August 2, 2022 01:55

Jonathancui123 approved these changes Aug 2, 2022

View reviewed changes

update documentation

ef445be

HyukjinKwon closed this in 31ab8bc Aug 3, 2022

HyukjinKwon mentioned this pull request Aug 11, 2022

[SPARK-34953][CORE][SQL] Add the code change for adding the DateType in the infer schema while reading in CSV and JSON #32558

Closed

gengliangwang mentioned this pull request Feb 15, 2023

[SPARK-42455][SQL] Rename JDBC option inferTimestampNTZType as preferTimestampNTZ #40042

Closed

[SPARK-39904][SQL] Rename inferDate to prefersDate and clarify semantics of the option in CSV data source #37327

[SPARK-39904][SQL] Rename inferDate to prefersDate and clarify semantics of the option in CSV data source #37327

Uh oh!

Conversation

sadikovi commented Jul 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

sadikovi commented Jul 28, 2022

Uh oh!

Uh oh!

Jonathancui123 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sadikovi commented Jul 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Jonathancui123 commented Jul 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sadikovi commented Jul 29, 2022

Uh oh!

cloud-fan commented Jul 29, 2022

Uh oh!

AmplabJenkins commented Jul 29, 2022

Uh oh!

Uh oh!

sadikovi commented Aug 1, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HyukjinKwon commented Aug 3, 2022

Uh oh!

Uh oh!

sadikovi commented Jul 28, 2022 •

edited

Loading

Jonathancui123 left a comment •

edited

Loading

sadikovi commented Jul 29, 2022 •

edited

Loading

Jonathancui123 commented Jul 29, 2022 •

edited

Loading