[MINOR][SQL][DOCS][2.4] Fix the timestamp pattern in the example for `to_timestamp` #27438

MaxGekk · 2020-02-02T20:15:32Z

What changes were proposed in this pull request?

In the PR, I propose to change the description of the to_timestamp() function, and change the pattern in the example.

Why are the changes needed?

To inform users about valid patterns for to_timestamp function.

Does this PR introduce any user-facing change?

No

How was this patch tested?

N/A

MaxGekk · 2020-02-02T20:15:57Z

@dongjoon-hyun Please, take a look at this.

SparkQA · 2020-02-02T23:23:01Z

Test build #117752 has finished for PR 27438 at commit f6a7060.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

dongjoon-hyun · 2020-02-03T00:03:20Z

It's too late for RC2, @MaxGekk . Let's consider this PR after RC2 vote.

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/datetimeExpressions.scala

sql/core/src/main/scala/org/apache/spark/sql/functions.scala

dongjoon-hyun · 2020-02-03T06:40:57Z

In general, this seems to be a follow-up of SPARK-23792 (at 2.4.0). Since it's too old JIRA, we had better use another JIRA or use [MINOR] like you. I support your choice. I believe that one thing we need is that yyyy-MM-dd HH:mm:ss.SSSS -> yyyy-MM-dd HH:mm:ss change. I recommend to focus on the above change only.

sql/core/src/main/scala/org/apache/spark/sql/functions.scala

dongjoon-hyun · 2020-02-03T07:01:28Z

@MaxGekk . Every committer's suggestion is his/her own criteria for his/her acceptance. I am also able to merge only what I can agree. And, as you know, the other committers also have different opinion and they will merge this if they agree with AS-IS status more. It always does. In addition, I don't complain about the other committer's decision when I understand it's on the edge. Since this PR is yours, it's up to you always~ @MaxGekk .

dongjoon-hyun · 2020-02-03T07:03:04Z

Anyway, thank you always for you active contribution. Apache Spark community really needs that. 😄

sql/core/src/main/scala/org/apache/spark/sql/functions.scala

HyukjinKwon

LGTM. Since @dongjoon-hyun is preparing the release, I will leave it to him though.

SparkQA · 2020-02-03T11:29:11Z

Test build #117767 has finished for PR 27438 at commit c7f75a9.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

cloud-fan · 2020-02-03T11:52:56Z

sql/core/src/main/scala/org/apache/spark/sql/functions.scala

   * See [[java.text.SimpleDateFormat]] for valid date and time format patterns
   *
   * @param s   A date, timestamp or string. If a string, the data must be in a format that can be
-   *            cast to a timestamp, such as `yyyy-MM-dd` or `yyyy-MM-dd HH:mm:ss.SSSS`


what's wrong with ".SSSS"?

The result is incorrect, look at the PR which fixes that [SPARK-29904][SQL][2.4] Parse timestamps in microsecond precision by JSON/CSV datasources #26507, and here is an example:

val sdf = new java.text.SimpleDateFormat("yyyy-MM-dd HH:mm:ss.SSSS") val res = sdf.parse("1970-01-01 00:00:00.1234") println(sdf.format(res)) 1970-01-01 00:00:01.0234

And the result of to_timestamp is truncated to seconds.

for 1), is it fixed in 2.4 or not?

for 2), have we documented it?

SPARK-29904 is merged at 2.4.5. In the worst case, we can revert @MaxGekk 's #26507 because of this.

for 1), is it fixed in 2.4 or not?

@cloud-fan It is fixed in JSON and CSV datasources only but not for to_timestamp() and other functions.

for 2), have we documented it?

I have tried to document it here in the PR

In the worst case, we can revert @MaxGekk 's #26507 because of this.

@dongjoon-hyun Why? What's the worst case? I do think we should apply the fix in other places but not reverting it.

@cloud-fan I tried to document the restriction of to_timestmp() in the PR but @dongjoon-hyun convinced me to do not do that here, see #27438 (comment) " I believe that one thing we need is that yyyy-MM-dd HH:mm:ss.SSSS -> yyyy-MM-dd HH:mm:ss change. I recommend to focus on the above change only."

@MaxGekk . To be clear, I want to make it sure we don't have any regression at 2.4.5 as a release manager. You raised this issue and we are investigating the relevant PRs. That's all. We are considering the scope and effect. So far, we didn't make any decision.

MaxGekk · 2020-02-03T18:41:32Z

sql/core/src/main/scala/org/apache/spark/sql/functions.scala

  /**
   * Converts time string with the given pattern to timestamp.
   *
   * See [[java.text.SimpleDateFormat]] for valid date and time format patterns


If you don't mind, I would like to say here that currently supported pattern for seconds fractions is SSS only. The second s fractions can be parsed but will be ignored while casting to TimestampType.

dongjoon-hyun

+1, LGTM (AS-IS). Thanks, @MaxGekk and @HyukjinKwon .
Let's postpone this PR during RC2 vote period.

cloud-fan · 2020-02-04T03:24:57Z

maybe we can just fix the issue in to_timestamp after RC2 passes?

MaxGekk · 2020-02-04T10:15:29Z

maybe we can just fix the issue in to_timestamp after RC2 passes?

@cloud-fan I have tried to port all functions on TimestampParser by rewriting DateTimeUtils.newDateFormat:

  def newDateFormat(formatString: String, timeZone: TimeZone): TimestampParser = {
    new TimestampParser(FastDateFormat.getInstance(formatString, timeZone))
  }

from

  def newDateFormat(formatString: String, timeZone: TimeZone): DateFormat = {
    val sdf = new SimpleDateFormat(formatString, Locale.US)
    sdf.setTimeZone(timeZone)
    // Enable strict parsing, if the input date/format is invalid, it will throw an exception.
    // e.g. to parse invalid date '2016-13-12', or '2016-01-12' with  invalid format 'yyyy-aa-dd',
    // an exception will be throwed.
    sdf.setLenient(false)
    sdf
  }

but I got a few test failures due to strict mode set via sdf.setLenient(false). I haven't found a way to set similar mode for FastDateFormat.

cloud-fan · 2020-02-04T10:16:49Z

OK then this doc change LGTM

dongjoon-hyun · 2020-02-04T21:01:02Z

Thank you for the investigation, @MaxGekk . And, thank you for the conclusion, @cloud-fan .

MaxGekk · 2020-02-06T21:37:52Z

@dongjoon-hyun Kindly remind you about this minor fix.

dongjoon-hyun · 2020-02-06T21:39:16Z

Sure, @MaxGekk . Sorry for making you wait. Merged to branch-2.4.

…`to_timestamp` ### What changes were proposed in this pull request? In the PR, I propose to change the description of the `to_timestamp()` function, and change the pattern in the example. ### Why are the changes needed? To inform users about valid patterns for `to_timestamp` function. ### Does this PR introduce any user-facing change? No ### How was this patch tested? N/A Closes #27438 from MaxGekk/to_timestamp-z-2.4. Authored-by: Maxim Gekk <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]>

…`to_timestamp` ### What changes were proposed in this pull request? In the PR, I propose to change the description of the `to_timestamp()` function, and change the pattern in the example. ### Why are the changes needed? To inform users about valid patterns for `to_timestamp` function. ### Does this PR introduce any user-facing change? No ### How was this patch tested? N/A Closes apache#27438 from MaxGekk/to_timestamp-z-2.4. Authored-by: Maxim Gekk <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]>

Fix to_timestamp description

f6a7060

dongjoon-hyun reviewed Feb 3, 2020

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/datetimeExpressions.scala Outdated Show resolved Hide resolved

dongjoon-hyun reviewed Feb 3, 2020

View reviewed changes

sql/core/src/main/scala/org/apache/spark/sql/functions.scala Outdated Show resolved Hide resolved

MaxGekk commented Feb 3, 2020

View reviewed changes

sql/core/src/main/scala/org/apache/spark/sql/functions.scala Outdated Show resolved Hide resolved

HyukjinKwon reviewed Feb 3, 2020

View reviewed changes

sql/core/src/main/scala/org/apache/spark/sql/functions.scala Outdated Show resolved Hide resolved

Remove seconds precision

c7f75a9

MaxGekk changed the title ~~[MINOR][SQL][DOCS][2.4] Fix the descriptions of to_timestamp and ParseToTimestamp~~ [MINOR][SQL][DOCS][2.4] Fix the timestamp pattern in the example for to_timestamp Feb 3, 2020

HyukjinKwon approved these changes Feb 3, 2020

View reviewed changes

cloud-fan reviewed Feb 3, 2020

View reviewed changes

MaxGekk commented Feb 3, 2020

View reviewed changes

dongjoon-hyun approved these changes Feb 3, 2020

View reviewed changes

dongjoon-hyun added SQL DOCUMENTATION labels Feb 5, 2020

dongjoon-hyun closed this Feb 6, 2020

MaxGekk deleted the to_timestamp-z-2.4 branch June 5, 2020 19:43

[MINOR][SQL][DOCS][2.4] Fix the timestamp pattern in the example for to_timestamp #27438

[MINOR][SQL][DOCS][2.4] Fix the timestamp pattern in the example for to_timestamp #27438

Uh oh!

Conversation

MaxGekk commented Feb 2, 2020 • 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!

MaxGekk commented Feb 2, 2020

Uh oh!

SparkQA commented Feb 2, 2020

Uh oh!

dongjoon-hyun commented Feb 3, 2020

Uh oh!

Uh oh!

Uh oh!

dongjoon-hyun commented Feb 3, 2020

Uh oh!

Uh oh!

dongjoon-hyun commented Feb 3, 2020

Uh oh!

dongjoon-hyun commented Feb 3, 2020

Uh oh!

Uh oh!

HyukjinKwon left a comment

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Feb 3, 2020

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!

MaxGekk Feb 3, 2020 • 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!

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!

dongjoon-hyun left a comment

Choose a reason for hiding this comment

Uh oh!

cloud-fan commented Feb 4, 2020

Uh oh!

MaxGekk commented Feb 4, 2020

Uh oh!

cloud-fan commented Feb 4, 2020

Uh oh!

dongjoon-hyun commented Feb 4, 2020

Uh oh!

MaxGekk commented Feb 6, 2020

Uh oh!

dongjoon-hyun commented Feb 6, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

[MINOR][SQL][DOCS][2.4] Fix the timestamp pattern in the example for `to_timestamp` #27438

[MINOR][SQL][DOCS][2.4] Fix the timestamp pattern in the example for `to_timestamp` #27438

MaxGekk commented Feb 2, 2020 •

edited

Loading

MaxGekk Feb 3, 2020 •

edited

Loading