[SPARK-50226][SQL] Correct MakeDTInterval and MakeYMInterval to catch Java exceptions #48773

gotocoding-DB · 2024-11-06T10:24:18Z

What changes were proposed in this pull request?

MakeDTInterval and MakeYMInterval do not catch Java exceptions in nullSafeEval like it does MakeInterval. So we making behavior similar.

Why are the changes needed?

To show to users readable nice error message.

Does this PR introduce any user-facing change?

Yes, I changed error message type in error-conditions.json => it's a behavioral change for clients who parse error message type or error message text.

How was this patch tested?

There already were few tests to check behavior, I just changed expected error type.

Was this patch authored or co-authored using generative AI tooling?

Yes, Copilot used.

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/intervalExpressions.scala

gotocoding-DB · 2024-11-06T11:22:57Z

Had a conversation with @mihailom-db.
We should use INTERVAL_ARITHMETIC_OVERFLOW, add 2 subclasses to it: with hint and without hint and use "without_hint" variation for Make[DT|YM]Interval.

mihailomilosevic2001 · 2024-11-06T11:23:10Z

Update title please. No spaces between tags and the name of the PR should be the same as the ticket on jira.

gotocoding-DB · 2024-11-06T11:24:30Z

Update title please. No spaces between tags and the name of the PR should be the same as the ticket on jira.

Done.

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/intervalExpressions.scala

…_OVERFLOW, it suggests disabling ANSI

common/utils/src/main/resources/error/error-conditions.json

...lyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/IntervalExpressionsSuite.scala

common/utils/src/main/resources/error/error-conditions.json

gotocoding-DB · 2024-11-08T09:59:14Z

