[SPARK-36970][SQL] Manual disabled format `B` of `date_format` function to make Java 17 compatible with Java 8 by LuciferYang · Pull Request #34237 · apache/spark

LuciferYang · 2021-10-11T05:02:32Z

What changes were proposed in this pull request?

The date_format function with B format has different behavior when use Java 8 and Java 17, select date_format('2018-11-17 13:33:33.333', 'B') in datetime-formatting-invalid.sql can prove this.

The case result with Java 8 is

-- !query
select date_format('2018-11-17 13:33:33.333', 'B')
-- !query schema
struct<>
-- !query output
java.lang.IllegalArgumentException
Unknown pattern letter: B

and the case result with Java 17 is

- datetime-formatting-invalid.sql *** FAILED ***
datetime-formatting-invalid.sql
Expected "struct<[]>", but got "struct<[date_format(2018-11-17 13:33:33.333, B):string]>" Schema did not match for query #34
select date_format('2018-11-17 13:33:33.333', 'B'): -- !query
select date_format('2018-11-17 13:33:33.333', 'B')
-- !query schema
struct<date_format(2018-11-17 13:33:33.333, B):string>
-- !query output
in the afternoon (SQLQueryTestSuite.scala:469)

We found that this is due to the new support of format B in Java 17

'B' is used to represent Pattern letters to output a day period in Java 17
* Pattern Count Equivalent builder methods
* ------- ----- --------------------------
* B 1 appendDayPeriodText(TextStyle.SHORT)
* BBBB 4 appendDayPeriodText(TextStyle.FULL)
* BBBBB 5 appendDayPeriodText(TextStyle.NARROW)

And through http://spark.apache.org/docs/latest/sql-ref-datetime-pattern.html , we can confirm that format B is not documented/supported for date_format function currently.

So the main change of this pr is manual disabled format B of date_format function in DateTimeFormatterHelper to make Java 17 compatible with Java 8.

Why are the changes needed?

Ensure that Java 17 and Java 8 have the same behavior.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Pass the Jenkins or GitHub Action
Manual test SQLQueryTestSuite with JDK 17

Before

- datetime-formatting-invalid.sql *** FAILED ***
datetime-formatting-invalid.sql
Expected "struct<[]>", but got "struct<[date_format(2018-11-17 13:33:33.333, B):string]>" Schema did not match for query #34
select date_format('2018-11-17 13:33:33.333', 'B'): -- !query
select date_format('2018-11-17 13:33:33.333', 'B')
-- !query schema
struct<date_format(2018-11-17 13:33:33.333, B):string>
-- !query output
in the afternoon (SQLQueryTestSuite.scala:469)

After
The test select date_format('2018-11-17 13:33:33.333', 'B') in datetime-formatting-invalid.sql passed

LuciferYang · 2021-10-11T05:10:19Z

cc @srowen @MaxGekk @dongjoon-hyun

SparkQA · 2021-10-11T05:50:12Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/48544/

SparkQA · 2021-10-11T06:34:35Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/48544/

MaxGekk · 2021-10-11T07:44:27Z

 // 2.4, the SimpleDateFormat uses Monday as the first day of week.
 final val weekBasedLetters = Set('Y', 'W', 'w', 'u', 'e', 'c')
 final val unsupportedLetters = Set('A', 'n', 'N', 'p')
+ final val unknownPatternLetters: Set[Char] = Set('B')


What's the difference between unsupported and unknown letters from your point of view?

@MaxGekk Both unsupportedLetters and unknownPatternLetters are used for explicitly banned some pattern letters and throw IllegalArgumentException
The only difference is the content of the error message, throw IllegalArgumentException with Unknown pattern letter: $c Is the current behavior.
spark/sql/core/src/test/resources/sql-tests/results/datetime-formatting-invalid.sql.out
Lines 311 to 317 in 32e11ea
-- !query
select date_format('2018-11-17 13:33:33.333', 'B')
-- !query schema
struct<>
-- !query output
java.lang.IllegalArgumentException
Unknown pattern letter: B
If we accept the change of error message content, we can reuse unsupportedLetters.

If we accept the change of error message content ...
I think it is ok. Let's put 'B' to unsupportedLetters. BTW, should we standardise the exception by following https://issues.apache.org/jira/browse/SPARK-33539, cc @allisonwang-db ?

Awesome thanks!

SparkQA · 2021-10-11T08:50:27Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/48550/

SparkQA · 2021-10-11T09:33:55Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/48550/

SparkQA · 2021-10-11T10:05:45Z

Test build #144066 has finished for PR 34237 at commit 1bc02ad.

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

SparkQA · 2021-10-11T10:56:02Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/48556/

SparkQA · 2021-10-11T11:53:51Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/48556/

SparkQA · 2021-10-11T13:00:35Z

Test build #144072 has finished for PR 34237 at commit c6713ce.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class SetCatalogAndNamespace(child: LogicalPlan) extends UnaryCommand
case class SetNamespaceCommand(namespace: Seq[String]) extends LeafRunnableCommand

SparkQA · 2021-10-11T14:57:14Z

Test build #144078 has finished for PR 34237 at commit 8271b47.

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

dongjoon-hyun

+1, LGTM. Thank you, @LuciferYang , @srowen , @MaxGekk .
Merged to master for Apache Spark 3.3.

dongjoon-hyun · 2021-10-11T18:33:25Z

