You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2020/05/19 06:17:38 UTC
[GitHub] [spark] cloud-fan opened a new pull request #28576: [SPARK-31755][SQL] allow missing year/hour when parsing date/timestamp string
cloud-fan opened a new pull request #28576:
URL: https://github.com/apache/spark/pull/28576
<!--
Thanks for sending a pull request! Here are some tips for you:
1. If this is your first time, please read our contributor guidelines: https://spark.apache.org/contributing.html
2. Ensure you have added or run the appropriate tests for your PR: https://spark.apache.org/developer-tools.html
3. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP][SPARK-XXXX] Your PR title ...'.
4. Be sure to keep the PR description updated to reflect all changes.
5. Please write your PR title to summarize what this PR proposes.
6. If possible, provide a concise example to reproduce the issue for a faster review.
7. If you want to add a new configuration, please read the guideline first for naming configurations in
'core/src/main/scala/org/apache/spark/internal/config/ConfigEntry.scala'.
-->
### What changes were proposed in this pull request?
<!--
Please clarify what changes you are proposing. The purpose of this section is to outline the changes and how this PR fixes the issue.
If possible, please consider writing useful notes for better and faster reviews in your PR. See the examples below.
1. If you refactor some codes with changing classes, showing the class hierarchy will help reviewers.
2. If you fix some SQL features, you can provide some references of other DBMSes.
3. If there is design documentation, please add the link.
4. If there is a discussion in the mailing list, please add the link.
-->
This PR allows missing year/hour fields when parsing date/timestamp string, with 1970 as the default value of year, and 0 as the default value of hour.
### Why are the changes needed?
<!--
Please clarify why the changes are needed. For instance,
1. If you propose a new API, clarify the use case for a new API.
2. If you fix a bug, you can clarify why it is a bug.
-->
To keep backward compatibility with Spark 2.4.
### Does this PR introduce _any_ user-facing change?
<!--
Note that it means *any* user-facing change including all aspects such as the documentation fix.
If yes, please clarify the previous behavior and the change this PR proposes - provide the console output, description and/or an example to show the behavior difference if possible.
If possible, please also clarify if this is a user-facing change compared to the released Spark versions or within the unreleased branches such as master.
If no, write 'No'.
-->
Yes.
Spark 2.4:
```
scala> sql("select to_timestamp('16', 'dd')").show
+------------------------+
|to_timestamp('16', 'dd')|
+------------------------+
| 1970-01-16 00:00:00|
+------------------------+
scala> sql("select to_date('16', 'dd')").show
+-------------------+
|to_date('16', 'dd')|
+-------------------+
| 1970-01-16|
+-------------------+
scala> sql("select to_timestamp('2019 40', 'yyyy mm')").show
+----------------------------------+
|to_timestamp('2019 40', 'yyyy mm')|
+----------------------------------+
| 2019-01-01 00:40:00|
+----------------------------------+
scala> sql("select to_timestamp('2019 10:10:10', 'yyyy hh:mm:ss')").show
+----------------------------------------------+
|to_timestamp('2019 10:10:10', 'yyyy hh:mm:ss')|
+----------------------------------------------+
| 2019-01-01 10:10:10|
+----------------------------------------------+
```
in branch 3.0
```
scala> sql("select to_timestamp('16', 'dd')").show
+--------------------+
|to_timestamp(16, dd)|
+--------------------+
| null|
+--------------------+
scala> sql("select to_date('16', 'dd')").show
+---------------+
|to_date(16, dd)|
+---------------+
| null|
+---------------+
scala> sql("select to_timestamp('2019 40', 'yyyy mm')").show
+------------------------------+
|to_timestamp(2019 40, yyyy mm)|
+------------------------------+
| 2019-01-01 00:00:00|
+------------------------------+
scala> sql("select to_timestamp('2019 10:10:10', 'yyyy hh:mm:ss')").show
+------------------------------------------+
|to_timestamp(2019 10:10:10, yyyy hh:mm:ss)|
+------------------------------------------+
| 2019-01-01 00:00:00|
+------------------------------------------+
```
After this PR, the behavior becomes the same as 2.4.
### How was this patch tested?
<!--
If tests were added, say they were added here. Please make sure to add some test cases that check the changes thoroughly including negative and positive cases if possible.
If it was tested in a way different from regular unit tests, please clarify how you tested step by step, ideally copy and paste-able, so that other reviewers can test and check, and descendants can verify in the future.
If tests were not added, please describe why they were not added and/or why it was difficult to add.
-->
new tests
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #28576: [SPARK-31755][SQL] allow missing year/hour when parsing date/timestamp string
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28576:
URL: https://github.com/apache/spark/pull/28576#issuecomment-631286052
**[Test build #122877 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/122877/testReport)** for PR 28576 at commit [`afb070c`](https://github.com/apache/spark/commit/afb070c4d38f5520af46c069a15e97c7f5465174).
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28576: [SPARK-31755][SQL] allow missing year/hour when parsing date/timestamp string
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28576:
URL: https://github.com/apache/spark/pull/28576#issuecomment-630627032
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/122832/
Test FAILed.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #28576: [SPARK-31755][SQL] allow missing year/hour when parsing date/timestamp string
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28576:
URL: https://github.com/apache/spark/pull/28576#issuecomment-630608610
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] xuanyuanking commented on a change in pull request #28576: [SPARK-31755][SQL] allow missing year/hour when parsing date/timestamp string
Posted by GitBox <gi...@apache.org>.
xuanyuanking commented on a change in pull request #28576:
URL: https://github.com/apache/spark/pull/28576#discussion_r427139637
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeFormatterHelper.scala
##########
@@ -31,17 +31,39 @@ import org.apache.spark.sql.internal.SQLConf
import org.apache.spark.sql.internal.SQLConf.LegacyBehaviorPolicy._
trait DateTimeFormatterHelper {
+ private def getOrDefault(accessor: TemporalAccessor, field: ChronoField, default: Int): Int = {
+ if (accessor.isSupported(field)) {
+ accessor.get(field)
+ } else {
+ default
+ }
+ }
+
+ protected def toLocalDate(temporalAccessor: TemporalAccessor): LocalDate = {
+ val year = getOrDefault(temporalAccessor, ChronoField.YEAR, 1970)
Review comment:
Maybe leave a comment here to illustrate the default year/month/day is for compatibility with Spark 2.4, it's a little strange to hard code the value here.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] bart-samwel commented on a change in pull request #28576: [SPARK-31755][SQL] allow missing year/hour when parsing date/timestamp string
Posted by GitBox <gi...@apache.org>.
bart-samwel commented on a change in pull request #28576:
URL: https://github.com/apache/spark/pull/28576#discussion_r427850722
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeFormatterHelper.scala
##########
@@ -31,17 +31,50 @@ import org.apache.spark.sql.internal.SQLConf
import org.apache.spark.sql.internal.SQLConf.LegacyBehaviorPolicy._
trait DateTimeFormatterHelper {
+ private def getOrDefault(accessor: TemporalAccessor, field: ChronoField, default: Int): Int = {
+ if (accessor.isSupported(field)) {
+ accessor.get(field)
+ } else {
+ default
+ }
+ }
+
+ protected def toLocalDate(accessor: TemporalAccessor, allowMissingYear: Boolean): LocalDate = {
+ val year = if (accessor.isSupported(ChronoField.YEAR)) {
+ accessor.get(ChronoField.YEAR)
+ } else if (allowMissingYear) {
+ // To keep backward compatibility with Spark 2.x, we pick 1970 as the default value of year.
+ 1970
+ } else {
+ throw new SparkUpgradeException("3.0",
+ "Year must be given in the date/timestamp string to be parsed. You can set " +
+ SQLConf.LEGACY_ALLOW_MISSING_YEAR_DURING_PARSING.key + " to true, to pick 1970 as " +
+ "the default value of year.", null)
+ }
+ val month = getOrDefault(accessor, ChronoField.MONTH_OF_YEAR, 1)
Review comment:
I guess we could even allow the default year, and if people hit Feb 29 then they'll get an error at runtime. No need for a legacy config in that case...
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28576: [SPARK-31755][SQL] allow missing year/hour when parsing date/timestamp string
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28576:
URL: https://github.com/apache/spark/pull/28576#issuecomment-632051493
Merged build finished. Test FAILed.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #28576: [SPARK-31755][SQL] allow missing year/hour when parsing date/timestamp string
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #28576:
URL: https://github.com/apache/spark/pull/28576#discussion_r427100400
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeFormatterHelper.scala
##########
@@ -31,17 +31,39 @@ import org.apache.spark.sql.internal.SQLConf
import org.apache.spark.sql.internal.SQLConf.LegacyBehaviorPolicy._
trait DateTimeFormatterHelper {
+ private def getOrDefault(accessor: TemporalAccessor, field: ChronoField, default: Int): Int = {
+ if (accessor.isSupported(field)) {
+ accessor.get(field)
+ } else {
+ default
+ }
+ }
+
+ protected def toLocalDate(temporalAccessor: TemporalAccessor): LocalDate = {
+ val year = getOrDefault(temporalAccessor, ChronoField.YEAR, 1970)
Review comment:
It's from 2.4. I don't care too much about it as long as we have a default value,
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #28576: [SPARK-31755][SQL] allow missing year/hour when parsing date/timestamp string
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28576:
URL: https://github.com/apache/spark/pull/28576#issuecomment-630894202
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] MaxGekk commented on a change in pull request #28576: [SPARK-31755][SQL] allow missing year/hour when parsing date/timestamp string
Posted by GitBox <gi...@apache.org>.
MaxGekk commented on a change in pull request #28576:
URL: https://github.com/apache/spark/pull/28576#discussion_r427139018
##########
File path: sql/catalyst/src/test/scala/org/apache/spark/sql/util/TimestampFormatterSuite.scala
##########
@@ -291,4 +291,95 @@ class TimestampFormatterSuite extends SparkFunSuite with SQLHelper with Matchers
}
}
}
+
+ test("parsing hour with various patterns") {
+ def createFormatter(pattern: String): TimestampFormatter = {
+ // Use `SIMPLE_DATE_FORMAT`, so that the legacy parser also fails with invalid value range.
+ TimestampFormatter(pattern, ZoneOffset.UTC, LegacyDateFormats.SIMPLE_DATE_FORMAT, false)
Review comment:
You can import `UTC` from DateTimeTestUtils instead of spreading ZoneOffset.UTC everywhere in tests.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #28576: [SPARK-31755][SQL] allow missing year/hour when parsing date/timestamp string
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28576:
URL: https://github.com/apache/spark/pull/28576#issuecomment-630893465
**[Test build #122851 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/122851/testReport)** for PR 28576 at commit [`58443e2`](https://github.com/apache/spark/commit/58443e2d211a614c206ca16b657875209e5302e1).
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #28576: [SPARK-31755][SQL] allow missing year/hour when parsing date/timestamp string
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #28576:
URL: https://github.com/apache/spark/pull/28576#discussion_r427054501
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeFormatterHelper.scala
##########
@@ -72,18 +94,14 @@ trait DateTimeFormatterHelper {
// DateTimeParseException will address by the caller side.
protected def checkDiffResult[T](
s: String, legacyParseFunc: String => T): PartialFunction[Throwable, T] = {
- case e: DateTimeParseException if SQLConf.get.legacyTimeParserPolicy == EXCEPTION =>
- val res = try {
- Some(legacyParseFunc(s))
- } catch {
- case _: Throwable => None
- }
- if (res.nonEmpty) {
+ case e: DateTimeException if SQLConf.get.legacyTimeParserPolicy == EXCEPTION =>
Review comment:
when the field value exceeds the valid range, the JDK throws `DateTimeException, we should catch it. `DateTimeParseException` extends `DateTimeException`
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #28576: [SPARK-31755][SQL] allow missing year/hour when parsing date/timestamp string
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #28576:
URL: https://github.com/apache/spark/pull/28576#discussion_r427869922
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeFormatterHelper.scala
##########
@@ -31,17 +31,50 @@ import org.apache.spark.sql.internal.SQLConf
import org.apache.spark.sql.internal.SQLConf.LegacyBehaviorPolicy._
trait DateTimeFormatterHelper {
+ private def getOrDefault(accessor: TemporalAccessor, field: ChronoField, default: Int): Int = {
+ if (accessor.isSupported(field)) {
+ accessor.get(field)
+ } else {
+ default
+ }
+ }
+
+ protected def toLocalDate(accessor: TemporalAccessor, allowMissingYear: Boolean): LocalDate = {
+ val year = if (accessor.isSupported(ChronoField.YEAR)) {
+ accessor.get(ChronoField.YEAR)
+ } else if (allowMissingYear) {
+ // To keep backward compatibility with Spark 2.x, we pick 1970 as the default value of year.
+ 1970
+ } else {
+ throw new SparkUpgradeException("3.0",
+ "Year must be given in the date/timestamp string to be parsed. You can set " +
+ SQLConf.LEGACY_ALLOW_MISSING_YEAR_DURING_PARSING.key + " to true, to pick 1970 as " +
+ "the default value of year.", null)
+ }
+ val month = getOrDefault(accessor, ChronoField.MONTH_OF_YEAR, 1)
Review comment:
ok so we should go back to the first version, which just uses 1970 as the default value and fails for invalid dates at runtime 😂
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA removed a comment on pull request #28576: [SPARK-31755][SQL] allow missing year/hour when parsing date/timestamp string
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #28576:
URL: https://github.com/apache/spark/pull/28576#issuecomment-632068610
**[Test build #122932 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/122932/testReport)** for PR 28576 at commit [`161223b`](https://github.com/apache/spark/commit/161223b5de92064a4cc3672702e09f868bd68870).
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #28576: [SPARK-31755][SQL] allow missing year/hour when parsing date/timestamp string
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #28576:
URL: https://github.com/apache/spark/pull/28576#discussion_r427054501
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeFormatterHelper.scala
##########
@@ -72,18 +94,14 @@ trait DateTimeFormatterHelper {
// DateTimeParseException will address by the caller side.
protected def checkDiffResult[T](
s: String, legacyParseFunc: String => T): PartialFunction[Throwable, T] = {
- case e: DateTimeParseException if SQLConf.get.legacyTimeParserPolicy == EXCEPTION =>
- val res = try {
- Some(legacyParseFunc(s))
- } catch {
- case _: Throwable => None
- }
- if (res.nonEmpty) {
+ case e: DateTimeException if SQLConf.get.legacyTimeParserPolicy == EXCEPTION =>
Review comment:
when the field value exceeds the valid range, the JDK throws `DateTimeException` too, we should catch it. `DateTimeParseException` extends `DateTimeException`
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #28576: [SPARK-31755][SQL] allow missing year/hour when parsing date/timestamp string
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28576:
URL: https://github.com/apache/spark/pull/28576#issuecomment-632069072
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] MaxGekk commented on a change in pull request #28576: [SPARK-31755][SQL] allow missing year/hour when parsing date/timestamp string
Posted by GitBox <gi...@apache.org>.
MaxGekk commented on a change in pull request #28576:
URL: https://github.com/apache/spark/pull/28576#discussion_r427140383
##########
File path: sql/catalyst/src/test/scala/org/apache/spark/sql/util/TimestampFormatterSuite.scala
##########
@@ -291,4 +291,95 @@ class TimestampFormatterSuite extends SparkFunSuite with SQLHelper with Matchers
}
}
}
+
+ test("parsing hour with various patterns") {
+ def createFormatter(pattern: String): TimestampFormatter = {
+ // Use `SIMPLE_DATE_FORMAT`, so that the legacy parser also fails with invalid value range.
+ TimestampFormatter(pattern, ZoneOffset.UTC, LegacyDateFormats.SIMPLE_DATE_FORMAT, false)
+ }
+
+ withClue("HH") {
+ val formatter = createFormatter("yyyy-MM-dd HH")
+
+ val micros1 = formatter.parse("2009-12-12 00")
+ assert(micros1 === TimeUnit.SECONDS.toMicros(
+ LocalDateTime.of(2009, 12, 12, 0, 0, 0).toEpochSecond(ZoneOffset.UTC)))
+
+ val micros2 = formatter.parse("2009-12-12 15")
+ assert(micros2 === TimeUnit.SECONDS.toMicros(
+ LocalDateTime.of(2009, 12, 12, 15, 0, 0).toEpochSecond(ZoneOffset.UTC)))
Review comment:
I would propose to improve test coverage and test different years -100, 0001, 1582, 1900, 1970, 2037, 2100
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28576: [SPARK-31755][SQL] allow missing year/hour when parsing date/timestamp string
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28576:
URL: https://github.com/apache/spark/pull/28576#issuecomment-632069072
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #28576: [SPARK-31755][SQL] allow missing year/hour when parsing date/timestamp string
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28576:
URL: https://github.com/apache/spark/pull/28576#issuecomment-632068610
**[Test build #122932 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/122932/testReport)** for PR 28576 at commit [`161223b`](https://github.com/apache/spark/commit/161223b5de92064a4cc3672702e09f868bd68870).
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #28576: [SPARK-31755][SQL] allow missing year/hour when parsing date/timestamp string
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28576:
URL: https://github.com/apache/spark/pull/28576#issuecomment-632051493
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon edited a comment on pull request #28576: [SPARK-31755][SQL] allow missing year/hour when parsing date/timestamp string
Posted by GitBox <gi...@apache.org>.
HyukjinKwon edited a comment on pull request #28576:
URL: https://github.com/apache/spark/pull/28576#issuecomment-632531311
Merged to master.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] bart-samwel commented on a change in pull request #28576: [SPARK-31755][SQL] allow missing year/hour when parsing date/timestamp string
Posted by GitBox <gi...@apache.org>.
bart-samwel commented on a change in pull request #28576:
URL: https://github.com/apache/spark/pull/28576#discussion_r427831829
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeFormatterHelper.scala
##########
@@ -31,17 +31,50 @@ import org.apache.spark.sql.internal.SQLConf
import org.apache.spark.sql.internal.SQLConf.LegacyBehaviorPolicy._
trait DateTimeFormatterHelper {
+ private def getOrDefault(accessor: TemporalAccessor, field: ChronoField, default: Int): Int = {
+ if (accessor.isSupported(field)) {
+ accessor.get(field)
+ } else {
+ default
+ }
+ }
+
+ protected def toLocalDate(accessor: TemporalAccessor, allowMissingYear: Boolean): LocalDate = {
+ val year = if (accessor.isSupported(ChronoField.YEAR)) {
+ accessor.get(ChronoField.YEAR)
+ } else if (allowMissingYear) {
+ // To keep backward compatibility with Spark 2.x, we pick 1970 as the default value of year.
+ 1970
+ } else {
+ throw new SparkUpgradeException("3.0",
+ "Year must be given in the date/timestamp string to be parsed. You can set " +
Review comment:
That's actually great -- if any part is missing from the format and the input, they can just provide a default by appending a constant to the input, _and_ a constant to the format string. I.e., the real format string is "MM dd" or "dd MM" or anything arbitrary but you're missing year, then you can *always* prepend "yyyy " to the format string, and "2009 " to the input value, and that fills out that part of the value.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28576: [SPARK-31755][SQL] allow missing year/hour when parsing date/timestamp string
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28576:
URL: https://github.com/apache/spark/pull/28576#issuecomment-630958801
Merged build finished. Test FAILed.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #28576: [SPARK-31755][SQL] allow missing year/hour when parsing date/timestamp string
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #28576:
URL: https://github.com/apache/spark/pull/28576#discussion_r427726709
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeFormatterHelper.scala
##########
@@ -31,17 +31,50 @@ import org.apache.spark.sql.internal.SQLConf
import org.apache.spark.sql.internal.SQLConf.LegacyBehaviorPolicy._
trait DateTimeFormatterHelper {
+ private def getOrDefault(accessor: TemporalAccessor, field: ChronoField, default: Int): Int = {
+ if (accessor.isSupported(field)) {
+ accessor.get(field)
+ } else {
+ default
+ }
+ }
+
+ protected def toLocalDate(accessor: TemporalAccessor, allowMissingYear: Boolean): LocalDate = {
+ val year = if (accessor.isSupported(ChronoField.YEAR)) {
+ accessor.get(ChronoField.YEAR)
+ } else if (allowMissingYear) {
+ // To keep backward compatibility with Spark 2.x, we pick 1970 as the default value of year.
+ 1970
+ } else {
+ throw new SparkUpgradeException("3.0",
+ "Year must be given in the date/timestamp string to be parsed. You can set " +
+ SQLConf.LEGACY_ALLOW_MISSING_YEAR_DURING_PARSING.key + " to true, to pick 1970 as " +
+ "the default value of year.", null)
+ }
+ val month = getOrDefault(accessor, ChronoField.MONTH_OF_YEAR, 1)
Review comment:
> asked for "mm" twice where they meant MM
This will error out unless the 2 mm parse to the same value, e.g. `mm mm yyyy` with value `10 10 1990`.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] bart-samwel commented on a change in pull request #28576: [SPARK-31755][SQL] allow missing year/hour when parsing date/timestamp string
Posted by GitBox <gi...@apache.org>.
bart-samwel commented on a change in pull request #28576:
URL: https://github.com/apache/spark/pull/28576#discussion_r427401584
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeFormatterHelper.scala
##########
@@ -31,17 +31,50 @@ import org.apache.spark.sql.internal.SQLConf
import org.apache.spark.sql.internal.SQLConf.LegacyBehaviorPolicy._
trait DateTimeFormatterHelper {
+ private def getOrDefault(accessor: TemporalAccessor, field: ChronoField, default: Int): Int = {
+ if (accessor.isSupported(field)) {
+ accessor.get(field)
+ } else {
+ default
+ }
+ }
+
+ protected def toLocalDate(accessor: TemporalAccessor, allowMissingYear: Boolean): LocalDate = {
+ val year = if (accessor.isSupported(ChronoField.YEAR)) {
+ accessor.get(ChronoField.YEAR)
+ } else if (allowMissingYear) {
+ // To keep backward compatibility with Spark 2.x, we pick 1970 as the default value of year.
+ 1970
+ } else {
+ throw new SparkUpgradeException("3.0",
+ "Year must be given in the date/timestamp string to be parsed. You can set " +
Review comment:
Maybe also suggest the alternative workaround, e.g. prepending `'1970 ' to the string-to-be-parsed and prepending `'yyyy '` to the format string. That one works without the legacy setting so I'd say it's preferred.
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeFormatterHelper.scala
##########
@@ -31,17 +31,50 @@ import org.apache.spark.sql.internal.SQLConf
import org.apache.spark.sql.internal.SQLConf.LegacyBehaviorPolicy._
trait DateTimeFormatterHelper {
+ private def getOrDefault(accessor: TemporalAccessor, field: ChronoField, default: Int): Int = {
+ if (accessor.isSupported(field)) {
+ accessor.get(field)
+ } else {
+ default
+ }
+ }
+
+ protected def toLocalDate(accessor: TemporalAccessor, allowMissingYear: Boolean): LocalDate = {
+ val year = if (accessor.isSupported(ChronoField.YEAR)) {
+ accessor.get(ChronoField.YEAR)
+ } else if (allowMissingYear) {
+ // To keep backward compatibility with Spark 2.x, we pick 1970 as the default value of year.
+ 1970
+ } else {
+ throw new SparkUpgradeException("3.0",
+ "Year must be given in the date/timestamp string to be parsed. You can set " +
+ SQLConf.LEGACY_ALLOW_MISSING_YEAR_DURING_PARSING.key + " to true, to pick 1970 as " +
+ "the default value of year.", null)
+ }
+ val month = getOrDefault(accessor, ChronoField.MONTH_OF_YEAR, 1)
Review comment:
Are we also going to error out if they specify the day but not the month? Really, the only formats that make sense are the ones where a full prefix is given in the y-m-d h-m-s sequence, and all others are likely to be a case where they made a mistake (e.g. asked for "mm" twice where they meant MM).
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala
##########
@@ -2586,6 +2586,15 @@ object SQLConf {
.checkValue(_ > 0, "The timeout value must be positive")
.createWithDefault(10L)
+ val LEGACY_ALLOW_MISSING_YEAR_DURING_PARSING =
+ buildConf("spark.sql.legacy.allowMissingYearDuringParsing")
+ .internal()
+ .doc("When true, DateFormatter/TimestampFormatter allows parsing date/timestamp string " +
Review comment:
Here too, you could suggest the alternative workaround that doesn't require setting the legacy flag.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28576: [SPARK-31755][SQL] allow missing year/hour when parsing date/timestamp string
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28576:
URL: https://github.com/apache/spark/pull/28576#issuecomment-631990533
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #28576: [SPARK-31755][SQL] allow missing year/hour when parsing date/timestamp string
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28576:
URL: https://github.com/apache/spark/pull/28576#issuecomment-631427668
**[Test build #122880 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/122880/testReport)** for PR 28576 at commit [`b17a648`](https://github.com/apache/spark/commit/b17a648bf31762f15cc414117308402649b92dde).
* This patch **fails Spark unit tests**.
* This patch merges cleanly.
* This patch adds no public classes.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #28576: [SPARK-31755][SQL] allow missing year/hour when parsing date/timestamp string
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #28576:
URL: https://github.com/apache/spark/pull/28576#discussion_r427100400
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeFormatterHelper.scala
##########
@@ -31,17 +31,39 @@ import org.apache.spark.sql.internal.SQLConf
import org.apache.spark.sql.internal.SQLConf.LegacyBehaviorPolicy._
trait DateTimeFormatterHelper {
+ private def getOrDefault(accessor: TemporalAccessor, field: ChronoField, default: Int): Int = {
+ if (accessor.isSupported(field)) {
+ accessor.get(field)
+ } else {
+ default
+ }
+ }
+
+ protected def toLocalDate(temporalAccessor: TemporalAccessor): LocalDate = {
+ val year = getOrDefault(temporalAccessor, ChronoField.YEAR, 1970)
Review comment:
It's from 2.4. I don't care too much about it as long as we have a default value.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28576: [SPARK-31755][SQL] allow missing year/hour when parsing date/timestamp string
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28576:
URL: https://github.com/apache/spark/pull/28576#issuecomment-630627023
Merged build finished. Test FAILed.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #28576: [SPARK-31755][SQL] allow missing year/hour when parsing date/timestamp string
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #28576:
URL: https://github.com/apache/spark/pull/28576#discussion_r427054501
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeFormatterHelper.scala
##########
@@ -72,18 +94,14 @@ trait DateTimeFormatterHelper {
// DateTimeParseException will address by the caller side.
protected def checkDiffResult[T](
s: String, legacyParseFunc: String => T): PartialFunction[Throwable, T] = {
- case e: DateTimeParseException if SQLConf.get.legacyTimeParserPolicy == EXCEPTION =>
- val res = try {
- Some(legacyParseFunc(s))
- } catch {
- case _: Throwable => None
- }
- if (res.nonEmpty) {
+ case e: DateTimeException if SQLConf.get.legacyTimeParserPolicy == EXCEPTION =>
Review comment:
when the field value exceeds the valid range, the JDK throws `DateTimeException`, and we should catch it too. `DateTimeParseException` extends `DateTimeException`
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #28576: [SPARK-31755][SQL] allow missing year/hour when parsing date/timestamp string
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #28576:
URL: https://github.com/apache/spark/pull/28576#discussion_r427726258
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeFormatterHelper.scala
##########
@@ -31,17 +31,50 @@ import org.apache.spark.sql.internal.SQLConf
import org.apache.spark.sql.internal.SQLConf.LegacyBehaviorPolicy._
trait DateTimeFormatterHelper {
+ private def getOrDefault(accessor: TemporalAccessor, field: ChronoField, default: Int): Int = {
+ if (accessor.isSupported(field)) {
+ accessor.get(field)
+ } else {
+ default
+ }
+ }
+
+ protected def toLocalDate(accessor: TemporalAccessor, allowMissingYear: Boolean): LocalDate = {
+ val year = if (accessor.isSupported(ChronoField.YEAR)) {
+ accessor.get(ChronoField.YEAR)
+ } else if (allowMissingYear) {
+ // To keep backward compatibility with Spark 2.x, we pick 1970 as the default value of year.
+ 1970
+ } else {
+ throw new SparkUpgradeException("3.0",
+ "Year must be given in the date/timestamp string to be parsed. You can set " +
Review comment:
A problem is that, the pattern is flexible and the year may not be in the beginning, e.g. `MM/dd yyyy ...`. I don't think there is a workaround, unless we provide an API to allow users to set the default values.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on pull request #28576: [SPARK-31755][SQL] allow missing year/hour when parsing date/timestamp string
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on pull request #28576:
URL: https://github.com/apache/spark/pull/28576#issuecomment-632532022
@cloud-fan, there's conflict on SQL file output. It's trivial but can you just open a PR to backport since we're running RCs?
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] MaxGekk commented on a change in pull request #28576: [SPARK-31755][SQL] allow missing year/hour when parsing date/timestamp string
Posted by GitBox <gi...@apache.org>.
MaxGekk commented on a change in pull request #28576:
URL: https://github.com/apache/spark/pull/28576#discussion_r427137030
##########
File path: sql/catalyst/src/test/scala/org/apache/spark/sql/util/TimestampFormatterSuite.scala
##########
@@ -291,4 +291,95 @@ class TimestampFormatterSuite extends SparkFunSuite with SQLHelper with Matchers
}
}
}
+
+ test("parsing hour with various patterns") {
+ def createFormatter(pattern: String): TimestampFormatter = {
+ // Use `SIMPLE_DATE_FORMAT`, so that the legacy parser also fails with invalid value range.
+ TimestampFormatter(pattern, ZoneOffset.UTC, LegacyDateFormats.SIMPLE_DATE_FORMAT, false)
+ }
+
+ withClue("HH") {
+ val formatter = createFormatter("yyyy-MM-dd HH")
+
+ val micros1 = formatter.parse("2009-12-12 00")
+ assert(micros1 === TimeUnit.SECONDS.toMicros(
+ LocalDateTime.of(2009, 12, 12, 0, 0, 0).toEpochSecond(ZoneOffset.UTC)))
Review comment:
Can you use DateTimeTestUtils.days https://github.com/apache/spark/blob/9f0c010a5c9b5c10401f8e16fa7a151714b6dbb0/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/util/DateTimeTestUtils.scala#L72-L85?
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on pull request #28576: [SPARK-31755][SQL] allow missing year/hour when parsing date/timestamp string
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on pull request #28576:
URL: https://github.com/apache/spark/pull/28576#issuecomment-630820332
> The reason being that it is not a leap year, which means that it would never parse Feb 29.
This is a good point. Now I agree 1970 is not a good default. I'll fail it by default, with a legacy config to use 1970 as default.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #28576: [SPARK-31755][SQL] allow missing year/hour when parsing date/timestamp string
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #28576:
URL: https://github.com/apache/spark/pull/28576#discussion_r427333877
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeFormatterHelper.scala
##########
@@ -31,17 +31,39 @@ import org.apache.spark.sql.internal.SQLConf
import org.apache.spark.sql.internal.SQLConf.LegacyBehaviorPolicy._
trait DateTimeFormatterHelper {
+ private def getOrDefault(accessor: TemporalAccessor, field: ChronoField, default: Int): Int = {
+ if (accessor.isSupported(field)) {
+ accessor.get(field)
+ } else {
+ default
+ }
+ }
+
+ protected def toLocalDate(temporalAccessor: TemporalAccessor): LocalDate = {
+ val year = getOrDefault(temporalAccessor, ChronoField.YEAR, 1970)
Review comment:
`TemporalAccessor.get` is not getting the field, it's actually a query. So for `yyyy G`, we can still query the `YEAR` field because both `YEAR_OF_ERA` and `ERA` are available.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] MaxGekk commented on a change in pull request #28576: [SPARK-31755][SQL] allow missing year/hour when parsing date/timestamp string
Posted by GitBox <gi...@apache.org>.
MaxGekk commented on a change in pull request #28576:
URL: https://github.com/apache/spark/pull/28576#discussion_r427120597
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeFormatterHelper.scala
##########
@@ -31,17 +31,39 @@ import org.apache.spark.sql.internal.SQLConf
import org.apache.spark.sql.internal.SQLConf.LegacyBehaviorPolicy._
trait DateTimeFormatterHelper {
+ private def getOrDefault(accessor: TemporalAccessor, field: ChronoField, default: Int): Int = {
+ if (accessor.isSupported(field)) {
+ accessor.get(field)
+ } else {
+ default
+ }
+ }
+
+ protected def toLocalDate(temporalAccessor: TemporalAccessor): LocalDate = {
+ val year = getOrDefault(temporalAccessor, ChronoField.YEAR, 1970)
Review comment:
How about the case when ERA is set. In this case, year will be in the `YEAR_OF_ERA` fields, right?
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28576: [SPARK-31755][SQL] allow missing year/hour when parsing date/timestamp string
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28576:
URL: https://github.com/apache/spark/pull/28576#issuecomment-631325110
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #28576: [SPARK-31755][SQL] allow missing year/hour when parsing date/timestamp string
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #28576:
URL: https://github.com/apache/spark/pull/28576#discussion_r427101383
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeFormatterHelper.scala
##########
@@ -31,17 +31,39 @@ import org.apache.spark.sql.internal.SQLConf
import org.apache.spark.sql.internal.SQLConf.LegacyBehaviorPolicy._
trait DateTimeFormatterHelper {
+ private def getOrDefault(accessor: TemporalAccessor, field: ChronoField, default: Int): Int = {
+ if (accessor.isSupported(field)) {
+ accessor.get(field)
+ } else {
+ default
+ }
+ }
+
+ protected def toLocalDate(temporalAccessor: TemporalAccessor): LocalDate = {
+ val year = getOrDefault(temporalAccessor, ChronoField.YEAR, 1970)
+ val month = getOrDefault(temporalAccessor, ChronoField.MONTH_OF_YEAR, 1)
+ val day = getOrDefault(temporalAccessor, ChronoField.DAY_OF_MONTH, 1)
+ LocalDate.of(year, month, day)
+ }
+
// Converts the parsed temporal object to ZonedDateTime. It sets time components to zeros
// if they does not exist in the parsed object.
protected def toZonedDateTime(
Review comment:
I agree. Can you help to re-generate benchmark numbers and send me a PR? It was generated by you last time and I'm afraid rerunning it locally at my side can't help us to show the difference.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #28576: [SPARK-31755][SQL] allow missing year/hour when parsing date/timestamp string
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28576:
URL: https://github.com/apache/spark/pull/28576#issuecomment-631989880
**[Test build #122923 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/122923/testReport)** for PR 28576 at commit [`8de3bda`](https://github.com/apache/spark/commit/8de3bdae1038810409f5fa1bea9e700c058af21b).
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #28576: [SPARK-31755][SQL] allow missing year/hour when parsing date/timestamp string
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28576:
URL: https://github.com/apache/spark/pull/28576#issuecomment-631324288
**[Test build #122880 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/122880/testReport)** for PR 28576 at commit [`b17a648`](https://github.com/apache/spark/commit/b17a648bf31762f15cc414117308402649b92dde).
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA removed a comment on pull request #28576: [SPARK-31755][SQL] allow missing year/hour when parsing date/timestamp string
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #28576:
URL: https://github.com/apache/spark/pull/28576#issuecomment-630893465
**[Test build #122851 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/122851/testReport)** for PR 28576 at commit [`58443e2`](https://github.com/apache/spark/commit/58443e2d211a614c206ca16b657875209e5302e1).
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28576: [SPARK-31755][SQL] allow missing year/hour when parsing date/timestamp string
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28576:
URL: https://github.com/apache/spark/pull/28576#issuecomment-631427935
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/122880/
Test FAILed.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] bart-samwel commented on a change in pull request #28576: [SPARK-31755][SQL] allow missing year/hour when parsing date/timestamp string
Posted by GitBox <gi...@apache.org>.
bart-samwel commented on a change in pull request #28576:
URL: https://github.com/apache/spark/pull/28576#discussion_r427892129
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeFormatterHelper.scala
##########
@@ -31,17 +31,50 @@ import org.apache.spark.sql.internal.SQLConf
import org.apache.spark.sql.internal.SQLConf.LegacyBehaviorPolicy._
trait DateTimeFormatterHelper {
+ private def getOrDefault(accessor: TemporalAccessor, field: ChronoField, default: Int): Int = {
+ if (accessor.isSupported(field)) {
+ accessor.get(field)
+ } else {
+ default
+ }
+ }
+
+ protected def toLocalDate(accessor: TemporalAccessor, allowMissingYear: Boolean): LocalDate = {
+ val year = if (accessor.isSupported(ChronoField.YEAR)) {
+ accessor.get(ChronoField.YEAR)
+ } else if (allowMissingYear) {
+ // To keep backward compatibility with Spark 2.x, we pick 1970 as the default value of year.
+ 1970
+ } else {
+ throw new SparkUpgradeException("3.0",
+ "Year must be given in the date/timestamp string to be parsed. You can set " +
+ SQLConf.LEGACY_ALLOW_MISSING_YEAR_DURING_PARSING.key + " to true, to pick 1970 as " +
+ "the default value of year.", null)
+ }
+ val month = getOrDefault(accessor, ChronoField.MONTH_OF_YEAR, 1)
Review comment:
Error out in ANSI mode only, I'd say, just like with all other parse failures (which should fail in ANSI mode -- I hope they already do).
FWIW, going back to the first version is not a shame now that we know it's the right thing to do. :) Maybe we can leave some comments in the code to explain the reasoning why we don't error out if things are missing.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon closed pull request #28576: [SPARK-31755][SQL] allow missing year/hour when parsing date/timestamp string
Posted by GitBox <gi...@apache.org>.
HyukjinKwon closed pull request #28576:
URL: https://github.com/apache/spark/pull/28576
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon edited a comment on pull request #28576: [SPARK-31755][SQL] allow missing year/hour when parsing date/timestamp string
Posted by GitBox <gi...@apache.org>.
HyukjinKwon edited a comment on pull request #28576:
URL: https://github.com/apache/spark/pull/28576#issuecomment-632532022
@cloud-fan, there's conflict on SQL file output against branch-3.0. It's trivial but can you just open a PR to backport since we're running RCs?
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #28576: [SPARK-31755][SQL] allow missing year/hour when parsing date/timestamp string
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28576:
URL: https://github.com/apache/spark/pull/28576#issuecomment-630626931
**[Test build #122832 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/122832/testReport)** for PR 28576 at commit [`086eeb5`](https://github.com/apache/spark/commit/086eeb55364530c20ed453857faa16131f044d76).
* This patch **fails due to an unknown error code, -9**.
* This patch merges cleanly.
* This patch adds no public classes.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] MaxGekk commented on a change in pull request #28576: [SPARK-31755][SQL] allow missing year/hour when parsing date/timestamp string
Posted by GitBox <gi...@apache.org>.
MaxGekk commented on a change in pull request #28576:
URL: https://github.com/apache/spark/pull/28576#discussion_r427087787
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeFormatterHelper.scala
##########
@@ -31,17 +31,39 @@ import org.apache.spark.sql.internal.SQLConf
import org.apache.spark.sql.internal.SQLConf.LegacyBehaviorPolicy._
trait DateTimeFormatterHelper {
+ private def getOrDefault(accessor: TemporalAccessor, field: ChronoField, default: Int): Int = {
+ if (accessor.isSupported(field)) {
+ accessor.get(field)
+ } else {
+ default
+ }
+ }
+
+ protected def toLocalDate(temporalAccessor: TemporalAccessor): LocalDate = {
+ val year = getOrDefault(temporalAccessor, ChronoField.YEAR, 1970)
+ val month = getOrDefault(temporalAccessor, ChronoField.MONTH_OF_YEAR, 1)
+ val day = getOrDefault(temporalAccessor, ChronoField.DAY_OF_MONTH, 1)
+ LocalDate.of(year, month, day)
+ }
+
// Converts the parsed temporal object to ZonedDateTime. It sets time components to zeros
// if they does not exist in the parsed object.
protected def toZonedDateTime(
Review comment:
I guess, the additional functional calls will slow down overall performance. I think it makes sense to re-run datetime benchmarks.
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeFormatterHelper.scala
##########
@@ -31,17 +31,39 @@ import org.apache.spark.sql.internal.SQLConf
import org.apache.spark.sql.internal.SQLConf.LegacyBehaviorPolicy._
trait DateTimeFormatterHelper {
+ private def getOrDefault(accessor: TemporalAccessor, field: ChronoField, default: Int): Int = {
+ if (accessor.isSupported(field)) {
+ accessor.get(field)
+ } else {
+ default
+ }
+ }
+
+ protected def toLocalDate(temporalAccessor: TemporalAccessor): LocalDate = {
+ val year = getOrDefault(temporalAccessor, ChronoField.YEAR, 1970)
Review comment:
How often do users consider 1970 as the default year, your guess? I would propose current year as we do in stringToTimestamp:
https://github.com/apache/spark/blob/ebc8fa50d0422f3db47b2c45025c7f2efe6ee39a/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeUtils.scala#L396
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] MaxGekk commented on pull request #28576: [SPARK-31755][SQL] allow missing year/hour when parsing date/timestamp string
Posted by GitBox <gi...@apache.org>.
MaxGekk commented on pull request #28576:
URL: https://github.com/apache/spark/pull/28576#issuecomment-631646962
I have re-generated results of DateTimeBenchmark, CSVBenchmark and JsonBenchmark on JDK 8 and 11. @cloud-fan Please, review and merge this PR https://github.com/cloud-fan/spark/pull/17
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #28576: [SPARK-31755][SQL] allow missing year/hour when parsing date/timestamp string
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28576:
URL: https://github.com/apache/spark/pull/28576#issuecomment-631430799
**[Test build #122877 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/122877/testReport)** for PR 28576 at commit [`afb070c`](https://github.com/apache/spark/commit/afb070c4d38f5520af46c069a15e97c7f5465174).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #28576: [SPARK-31755][SQL] allow missing year/hour when parsing date/timestamp string
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28576:
URL: https://github.com/apache/spark/pull/28576#issuecomment-631431871
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28576: [SPARK-31755][SQL] allow missing year/hour when parsing date/timestamp string
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28576:
URL: https://github.com/apache/spark/pull/28576#issuecomment-630958815
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/122851/
Test FAILed.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #28576: [SPARK-31755][SQL] allow missing year/hour when parsing date/timestamp string
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #28576:
URL: https://github.com/apache/spark/pull/28576#discussion_r427726478
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeFormatterHelper.scala
##########
@@ -31,17 +31,50 @@ import org.apache.spark.sql.internal.SQLConf
import org.apache.spark.sql.internal.SQLConf.LegacyBehaviorPolicy._
trait DateTimeFormatterHelper {
+ private def getOrDefault(accessor: TemporalAccessor, field: ChronoField, default: Int): Int = {
+ if (accessor.isSupported(field)) {
+ accessor.get(field)
+ } else {
+ default
+ }
+ }
+
+ protected def toLocalDate(accessor: TemporalAccessor, allowMissingYear: Boolean): LocalDate = {
+ val year = if (accessor.isSupported(ChronoField.YEAR)) {
+ accessor.get(ChronoField.YEAR)
+ } else if (allowMissingYear) {
+ // To keep backward compatibility with Spark 2.x, we pick 1970 as the default value of year.
+ 1970
+ } else {
+ throw new SparkUpgradeException("3.0",
+ "Year must be given in the date/timestamp string to be parsed. You can set " +
+ SQLConf.LEGACY_ALLOW_MISSING_YEAR_DURING_PARSING.key + " to true, to pick 1970 as " +
+ "the default value of year.", null)
+ }
+ val month = getOrDefault(accessor, ChronoField.MONTH_OF_YEAR, 1)
Review comment:
January should be fine? It accepts any DAY_OF_MONTH.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #28576: [SPARK-31755][SQL] allow missing year/hour when parsing date/timestamp string
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28576:
URL: https://github.com/apache/spark/pull/28576#issuecomment-631990533
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #28576: [SPARK-31755][SQL] allow missing year/hour when parsing date/timestamp string
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28576:
URL: https://github.com/apache/spark/pull/28576#issuecomment-632244984
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #28576: [SPARK-31755][SQL] allow missing year/hour when parsing date/timestamp string
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28576:
URL: https://github.com/apache/spark/pull/28576#issuecomment-630627023
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] MaxGekk commented on a change in pull request #28576: [SPARK-31755][SQL] allow missing year/hour when parsing date/timestamp string
Posted by GitBox <gi...@apache.org>.
MaxGekk commented on a change in pull request #28576:
URL: https://github.com/apache/spark/pull/28576#discussion_r427104938
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeFormatterHelper.scala
##########
@@ -31,17 +31,39 @@ import org.apache.spark.sql.internal.SQLConf
import org.apache.spark.sql.internal.SQLConf.LegacyBehaviorPolicy._
trait DateTimeFormatterHelper {
+ private def getOrDefault(accessor: TemporalAccessor, field: ChronoField, default: Int): Int = {
+ if (accessor.isSupported(field)) {
+ accessor.get(field)
+ } else {
+ default
+ }
+ }
+
+ protected def toLocalDate(temporalAccessor: TemporalAccessor): LocalDate = {
+ val year = getOrDefault(temporalAccessor, ChronoField.YEAR, 1970)
Review comment:
It is some kind of inconsistent behaviour between CAST and parsing via patterns (in to_timestamp for example). Should we document somewhere the default year?
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on a change in pull request #28576: [SPARK-31755][SQL] allow missing year/hour when parsing date/timestamp string
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on a change in pull request #28576:
URL: https://github.com/apache/spark/pull/28576#discussion_r429075994
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeFormatterHelper.scala
##########
@@ -31,17 +31,52 @@ import org.apache.spark.sql.internal.SQLConf
import org.apache.spark.sql.internal.SQLConf.LegacyBehaviorPolicy._
trait DateTimeFormatterHelper {
+ private def getOrDefault(accessor: TemporalAccessor, field: ChronoField, default: Int): Int = {
Review comment:
nit: I would follow the naming of `getOrElse` though ..
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #28576: [SPARK-31755][SQL] allow missing year/hour when parsing date/timestamp string
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28576:
URL: https://github.com/apache/spark/pull/28576#issuecomment-632243655
**[Test build #122932 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/122932/testReport)** for PR 28576 at commit [`161223b`](https://github.com/apache/spark/commit/161223b5de92064a4cc3672702e09f868bd68870).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28576: [SPARK-31755][SQL] allow missing year/hour when parsing date/timestamp string
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28576:
URL: https://github.com/apache/spark/pull/28576#issuecomment-632244984
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28576: [SPARK-31755][SQL] allow missing year/hour when parsing date/timestamp string
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28576:
URL: https://github.com/apache/spark/pull/28576#issuecomment-631431871
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #28576: [SPARK-31755][SQL] allow missing year/hour when parsing date/timestamp string
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #28576:
URL: https://github.com/apache/spark/pull/28576#discussion_r427847756
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeFormatterHelper.scala
##########
@@ -31,17 +31,50 @@ import org.apache.spark.sql.internal.SQLConf
import org.apache.spark.sql.internal.SQLConf.LegacyBehaviorPolicy._
trait DateTimeFormatterHelper {
+ private def getOrDefault(accessor: TemporalAccessor, field: ChronoField, default: Int): Int = {
+ if (accessor.isSupported(field)) {
+ accessor.get(field)
+ } else {
+ default
+ }
+ }
+
+ protected def toLocalDate(accessor: TemporalAccessor, allowMissingYear: Boolean): LocalDate = {
+ val year = if (accessor.isSupported(ChronoField.YEAR)) {
+ accessor.get(ChronoField.YEAR)
+ } else if (allowMissingYear) {
+ // To keep backward compatibility with Spark 2.x, we pick 1970 as the default value of year.
+ 1970
+ } else {
+ throw new SparkUpgradeException("3.0",
+ "Year must be given in the date/timestamp string to be parsed. You can set " +
+ SQLConf.LEGACY_ALLOW_MISSING_YEAR_DURING_PARSING.key + " to true, to pick 1970 as " +
+ "the default value of year.", null)
+ }
+ val month = getOrDefault(accessor, ChronoField.MONTH_OF_YEAR, 1)
Review comment:
ah this looks better!
I'm wondering if we still need the legacy config though. It's very hard to hit the missing year exception, and if it happens it's very likely to be a data error. Users should fix the data or change the pattern and provide the default year.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] bart-samwel commented on a change in pull request #28576: [SPARK-31755][SQL] allow missing year/hour when parsing date/timestamp string
Posted by GitBox <gi...@apache.org>.
bart-samwel commented on a change in pull request #28576:
URL: https://github.com/apache/spark/pull/28576#discussion_r428104252
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeFormatterHelper.scala
##########
@@ -31,17 +31,50 @@ import org.apache.spark.sql.internal.SQLConf
import org.apache.spark.sql.internal.SQLConf.LegacyBehaviorPolicy._
trait DateTimeFormatterHelper {
+ private def getOrDefault(accessor: TemporalAccessor, field: ChronoField, default: Int): Int = {
+ if (accessor.isSupported(field)) {
+ accessor.get(field)
+ } else {
+ default
+ }
+ }
+
+ protected def toLocalDate(accessor: TemporalAccessor, allowMissingYear: Boolean): LocalDate = {
+ val year = if (accessor.isSupported(ChronoField.YEAR)) {
+ accessor.get(ChronoField.YEAR)
+ } else if (allowMissingYear) {
+ // To keep backward compatibility with Spark 2.x, we pick 1970 as the default value of year.
+ 1970
+ } else {
+ throw new SparkUpgradeException("3.0",
+ "Year must be given in the date/timestamp string to be parsed. You can set " +
+ SQLConf.LEGACY_ALLOW_MISSING_YEAR_DURING_PARSING.key + " to true, to pick 1970 as " +
+ "the default value of year.", null)
+ }
+ val month = getOrDefault(accessor, ChronoField.MONTH_OF_YEAR, 1)
Review comment:
Yikes! It seems terrible that we have to do that later, because then it can break existing workloads. Launching the ANSI behavior was our chance to do the right thing here. If we can still fix this in 3.0 then that would of course be preferable, but that seems like a stretch...
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #28576: [SPARK-31755][SQL] allow missing year/hour when parsing date/timestamp string
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28576:
URL: https://github.com/apache/spark/pull/28576#issuecomment-631427928
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on pull request #28576: [SPARK-31755][SQL] allow missing year/hour when parsing date/timestamp string
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on pull request #28576:
URL: https://github.com/apache/spark/pull/28576#issuecomment-630608989
cc @MaxGekk @xuanyuanking @HyukjinKwon
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #28576: [SPARK-31755][SQL] allow missing year/hour when parsing date/timestamp string
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28576:
URL: https://github.com/apache/spark/pull/28576#issuecomment-630608038
**[Test build #122832 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/122832/testReport)** for PR 28576 at commit [`086eeb5`](https://github.com/apache/spark/commit/086eeb55364530c20ed453857faa16131f044d76).
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #28576: [SPARK-31755][SQL] allow missing year/hour when parsing date/timestamp string
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #28576:
URL: https://github.com/apache/spark/pull/28576#discussion_r427055167
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeFormatterHelper.scala
##########
@@ -101,10 +119,6 @@ private object DateTimeFormatterHelper {
def toFormatter(builder: DateTimeFormatterBuilder, locale: Locale): DateTimeFormatter = {
builder
- .parseDefaulting(ChronoField.MONTH_OF_YEAR, 1)
- .parseDefaulting(ChronoField.DAY_OF_MONTH, 1)
- .parseDefaulting(ChronoField.MINUTE_OF_HOUR, 0)
- .parseDefaulting(ChronoField.SECOND_OF_MINUTE, 0)
Review comment:
It's too fragile to use these default values, as it may fail to parse due to conflicts. This PR decides default values manually, after parsing. See https://github.com/apache/spark/pull/28576/files#diff-3e4b85f54d2e75f1b5b845279749024eR42
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA removed a comment on pull request #28576: [SPARK-31755][SQL] allow missing year/hour when parsing date/timestamp string
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #28576:
URL: https://github.com/apache/spark/pull/28576#issuecomment-631989880
**[Test build #122923 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/122923/testReport)** for PR 28576 at commit [`8de3bda`](https://github.com/apache/spark/commit/8de3bdae1038810409f5fa1bea9e700c058af21b).
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28576: [SPARK-31755][SQL] allow missing year/hour when parsing date/timestamp string
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28576:
URL: https://github.com/apache/spark/pull/28576#issuecomment-631283001
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on pull request #28576: [SPARK-31755][SQL] allow missing year/hour when parsing date/timestamp string
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on pull request #28576:
URL: https://github.com/apache/spark/pull/28576#issuecomment-632531311
Merged to master and branch-3.0.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #28576: [SPARK-31755][SQL] allow missing year/hour when parsing date/timestamp string
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #28576:
URL: https://github.com/apache/spark/pull/28576#discussion_r427870805
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeFormatterHelper.scala
##########
@@ -31,17 +31,50 @@ import org.apache.spark.sql.internal.SQLConf
import org.apache.spark.sql.internal.SQLConf.LegacyBehaviorPolicy._
trait DateTimeFormatterHelper {
+ private def getOrDefault(accessor: TemporalAccessor, field: ChronoField, default: Int): Int = {
+ if (accessor.isSupported(field)) {
+ accessor.get(field)
+ } else {
+ default
+ }
+ }
+
+ protected def toLocalDate(accessor: TemporalAccessor, allowMissingYear: Boolean): LocalDate = {
+ val year = if (accessor.isSupported(ChronoField.YEAR)) {
+ accessor.get(ChronoField.YEAR)
+ } else if (allowMissingYear) {
+ // To keep backward compatibility with Spark 2.x, we pick 1970 as the default value of year.
+ 1970
+ } else {
+ throw new SparkUpgradeException("3.0",
+ "Year must be given in the date/timestamp string to be parsed. You can set " +
+ SQLConf.LEGACY_ALLOW_MISSING_YEAR_DURING_PARSING.key + " to true, to pick 1970 as " +
+ "the default value of year.", null)
+ }
+ val month = getOrDefault(accessor, ChronoField.MONTH_OF_YEAR, 1)
Review comment:
BTW the current framework just returns null if the parser fails. Shall we treat this case specially and throw an error that fails the query?
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] MaxGekk commented on a change in pull request #28576: [SPARK-31755][SQL] allow missing year/hour when parsing date/timestamp string
Posted by GitBox <gi...@apache.org>.
MaxGekk commented on a change in pull request #28576:
URL: https://github.com/apache/spark/pull/28576#discussion_r427140383
##########
File path: sql/catalyst/src/test/scala/org/apache/spark/sql/util/TimestampFormatterSuite.scala
##########
@@ -291,4 +291,95 @@ class TimestampFormatterSuite extends SparkFunSuite with SQLHelper with Matchers
}
}
}
+
+ test("parsing hour with various patterns") {
+ def createFormatter(pattern: String): TimestampFormatter = {
+ // Use `SIMPLE_DATE_FORMAT`, so that the legacy parser also fails with invalid value range.
+ TimestampFormatter(pattern, ZoneOffset.UTC, LegacyDateFormats.SIMPLE_DATE_FORMAT, false)
+ }
+
+ withClue("HH") {
+ val formatter = createFormatter("yyyy-MM-dd HH")
+
+ val micros1 = formatter.parse("2009-12-12 00")
+ assert(micros1 === TimeUnit.SECONDS.toMicros(
+ LocalDateTime.of(2009, 12, 12, 0, 0, 0).toEpochSecond(ZoneOffset.UTC)))
+
+ val micros2 = formatter.parse("2009-12-12 15")
+ assert(micros2 === TimeUnit.SECONDS.toMicros(
+ LocalDateTime.of(2009, 12, 12, 15, 0, 0).toEpochSecond(ZoneOffset.UTC)))
Review comment:
I would propose to improve test coverage and test different years -100, 0001, 1582, 1900, 1970, 2037, 2100 instead of checking only one year in all tests
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #28576: [SPARK-31755][SQL] allow missing year/hour when parsing date/timestamp string
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28576:
URL: https://github.com/apache/spark/pull/28576#issuecomment-631325110
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA removed a comment on pull request #28576: [SPARK-31755][SQL] allow missing year/hour when parsing date/timestamp string
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #28576:
URL: https://github.com/apache/spark/pull/28576#issuecomment-630608038
**[Test build #122832 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/122832/testReport)** for PR 28576 at commit [`086eeb5`](https://github.com/apache/spark/commit/086eeb55364530c20ed453857faa16131f044d76).
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #28576: [SPARK-31755][SQL] allow missing year/hour when parsing date/timestamp string
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28576:
URL: https://github.com/apache/spark/pull/28576#issuecomment-631283001
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] bart-samwel commented on a change in pull request #28576: [SPARK-31755][SQL] allow missing year/hour when parsing date/timestamp string
Posted by GitBox <gi...@apache.org>.
bart-samwel commented on a change in pull request #28576:
URL: https://github.com/apache/spark/pull/28576#discussion_r427836822
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeFormatterHelper.scala
##########
@@ -31,17 +31,50 @@ import org.apache.spark.sql.internal.SQLConf
import org.apache.spark.sql.internal.SQLConf.LegacyBehaviorPolicy._
trait DateTimeFormatterHelper {
+ private def getOrDefault(accessor: TemporalAccessor, field: ChronoField, default: Int): Int = {
+ if (accessor.isSupported(field)) {
+ accessor.get(field)
+ } else {
+ default
+ }
+ }
+
+ protected def toLocalDate(accessor: TemporalAccessor, allowMissingYear: Boolean): LocalDate = {
+ val year = if (accessor.isSupported(ChronoField.YEAR)) {
+ accessor.get(ChronoField.YEAR)
+ } else if (allowMissingYear) {
+ // To keep backward compatibility with Spark 2.x, we pick 1970 as the default value of year.
+ 1970
+ } else {
+ throw new SparkUpgradeException("3.0",
+ "Year must be given in the date/timestamp string to be parsed. You can set " +
+ SQLConf.LEGACY_ALLOW_MISSING_YEAR_DURING_PARSING.key + " to true, to pick 1970 as " +
+ "the default value of year.", null)
+ }
+ val month = getOrDefault(accessor, ChronoField.MONTH_OF_YEAR, 1)
Review comment:
I'm saying it works (i.e., it accepts all valid values) but it's nonsensical; it'll never return the complete timestamp that people intended.
That said, I can see cases where people want to parse contiguous ranges (e.g. just a time, or just a month, or just a month + day) and then extract the parsed components using `EXTRACT` / `date_part`, or just to format them differently.
So I guess the most permissive but still safe thing here would be to forbid missing "year" only if someone also requests "day" and "month", because that's the situation where "year" affects the valid range of "day". But if they don't request year *and* they don't request month, then month would default to 1 and there would be no impact on the valid range of days, it would always go up to 32. I.e.:
`"yyyy MM dd"` -> OK
`"MM dd"` -> fail, need year to know valid range of `dd`
`"MM"` -> OK
`"dd"` -> OK
`"hh:mm:ss"` -> OK
`"MM dd hh:mm:ss"` -> fail, need year to know valid range of `dd`
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #28576: [SPARK-31755][SQL] allow missing year/hour when parsing date/timestamp string
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #28576:
URL: https://github.com/apache/spark/pull/28576#discussion_r427337528
##########
File path: sql/catalyst/src/test/scala/org/apache/spark/sql/util/TimestampFormatterSuite.scala
##########
@@ -291,4 +291,95 @@ class TimestampFormatterSuite extends SparkFunSuite with SQLHelper with Matchers
}
}
}
+
+ test("parsing hour with various patterns") {
+ def createFormatter(pattern: String): TimestampFormatter = {
+ // Use `SIMPLE_DATE_FORMAT`, so that the legacy parser also fails with invalid value range.
+ TimestampFormatter(pattern, ZoneOffset.UTC, LegacyDateFormats.SIMPLE_DATE_FORMAT, false)
+ }
+
+ withClue("HH") {
+ val formatter = createFormatter("yyyy-MM-dd HH")
+
+ val micros1 = formatter.parse("2009-12-12 00")
+ assert(micros1 === TimeUnit.SECONDS.toMicros(
+ LocalDateTime.of(2009, 12, 12, 0, 0, 0).toEpochSecond(ZoneOffset.UTC)))
+
+ val micros2 = formatter.parse("2009-12-12 15")
+ assert(micros2 === TimeUnit.SECONDS.toMicros(
+ LocalDateTime.of(2009, 12, 12, 15, 0, 0).toEpochSecond(ZoneOffset.UTC)))
Review comment:
It's UT, which is whitebox testing. It's clear from the code that the time parsing is unrelated to the value of year
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #28576: [SPARK-31755][SQL] allow missing year/hour when parsing date/timestamp string
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28576:
URL: https://github.com/apache/spark/pull/28576#issuecomment-632051200
**[Test build #122923 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/122923/testReport)** for PR 28576 at commit [`8de3bda`](https://github.com/apache/spark/commit/8de3bdae1038810409f5fa1bea9e700c058af21b).
* This patch **fails Spark unit tests**.
* This patch merges cleanly.
* This patch adds no public classes.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28576: [SPARK-31755][SQL] allow missing year/hour when parsing date/timestamp string
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28576:
URL: https://github.com/apache/spark/pull/28576#issuecomment-630608610
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28576: [SPARK-31755][SQL] allow missing year/hour when parsing date/timestamp string
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28576:
URL: https://github.com/apache/spark/pull/28576#issuecomment-632051500
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/122923/
Test FAILed.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] bart-samwel commented on pull request #28576: [SPARK-31755][SQL] allow missing year/hour when parsing date/timestamp string
Posted by GitBox <gi...@apache.org>.
bart-samwel commented on pull request #28576:
URL: https://github.com/apache/spark/pull/28576#issuecomment-630696349
Note that if we were designing this fresh, the year 1970 would not be a great choice when it would be used to parse arbitrary Month/Day combinations. The reason being that it is not a leap year, which means that it would never parse Feb 29.
This behavior seems to be pretty broken. We should consider forbidding missing things in the middle / from the top, i.e., you have to have a prefix of (year, month, day, hour, minute, second, millis/micros/nanos). If anybody wants that, they can simulate the defaults by prefixing with their own stuff, e.g. if you want to parse hh:mm:ss then you have to parse `to_timestamp(concat('1970-01-01 ', the_thing_we_want_to_parse, 'yyyy-MM-dd hh:mm:ss')`. We can even hint this in the error message if we would forbid this by default, with a legacy config that people can use as a fallback.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #28576: [SPARK-31755][SQL] allow missing year/hour when parsing date/timestamp string
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #28576:
URL: https://github.com/apache/spark/pull/28576#issuecomment-630958801
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28576: [SPARK-31755][SQL] allow missing year/hour when parsing date/timestamp string
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28576:
URL: https://github.com/apache/spark/pull/28576#issuecomment-630894202
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #28576: [SPARK-31755][SQL] allow missing year/hour when parsing date/timestamp string
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #28576:
URL: https://github.com/apache/spark/pull/28576#discussion_r427980379
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeFormatterHelper.scala
##########
@@ -31,17 +31,50 @@ import org.apache.spark.sql.internal.SQLConf
import org.apache.spark.sql.internal.SQLConf.LegacyBehaviorPolicy._
trait DateTimeFormatterHelper {
+ private def getOrDefault(accessor: TemporalAccessor, field: ChronoField, default: Int): Int = {
+ if (accessor.isSupported(field)) {
+ accessor.get(field)
+ } else {
+ default
+ }
+ }
+
+ protected def toLocalDate(accessor: TemporalAccessor, allowMissingYear: Boolean): LocalDate = {
+ val year = if (accessor.isSupported(ChronoField.YEAR)) {
+ accessor.get(ChronoField.YEAR)
+ } else if (allowMissingYear) {
+ // To keep backward compatibility with Spark 2.x, we pick 1970 as the default value of year.
+ 1970
+ } else {
+ throw new SparkUpgradeException("3.0",
+ "Year must be given in the date/timestamp string to be parsed. You can set " +
+ SQLConf.LEGACY_ALLOW_MISSING_YEAR_DURING_PARSING.key + " to true, to pick 1970 as " +
+ "the default value of year.", null)
+ }
+ val month = getOrDefault(accessor, ChronoField.MONTH_OF_YEAR, 1)
Review comment:
unfortunately datetime functions don't respect the ANSI flag at all. Maybe we should keep everything the same as Spark 2.4 (return null when parsing Feb 29 because 1970-02-29 is invalid), and implement ANSI behavior in Spark 3.1?
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA removed a comment on pull request #28576: [SPARK-31755][SQL] allow missing year/hour when parsing date/timestamp string
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #28576:
URL: https://github.com/apache/spark/pull/28576#issuecomment-631324288
**[Test build #122880 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/122880/testReport)** for PR 28576 at commit [`b17a648`](https://github.com/apache/spark/commit/b17a648bf31762f15cc414117308402649b92dde).
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] MaxGekk commented on a change in pull request #28576: [SPARK-31755][SQL] allow missing year/hour when parsing date/timestamp string
Posted by GitBox <gi...@apache.org>.
MaxGekk commented on a change in pull request #28576:
URL: https://github.com/apache/spark/pull/28576#discussion_r427115840
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeFormatterHelper.scala
##########
@@ -31,17 +31,39 @@ import org.apache.spark.sql.internal.SQLConf
import org.apache.spark.sql.internal.SQLConf.LegacyBehaviorPolicy._
trait DateTimeFormatterHelper {
+ private def getOrDefault(accessor: TemporalAccessor, field: ChronoField, default: Int): Int = {
+ if (accessor.isSupported(field)) {
+ accessor.get(field)
+ } else {
+ default
+ }
+ }
+
+ protected def toLocalDate(temporalAccessor: TemporalAccessor): LocalDate = {
+ val year = getOrDefault(temporalAccessor, ChronoField.YEAR, 1970)
+ val month = getOrDefault(temporalAccessor, ChronoField.MONTH_OF_YEAR, 1)
+ val day = getOrDefault(temporalAccessor, ChronoField.DAY_OF_MONTH, 1)
+ LocalDate.of(year, month, day)
+ }
+
// Converts the parsed temporal object to ZonedDateTime. It sets time components to zeros
// if they does not exist in the parsed object.
protected def toZonedDateTime(
temporalAccessor: TemporalAccessor,
zoneId: ZoneId): ZonedDateTime = {
- // Parsed input might not have time related part. In that case, time component is set to zeros.
- val parsedLocalTime = temporalAccessor.query(TemporalQueries.localTime)
- val localTime = if (parsedLocalTime == null) LocalTime.MIDNIGHT else parsedLocalTime
- // Parsed input must have date component. At least, year must present in temporalAccessor.
- val localDate = temporalAccessor.query(TemporalQueries.localDate)
-
+ val hour = if (temporalAccessor.isSupported(ChronoField.HOUR_OF_DAY)) {
+ temporalAccessor.get(ChronoField.HOUR_OF_DAY)
+ } else if (temporalAccessor.isSupported(ChronoField.HOUR_OF_AMPM)) {
+ // When we reach here, is mean am/pm is not specified. Here we assume it's am.
Review comment:
`is mean am/pm is not specified.` -> `it means that am/pm is not specified.`
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #28576: [SPARK-31755][SQL] allow missing year/hour when parsing date/timestamp string
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #28576:
URL: https://github.com/apache/spark/pull/28576#issuecomment-631427928
Merged build finished. Test FAILed.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #28576: [SPARK-31755][SQL] allow missing year/hour when parsing date/timestamp string
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #28576:
URL: https://github.com/apache/spark/pull/28576#issuecomment-630958614
**[Test build #122851 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/122851/testReport)** for PR 28576 at commit [`58443e2`](https://github.com/apache/spark/commit/58443e2d211a614c206ca16b657875209e5302e1).
* This patch **fails Spark unit tests**.
* This patch merges cleanly.
* This patch adds no public classes.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA removed a comment on pull request #28576: [SPARK-31755][SQL] allow missing year/hour when parsing date/timestamp string
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #28576:
URL: https://github.com/apache/spark/pull/28576#issuecomment-631286052
**[Test build #122877 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/122877/testReport)** for PR 28576 at commit [`afb070c`](https://github.com/apache/spark/commit/afb070c4d38f5520af46c069a15e97c7f5465174).
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org