CI fails, but testOnly org.apache.spark.sql.SparkSessionE2ESuite locally works fine (I've checked it 5 times, always green). Could it be useful just to restart CI (if yes – how to do it? I can just add 1 more commit with merging to fresh master, but maybe there is just a button "retry").

UPD Found that testOnly org.apache.spark.sql.SingleLevelAggregateHashMapSuite really fails, so it's not because of flaps.

…low-error

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/intervalExpressions.scala

sql/core/src/test/scala/org/apache/spark/sql/DataFrameAggregateSuite.scala

mihailomilosevic2001 · 2024-11-08T10:53:41Z

@srielau Does this look good to you?

mihailomilosevic2001 · 2024-11-08T10:54:29Z

You can remove [WIP] and Draft as well.

common/utils/src/main/resources/error/error-conditions.json

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/IntervalMathUtils.scala

common/utils/src/main/resources/error/error-conditions.json

sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryExecutionErrors.scala

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/IntervalUtils.scala

…low-error

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/IntervalMathUtils.scala

sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryExecutionErrors.scala

mihailomilosevic2001

LGTM except for few comments

…low-error

MaxGekk · 2024-11-13T15:29:40Z

+1, LGTM. Merging to master.
Thank you, @gotocoding-DB and @mihailom-db @srielau for review.

MaxGekk · 2024-11-13T15:31:22Z

@gotocoding-DB Congratulations with your first contribution to Apache Spark!

cloud-fan · 2024-11-14T04:09:59Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/intervalExpressions.scala

+      Math.toIntExact(
+        Math.addExact(month.asInstanceOf[Int],
+          Math.multiplyExact(year.asInstanceOf[Int], MONTHS_PER_YEAR)))
+    } catch {


shall we add IntervalUtils.makeYearMonthInterval so that we duplicate less code between interpreted and codegen?

Off course we should deduplicate it, but I didn't succeed with it: #48773 (comment).
Max suggested to use StaticInvoke, I'll try it in separate PR.

there is already makeDayTimeInterval and we can just follow it. Can you try it again and create a PR? We can help to check the code together.

Ok, I'll do it a bit later.
UPD: #48848

Throw ARITHMETIC_OVERFLOW in MakeYMInterval, MakeDTInterval

3426c34

github-actions bot added the SQL label Nov 6, 2024

gotocoding-DB changed the title ~~[WIP] [SPARK-50226] Throw ARITHMETIC_OVERFLOW in MakeYMInterval and MakeDTInterval~~ [WIP] [SPARK-50226] [SQL] Throw ARITHMETIC_OVERFLOW in MakeYMInterval and MakeDTInterval Nov 6, 2024

gotocoding-DB commented Nov 6, 2024

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/intervalExpressions.scala Outdated Show resolved Hide resolved

gotocoding-DB changed the title ~~[WIP] [SPARK-50226] [SQL] Throw ARITHMETIC_OVERFLOW in MakeYMInterval and MakeDTInterval~~ [WIP][SPARK-50226][SQL] Correct MakeDTInterval and MakeYMInterval to catch Java exceptions Nov 6, 2024

gotocoding-DB marked this pull request as draft November 6, 2024 11:29

MaxGekk reviewed Nov 6, 2024

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/intervalExpressions.scala Outdated Show resolved Hide resolved

Add another type of exception because we can't use current ARITHMETIC…

908e745

…_OVERFLOW, it suggests disabling ANSI

gotocoding-DB commented Nov 6, 2024

View reviewed changes

common/utils/src/main/resources/error/error-conditions.json Outdated Show resolved Hide resolved

gotocoding-DB commented Nov 6, 2024

View reviewed changes

common/utils/src/main/resources/error/error-conditions.json Outdated Show resolved Hide resolved

mihailomilosevic2001 reviewed Nov 7, 2024

View reviewed changes

...lyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/IntervalExpressionsSuite.scala Outdated Show resolved Hide resolved

better error message

b5cd32a

mihailomilosevic2001 reviewed Nov 7, 2024

View reviewed changes

common/utils/src/main/resources/error/error-conditions.json Outdated Show resolved Hide resolved

mihailomilosevic2001 reviewed Nov 7, 2024

View reviewed changes

common/utils/src/main/resources/error/error-conditions.json Outdated Show resolved Hide resolved

gotocoding-DB added 3 commits November 7, 2024 16:30

Merge branch 'master' into SPARK-50226-overflow-error

2fa9f9d

fix styletest for error-conditions.json

abd4255

WITH_TRY_SUGGESTION without message

de44e30

gotocoding-DB added 2 commits November 8, 2024 11:30

Merge remote-tracking branch 'upstream/master' into SPARK-50226-overf…

87e3c5c

…low-error

fix tests

3b94f96

MaxGekk reviewed Nov 8, 2024

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/intervalExpressions.scala Outdated Show resolved Hide resolved

MaxGekk reviewed Nov 8, 2024

View reviewed changes

sql/core/src/test/scala/org/apache/spark/sql/DataFrameAggregateSuite.scala Outdated Show resolved Hide resolved

Better tests, return back message field because of 'Divide by -1' part

d7352a4

gotocoding-DB force-pushed the SPARK-50226-overflow-error branch from 98c4d11 to d7352a4 Compare November 8, 2024 12:52

mihailomilosevic2001 reviewed Nov 8, 2024

View reviewed changes

common/utils/src/main/resources/error/error-conditions.json Outdated Show resolved Hide resolved

Regenerate expected sql output

68594ff

gotocoding-DB commented Nov 8, 2024

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/IntervalMathUtils.scala Outdated Show resolved Hide resolved

gotocoding-DB added 2 commits November 8, 2024 17:38

Fix tests

30c63f6

Enforce setting context

8388de2

gotocoding-DB changed the title ~~[WIP][SPARK-50226][SQL] Correct MakeDTInterval and MakeYMInterval to catch Java exceptions~~ [SPARK-50226][SQL] Correct MakeDTInterval and MakeYMInterval to catch Java exceptions Nov 8, 2024

gotocoding-DB marked this pull request as ready for review November 8, 2024 18:08

MaxGekk reviewed Nov 8, 2024

View reviewed changes

common/utils/src/main/resources/error/error-conditions.json Show resolved Hide resolved

MaxGekk reviewed Nov 8, 2024

View reviewed changes

common/utils/src/main/resources/error/error-conditions.json Outdated Show resolved Hide resolved

MaxGekk reviewed Nov 8, 2024

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryExecutionErrors.scala Outdated Show resolved Hide resolved

MaxGekk reviewed Nov 8, 2024

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/IntervalUtils.scala Outdated Show resolved Hide resolved

srielau approved these changes Nov 11, 2024

View reviewed changes

gotocoding-DB added 7 commits November 12, 2024 12:23

remove TRY

4f72f7e

Merge remote-tracking branch 'upstream/master' into SPARK-50226-overf…

9ed5eb2

…low-error

fix PR issues

494d648

recanonize tests

b042247

fix tests

0fcd3cc

add err context

2925363

recanonize tests

2d84c95

mihailomilosevic2001 reviewed Nov 13, 2024

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/IntervalMathUtils.scala Show resolved Hide resolved

mihailomilosevic2001 reviewed Nov 13, 2024

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryExecutionErrors.scala Outdated Show resolved Hide resolved

mihailomilosevic2001 approved these changes Nov 13, 2024

View reviewed changes

gotocoding-DB added 2 commits November 13, 2024 11:02

Merge remote-tracking branch 'upstream/master' into SPARK-50226-overf…

c2d2490

…low-error

fix PR issues

2a36749

MaxGekk approved these changes Nov 13, 2024

View reviewed changes

MaxGekk closed this in bd94419 Nov 13, 2024

gotocoding-DB deleted the SPARK-50226-overflow-error branch November 13, 2024 17:46

cloud-fan reviewed Nov 14, 2024

View reviewed changes

[SPARK-50226][SQL] Correct MakeDTInterval and MakeYMInterval to catch Java exceptions #48773

[SPARK-50226][SQL] Correct MakeDTInterval and MakeYMInterval to catch Java exceptions #48773

Uh oh!

Conversation

gotocoding-DB commented Nov 6, 2024 • 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?

Was this patch authored or co-authored using generative AI tooling?

Uh oh!

Uh oh!

gotocoding-DB commented Nov 6, 2024

Uh oh!

mihailomilosevic2001 commented Nov 6, 2024

Uh oh!

gotocoding-DB commented Nov 6, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gotocoding-DB commented Nov 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mihailomilosevic2001 commented Nov 8, 2024

Uh oh!

mihailomilosevic2001 commented Nov 8, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mihailomilosevic2001 left a comment

Choose a reason for hiding this comment

Uh oh!

MaxGekk commented Nov 13, 2024

Uh oh!

MaxGekk commented Nov 13, 2024

Uh oh!

cloud-fan Nov 14, 2024

Choose a reason for hiding this comment

Uh oh!

gotocoding-DB Nov 14, 2024

Choose a reason for hiding this comment

Uh oh!

cloud-fan Nov 14, 2024

Choose a reason for hiding this comment

Uh oh!

gotocoding-DB Nov 14, 2024 • 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

gotocoding-DB commented Nov 6, 2024 •

edited

Loading

gotocoding-DB commented Nov 8, 2024 •

edited

Loading

gotocoding-DB Nov 14, 2024 •

edited

Loading