Oh, I'm taking my words back. It seems that there is a TPCDS test failure still.

dongjoon-hyun · 2021-10-11T18:34:10Z

Could you re-trigger GitHub Action once more, @LuciferYang ?

LuciferYang · 2021-10-12T02:28:33Z

Could you re-trigger GitHub Action once more, @LuciferYang ?

OK, the exit code is 137. It looks like it was killed

LuciferYang · 2021-10-12T03:03:01Z

@dongjoon-hyun

TPCDS passed although other tests are still running

MaxGekk · 2021-10-12T08:01:29Z

@LuciferYang Could you re-trigger GAs via an empty commit:

$ git commit --allow-empty -m "Trigger build"

SparkQA · 2021-10-12T10:19:14Z

Test build #144143 has finished for PR 34237 at commit 0ad1959.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
protected class YarnSchedulerEndpoint(override val rpcEnv: RpcEnv)
case class HashedRelationBroadcastMode(key: Seq[Expression], isNullAware: Boolean = false)

SparkQA · 2021-10-12T10:59:31Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/48620/

SparkQA · 2021-10-12T11:42:45Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/48620/

SparkQA · 2021-10-12T12:59:39Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/48630/

SparkQA · 2021-10-12T13:00:15Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/48628/

SparkQA · 2021-10-12T13:56:11Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/48628/

SparkQA · 2021-10-12T13:56:40Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/48630/

LuciferYang · 2021-10-12T14:58:36Z

Already re-trigger GAs many times，it seems that I need to investigate the reasons for the failure of hive SlowTest

SparkQA · 2021-10-12T17:07:33Z

Test build #144152 has finished for PR 34237 at commit ac2e700.

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

MaxGekk · 2021-10-12T17:13:18Z

+1, LGTM. Merging to master. All GAs passed

Thank you, @LuciferYang and @srowen @dongjoon-hyun @allisonwang-db for review.

dongjoon-hyun · 2021-10-12T23:44:48Z

Thank you all. :)

LuciferYang · 2021-10-13T02:16:29Z

thank all ~

Manual disabled format B
1bc02ad

github-actionsBot added the SQL label Oct 11, 2021

Merge branch 'upmaster' into SPARK-36970
c6713ce

MaxGekk reviewed Oct 11, 2021
View reviewed changes

LuciferYang added 2 commits October 11, 2021 18:01

use unsupportedLetters
df5ec7e

fix case
8271b47

srowen approved these changes Oct 11, 2021
View reviewed changes

MaxGekk approved these changes Oct 11, 2021
View reviewed changes

dongjoon-hyun approved these changes Oct 11, 2021
View reviewed changes

Merge branch 'upmaster' into SPARK-36970
0ad1959

LuciferYang added 2 commits October 12, 2021 19:26

Trigger build
e274c7c

Trigger build
ac2e700

MaxGekk closed this in 1af7072Oct 12, 2021

LuciferYang deleted the SPARK-36970 branch October 22, 2023 07:43

	-- !query
	select date_format('2018-11-17 13:33:33.333', 'B')
	-- !query schema
	struct<>
	-- !query output
	java.lang.IllegalArgumentException
	Unknown pattern letter: B

Uh oh!

Conversation

LuciferYang commented Oct 11, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

LuciferYang commented Oct 11, 2021

Uh oh!

SparkQA commented Oct 11, 2021

Uh oh!

SparkQA commented Oct 11, 2021

Uh oh!

MaxGekkOct 11, 2021

Choose a reason for hiding this comment

Uh oh!

LuciferYangOct 11, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MaxGekkOct 11, 2021

Choose a reason for hiding this comment

Uh oh!

LuciferYangOct 11, 2021

Choose a reason for hiding this comment

Uh oh!

allisonwang-dbOct 11, 2021

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Oct 11, 2021

Uh oh!

SparkQA commented Oct 11, 2021

Uh oh!

SparkQA commented Oct 11, 2021

Uh oh!

SparkQA commented Oct 11, 2021

Uh oh!

SparkQA commented Oct 11, 2021

Uh oh!

SparkQA commented Oct 11, 2021

Uh oh!

SparkQA commented Oct 11, 2021

Uh oh!

dongjoon-hyun left a comment

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun commented Oct 11, 2021

Uh oh!

dongjoon-hyun commented Oct 11, 2021

Uh oh!

LuciferYang commented Oct 12, 2021

Uh oh!

LuciferYang commented Oct 12, 2021

Uh oh!

MaxGekk commented Oct 12, 2021

Uh oh!

SparkQA commented Oct 12, 2021

Uh oh!

SparkQA commented Oct 12, 2021

Uh oh!

SparkQA commented Oct 12, 2021

Uh oh!

SparkQA commented Oct 12, 2021

Uh oh!

SparkQA commented Oct 12, 2021

Uh oh!

SparkQA commented Oct 12, 2021

Uh oh!

SparkQA commented Oct 12, 2021

Uh oh!

LuciferYang commented Oct 12, 2021

Uh oh!

SparkQA commented Oct 12, 2021

Uh oh!

MaxGekk commented Oct 12, 2021

Uh oh!

dongjoon-hyun commented Oct 12, 2021

Uh oh!

LuciferYang commented Oct 11, 2021•
edited
Loading

LuciferYangOct 11, 2021•
edited
Loading