-
Notifications
You must be signed in to change notification settings - Fork 29.3k
[SPARK-31879][SQL][test-java11] Make week-based pattern invalid for formatting too #28728
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 3 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -234,22 +234,22 @@ private object DateTimeFormatterHelper { | |
| val formatter = DateTimeFormatter.ofPattern("LLL qqq", Locale.US) | ||
| formatter.format(LocalDate.of(2000, 1, 1)) == "1 1" | ||
| } | ||
| final val unsupportedLetters = Set('A', 'c', 'e', 'n', 'N', 'p') | ||
| final val unsupportedLetters = Set('A', 'c', 'n', 'N', 'p', 'Y', 'W', 'w', 'u') | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. how about we add a new list |
||
| // SPARK-31892: The week-based date fields are rarely used and really confusing for parsing values | ||
| // to datetime, especially when they are mixed with other non-week-based ones | ||
| // The quarter fields will also be parsed strangely, e.g. when the pattern contains `yMd` and can | ||
| // be directly resolved then the `q` do check for whether the month is valid, but if the date | ||
| // fields is incomplete, e.g. `yM`, the checking will be bypassed. | ||
| final val unsupportedLettersForParsing = Set('Y', 'W', 'w', 'E', 'u', 'F', 'q', 'Q') | ||
| final val unsupportedLettersForParsing = Set('E', 'e', 'F', 'q', 'Q') | ||
| final val unsupportedPatternLengths = { | ||
| // SPARK-31771: Disable Narrow-form TextStyle to avoid silent data change, as it is Full-form in | ||
| // 2.4 | ||
| Seq("G", "M", "L", "E", "u", "Q", "q").map(_ * 5) ++ | ||
| Seq("G", "M", "L", "E", "e", "Q", "q").map(_ * 5) ++ | ||
| // SPARK-31867: Disable year pattern longer than 10 which will cause Java time library throw | ||
| // unchecked `ArrayIndexOutOfBoundsException` by the `NumberPrinterParser` for formatting. It | ||
| // makes the call side difficult to handle exceptions and easily leads to silent data change | ||
| // because of the exceptions being suppressed. | ||
| Seq("y", "Y").map(_ * 11) | ||
| Seq("y").map(_ * 11) | ||
| }.toSet | ||
|
|
||
| /** | ||
|
|
@@ -282,20 +282,13 @@ private object DateTimeFormatterHelper { | |
| "or upgrade your Java version. For more details, please read " + | ||
| "https://bugs.openjdk.java.net/browse/JDK-8114833") | ||
| } | ||
| // The meaning of 'u' was day number of week in SimpleDateFormat, it was changed to year | ||
| // in DateTimeFormatter. Substitute 'u' to 'e' and use DateTimeFormatter to parse the | ||
| // string. If parsable, return the result; otherwise, fall back to 'u', and then use the | ||
| // legacy SimpleDateFormat parser to parse. When it is successfully parsed, throw an | ||
| // exception and ask users to change the pattern strings or turn on the legacy mode; | ||
| // otherwise, return NULL as what Spark 2.4 does. | ||
| val res = patternPart.replace("u", "e") | ||
| // In DateTimeFormatter, 'u' supports negative years. We substitute 'y' to 'u' here for | ||
| // keeping the support in Spark 3.0. If parse failed in Spark 3.0, fall back to 'y'. | ||
| // We only do this substitution when there is no era designator found in the pattern. | ||
| if (!eraDesignatorContained) { | ||
| res.replace("y", "u") | ||
| patternPart.replace("y", "u") | ||
| } else { | ||
| res | ||
| patternPart | ||
| } | ||
| } else { | ||
| patternPart | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,19 +15,17 @@ | |
| * limitations under the License. | ||
| */ | ||
|
|
||
| package org.apache.spark.sql.util | ||
| package org.apache.spark.sql.catalyst.util | ||
|
|
||
| import java.time.{DateTimeException, LocalDate} | ||
|
|
||
| import org.apache.spark.{SparkFunSuite, SparkUpgradeException} | ||
| import org.apache.spark.sql.catalyst.plans.SQLHelper | ||
| import org.apache.spark.sql.catalyst.util.{DateFormatter, LegacyDateFormats} | ||
| import org.apache.spark.SparkUpgradeException | ||
| import org.apache.spark.sql.catalyst.util.DateTimeTestUtils._ | ||
| import org.apache.spark.sql.catalyst.util.DateTimeUtils._ | ||
| import org.apache.spark.sql.internal.SQLConf | ||
| import org.apache.spark.sql.internal.SQLConf.LegacyBehaviorPolicy | ||
|
|
||
| class DateFormatterSuite extends SparkFunSuite with SQLHelper { | ||
| class DateFormatterSuite extends DatetimeFormatterSuite { | ||
| test("parsing dates") { | ||
| outstandingTimezonesIds.foreach { timeZone => | ||
| withSQLConf(SQLConf.SESSION_LOCAL_TIMEZONE.key -> timeZone) { | ||
|
|
@@ -199,4 +197,8 @@ class DateFormatterSuite extends SparkFunSuite with SQLHelper { | |
| // SparkUpgradeException here. | ||
| intercept[SparkUpgradeException](formatter.parse("02-29")) | ||
| } | ||
|
|
||
| override def checkFormatterCreation(pattern: String, isParsing: Boolean): Unit = { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can we move it to the beginning of this class? to avoid code conflict when we add new tests |
||
| DateFormatter(pattern, UTC, isParsing) | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,53 @@ | ||
| /* | ||
| * Licensed to the Apache Software Foundation (ASF) under one or more | ||
| * contributor license agreements. See the NOTICE file distributed with | ||
| * this work for additional information regarding copyright ownership. | ||
| * The ASF licenses this file to You under the Apache License, Version 2.0 | ||
| * (the "License"); you may not use this file except in compliance with | ||
| * the License. You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
|
|
||
| package org.apache.spark.sql.catalyst.util | ||
|
|
||
| import org.scalatest.Matchers | ||
|
|
||
| import org.apache.spark.{SparkFunSuite, SparkUpgradeException} | ||
| import org.apache.spark.sql.catalyst.plans.SQLHelper | ||
|
|
||
| trait DatetimeFormatterSuite extends SparkFunSuite with SQLHelper with Matchers { | ||
| import DateTimeFormatterHelper._ | ||
| def checkFormatterCreation(pattern: String, isParsing: Boolean): Unit | ||
|
|
||
| test("explicitly forbidden datetime patterns") { | ||
|
|
||
| Seq(true, false).foreach { isParsing => | ||
| // not support by the legacy one too | ||
| val unsupportedBoth = Seq("QQQQQ", "qqqqq", "eeeee", "A", "c", "n", "N", "p") | ||
| unsupportedBoth.foreach { pattern => | ||
| intercept[IllegalArgumentException](checkFormatterCreation(pattern, isParsing)) | ||
| } | ||
| // supported by the legacy one, then we will suggest users with SparkUpgradeException | ||
| (unsupportedLetters.map(_.toString) ++ unsupportedPatternLengths -- unsupportedBoth).foreach { | ||
| pattern => intercept[SparkUpgradeException](checkFormatterCreation(pattern, isParsing)) | ||
| } | ||
| } | ||
|
|
||
| // not support by the legacy one too | ||
| val unsupportedBoth = Seq("e", "q", "Q") | ||
| unsupportedBoth.foreach { pattern => | ||
| intercept[IllegalArgumentException](checkFormatterCreation(pattern, true)) | ||
| } | ||
| // supported by the legacy one, then we will suggest users with SparkUpgradeException | ||
| (unsupportedLettersForParsing.map(_.toString) -- unsupportedBoth).foreach { | ||
| pattern => intercept[SparkUpgradeException](checkFormatterCreation(pattern, true)) | ||
| } | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,7 +15,7 @@ | |
| * limitations under the License. | ||
| */ | ||
|
|
||
| package org.apache.spark.sql.util | ||
| package org.apache.spark.sql.catalyst.util | ||
|
|
||
| import java.time.{DateTimeException, Instant, LocalDateTime, LocalTime} | ||
| import java.util.concurrent.TimeUnit | ||
|
|
@@ -24,14 +24,13 @@ import org.scalatest.Matchers | |
|
|
||
| import org.apache.spark.{SparkFunSuite, SparkUpgradeException} | ||
| import org.apache.spark.sql.catalyst.plans.SQLHelper | ||
| import org.apache.spark.sql.catalyst.util.{LegacyDateFormats, TimestampFormatter} | ||
| import org.apache.spark.sql.catalyst.util.DateTimeTestUtils._ | ||
| import org.apache.spark.sql.catalyst.util.DateTimeUtils._ | ||
| import org.apache.spark.sql.internal.SQLConf | ||
| import org.apache.spark.sql.internal.SQLConf.LegacyBehaviorPolicy | ||
| import org.apache.spark.unsafe.types.UTF8String | ||
|
|
||
| class TimestampFormatterSuite extends SparkFunSuite with SQLHelper with Matchers { | ||
| class TimestampFormatterSuite extends DatetimeFormatterSuite { | ||
|
|
||
| test("parsing timestamps using time zones") { | ||
| val localDate = "2018-12-02T10:11:12.001234" | ||
|
|
@@ -418,15 +417,7 @@ class TimestampFormatterSuite extends SparkFunSuite with SQLHelper with Matchers | |
| assert(t5 === date(1970)) | ||
| } | ||
|
|
||
| test("explicitly forbidden datetime patterns") { | ||
| // not support by the legacy one too | ||
| Seq("QQQQQ", "qqqqq", "A", "c", "e", "n", "N", "p").foreach { pattern => | ||
| intercept[IllegalArgumentException](TimestampFormatter(pattern, UTC).format(0)) | ||
| } | ||
| // supported by the legacy one, then we will suggest users with SparkUpgradeException | ||
| Seq("GGGGG", "MMMMM", "LLLLL", "EEEEE", "uuuuu", "aa", "aaa", "y" * 11, "y" * 11) | ||
| .foreach { pattern => | ||
| intercept[SparkUpgradeException](TimestampFormatter(pattern, UTC).format(0)) | ||
| } | ||
| override def checkFormatterCreation(pattern: String, isParsing: Boolean): Unit = { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ditto |
||
| TimestampFormatter(pattern, UTC, isParsing) | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shall we forbid
eas well? It's not supported in 2.4 either.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Forgot to update the doc, thanks for reminding