Skip to content

Conversation

@yaooqinn
Copy link
Member

@yaooqinn yaooqinn commented Jun 3, 2020

What changes were proposed in this pull request?

This PR set the hour to 12/0 when the AMPM_OF_DAY field exists

Why are the changes needed?

When the hour is absent but the am-pm is present, the time is incorrect for pm

Does this PR introduce any user-facing change?

yes, the change is user-facing but to change back to 2.4 to keep backward compatibility

e.g.

spark-sql> select to_timestamp('33:33 PM', 'mm:ss a');
1970-01-01 12:33:33
spark-sql> select to_timestamp('33:33 AM', 'mm:ss a');
1970-01-01 00:33:33

otherwise, the results are all 1970-01-01 00:33:33

How was this patch tested?

add unit tests

@yaooqinn
Copy link
Member Author

yaooqinn commented Jun 3, 2020

cc @cloud-fan @maropu @MaxGekk thanks

@SparkQA
Copy link

SparkQA commented Jun 3, 2020

Test build #123460 has finished for PR 28713 at commit 81eb751.

  • This patch fails due to an unknown error code, -9.
  • This patch merges cleanly.
  • This patch adds no public classes.

accessor.get(ChronoField.HOUR_OF_AMPM)
} else if (accessor.isSupported(ChronoField.AMPM_OF_DAY) &&
accessor.get(ChronoField.AMPM_OF_DAY) == 1) {
// When reach here, the `hour` part is missing, the pattern is always contains `a` and minute
Copy link
Contributor

Choose a reason for hiding this comment

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

the hour part is missing and PM is specified

@SparkQA
Copy link

SparkQA commented Jun 3, 2020

Test build #123465 has finished for PR 28713 at commit c0ede09.

  • This patch fails Spark unit tests.
  • This patch does not merge cleanly.
  • This patch adds no public classes.

@yaooqinn
Copy link
Member Author

yaooqinn commented Jun 3, 2020

retest this please

@SparkQA
Copy link

SparkQA commented Jun 3, 2020

Test build #123468 has finished for PR 28713 at commit 6353ed7.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Jun 3, 2020

Test build #123467 has finished for PR 28713 at commit 6b35051.

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

@cloud-fan
Copy link
Contributor

thanks, merging to master/3.0!

cloud-fan pushed a commit that referenced this pull request Jun 3, 2020
### What changes were proposed in this pull request?

This PR set the hour to 12/0 when the AMPM_OF_DAY field exists

### Why are the changes needed?

When the hour is absent but the am-pm is present, the time is incorrect for pm

### Does this PR introduce _any_ user-facing change?
yes, the change is user-facing but to change back to 2.4 to keep backward compatibility

e.g.
```sql
spark-sql> select to_timestamp('33:33 PM', 'mm:ss a');
1970-01-01 12:33:33
spark-sql> select to_timestamp('33:33 AM', 'mm:ss a');
1970-01-01 00:33:33

```

otherwise, the results are all `1970-01-01 00:33:33`

### How was this patch tested?

add unit tests

Closes #28713 from yaooqinn/SPARK-31896.

Authored-by: Kent Yao <[email protected]>
Signed-off-by: Wenchen Fan <[email protected]>
(cherry picked from commit afcc14c)
Signed-off-by: Wenchen Fan <[email protected]>
@cloud-fan cloud-fan closed this in afcc14c Jun 3, 2020
@SparkQA
Copy link

SparkQA commented Jun 3, 2020

Test build #123481 has finished for PR 28713 at commit 6353ed7.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants