[SPARK-38112][SQL] Use error classes in the execution errors of date/timestamp handling by ivoson · Pull Request #35670 · apache/spark

ivoson · 2022-02-27T14:46:02Z

What changes were proposed in this pull request?

Migrate the following errors in QueryExecutionErrors onto use error classes:

sparkUpgradeInReadingDatesError => INCONSISTENT_BEHAVIOR_CROSS_VERSION
sparkUpgradeInWritingDatesError => INCONSISTENT_BEHAVIOR_CROSS_VERSION
timeZoneIdNotSpecifiedForTimestampTypeError => UNSUPPORTED_OPERATION
cannotConvertOrcTimestampToTimestampNTZError => UNSUPPORTED_OPERATION

Why are the changes needed?

Porting date/timestamp execute errors to new error framework.

Does this PR introduce any user-facing change?

No

How was this patch tested?

UT added.

HyukjinKwon · 2022-02-28T00:55:38Z

cc @MaxGekk FYI

MaxGekk · 2022-02-28T09:34:39Z

core/src/main/scala/org/apache/spark/SparkException.scala

+  def this(version: String, message: String, cause: Throwable) =
+    this (
+      version = version,
+      message = s"You may get a different result due to the upgrading of Spark $version: $message",


Let's avoid error message duplication. error-classes.json should be one source of truth:

"INCONSISTENT_BEHAVIOR_CROSS_VERSION" : { "message" : [ "You may get a different result due to the upgrading of Spark %s: %s" ] },

Thanks @MaxGekk. Will fix this.

MaxGekk · 2022-02-28T09:39:44Z

sql/core/src/test/scala/org/apache/spark/sql/errors/QueryExecutionErrorsSuite.scala

+
+      assert(e.getErrorClass === "INCONSISTENT_BEHAVIOR_CROSS_VERSION")
+      assert(e.getMessage
+        .startsWith("You may get a different result due to the upgrading of Spark 3.0: \n" +


The message confuses slightly. We run Spark 3.3.0-SNAPSHOT (almost 3.3) and try to read a file written by Spark 2.4 but the message says: due to the upgrading of Spark 3.0.

Should be somehow: ... due to upgrading to Spark >= 3.0?

Spark >= 3.0 sounds good. Will address in next iteration.

AmplabJenkins · 2022-03-01T07:38:43Z

Can one of the admins verify this patch?

ivoson · 2022-03-06T15:00:26Z

Hey @MaxGekk . Comments addressed, please take a look when you have time. Thanks.

MaxGekk · 2022-03-07T16:04:31Z

sql/core/src/test/scala/org/apache/spark/sql/errors/QueryExecutionErrorsSuite.scala

+
+      assert(e.getErrorClass === "INCONSISTENT_BEHAVIOR_CROSS_VERSION")
+      assert(e.getMessage
+        .startsWith("You may get a different result due to the upgrading to Spark >= 3.0: \n" +


Could you check entire error message, please. See the motivation for that in PR's description #35416

MaxGekk

LGTM except of a comment.

MaxGekk · 2022-03-08T07:20:56Z

@ivoson Could you fix coding style:

[error] /home/runner/work/spark/spark/sql/core/src/test/scala/org/apache/spark/sql/errors/QueryExecutionErrorsSuite.scala:215: File line length exceeds 100 characters
[error] /home/runner/work/spark/spark/sql/core/src/test/scala/org/apache/spark/sql/errors/QueryExecutionErrorsSuite.scala:237: File line length exceeds 100 characters
[error] /home/runner/work/spark/spark/sql/core/src/test/scala/org/apache/spark/sql/errors/QueryExecutionErrorsSuite.scala:238: File line length exceeds 100 characters
[error] /home/runner/work/spark/spark/sql/core/src/test/scala/org/apache/spark/sql/errors/QueryExecutionErrorsSuite.scala:239: File line length exceeds 100 characters
[error] /home/runner/work/spark/spark/sql/core/src/test/scala/org/apache/spark/sql/errors/QueryExecutionErrorsSuite.scala:240: File line length exceeds 100 characters
[error] /home/runner/work/spark/spark/sql/core/src/test/scala/org/apache/spark/sql/errors/QueryExecutionErrorsSuite.scala:241: File line length exceeds 100 characters
[error] /home/runner/work/spark/spark/sql/core/src/test/scala/org/apache/spark/sql/errors/QueryExecutionErrorsSuite.scala:242: File line length exceeds 100 characters

ivoson · 2022-03-08T09:05:18Z

@ivoson Could you fix coding style:

[error] /home/runner/work/spark/spark/sql/core/src/test/scala/org/apache/spark/sql/errors/QueryExecutionErrorsSuite.scala:215: File line length exceeds 100 characters
[error] /home/runner/work/spark/spark/sql/core/src/test/scala/org/apache/spark/sql/errors/QueryExecutionErrorsSuite.scala:237: File line length exceeds 100 characters
[error] /home/runner/work/spark/spark/sql/core/src/test/scala/org/apache/spark/sql/errors/QueryExecutionErrorsSuite.scala:238: File line length exceeds 100 characters
[error] /home/runner/work/spark/spark/sql/core/src/test/scala/org/apache/spark/sql/errors/QueryExecutionErrorsSuite.scala:239: File line length exceeds 100 characters
[error] /home/runner/work/spark/spark/sql/core/src/test/scala/org/apache/spark/sql/errors/QueryExecutionErrorsSuite.scala:240: File line length exceeds 100 characters
[error] /home/runner/work/spark/spark/sql/core/src/test/scala/org/apache/spark/sql/errors/QueryExecutionErrorsSuite.scala:241: File line length exceeds 100 characters
[error] /home/runner/work/spark/spark/sql/core/src/test/scala/org/apache/spark/sql/errors/QueryExecutionErrorsSuite.scala:242: File line length exceeds 100 characters

Thanks @MaxGekk . Fixed.

MaxGekk · 2022-03-08T15:14:41Z

+1, LGTM. Merging to master.
Thank you, @ivoson.

ivoson · 2022-03-09T01:02:53Z

Thanks for the review. @MaxGekk

Use error classes in the execution errors of date/timestamp handling

1f816f1

github-actions bot added CORE SQL labels Feb 27, 2022

fix ut

ca46610

MaxGekk reviewed Feb 28, 2022

View reviewed changes

ivoson added 3 commits March 6, 2022 13:42

address comments

cfd6052

fix err msg

9730b12

Merge branch 'master' into SPARK-38112-Rebase

e58052d

MaxGekk reviewed Mar 7, 2022

View reviewed changes

Address comments, check the whole error message

d51c6ab

fix codestyle issue

d7795f9

MaxGekk approved these changes Mar 8, 2022

View reviewed changes

MaxGekk closed this in 8a0b101 Mar 8, 2022

ivoson deleted the SPARK-38112-Rebase branch March 9, 2022 01:02

Conversation

ivoson commented Feb 27, 2022 • edited by MaxGekk 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!

HyukjinKwon commented Feb 28, 2022

Uh oh!

MaxGekk Feb 28, 2022

Choose a reason for hiding this comment

Uh oh!

ivoson Mar 2, 2022

Choose a reason for hiding this comment

Uh oh!

MaxGekk Feb 28, 2022

Choose a reason for hiding this comment

Uh oh!

ivoson Mar 2, 2022

Choose a reason for hiding this comment

Uh oh!

AmplabJenkins commented Mar 1, 2022

Uh oh!

ivoson commented Mar 6, 2022

Uh oh!

MaxGekk Mar 7, 2022

Choose a reason for hiding this comment

Uh oh!

MaxGekk left a comment

Choose a reason for hiding this comment

Uh oh!

MaxGekk commented Mar 8, 2022

Uh oh!

ivoson commented Mar 8, 2022

Uh oh!

MaxGekk commented Mar 8, 2022

Uh oh!

ivoson commented Mar 9, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ivoson commented Feb 27, 2022 •

edited by MaxGekk

Loading