[SPARK-42307][SQL] Assign name for error _LEGACY_ERROR_TEMP_2232 by junyuc25 · Pull Request #47354 · apache/spark

junyuc25 · 2024-07-15T10:39:00Z

What changes were proposed in this pull request?

In this PR, I propose to replace the legacy error name _LEGACY_ERROR_TEMP_2232 with ROW_VALUE_IS_NULL , and add a test case for it.

Why are the changes needed?

Proper name improves user experience with Spark SQL.

Does this PR introduce any user-facing change?

Yes.

How was this patch tested?

Ran all the tests in the suite:

build/sbt "testOnly *org.apache.spark.sql.RowSuite"

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

No

junyuc25 · 2024-07-16T03:34:14Z

Hi @MaxGekk , I propose a PR to update the name for legacy errors. Do you mind taking a look? Thanks.

allisonwang-db

cc @cloud-fan

allisonwang-db · 2024-07-18T18:28:29Z

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

nit:

Suggested change

test("SPARK-42307 - Trying to get the value from a null column should result in error") {

test("SPARK-42307: get the value from a null column should result in error") {

Thanks for the comment @allisonwang-db. I updated the test description.

My bad I clicked the revoke review by mistake...

cc @allisonwang-db @cloud-fan @MaxGekk. Not sure if you got a chance to review this small PR? Thanks!

allisonwang-db · 2024-07-26T17:14:11Z

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

Suggested change

test("SPARK-42307 - get the value from a null column should result in error") {

test("SPARK-42307: get a value from a null column should result in error") {

Thanks @allisonwang-db! Updated.

MaxGekk

Is it correct? If not, please, fix it.

spark/sql/api/src/main/scala/org/apache/spark/sql/Row.scala

Line 526 in 731a104

* @throws NullPointerException when value is null.

MaxGekk · 2024-08-09T10:37:11Z

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

Let's name in more precise way:

Suggested change

"COLUMN_VALUE_IS_NULL" : {

"ROW_VALUE_IS_NULL" : {

MaxGekk · 2024-08-09T10:44:23Z

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

Suggested change

"Value at index <index> is null."

Found NULL in a row at the index <index>, expected a non-NULL value.

MaxGekk · 2024-08-09T10:45:45Z

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

How about this number "22023"

MaxGekk

... name _LEGACY_ERROR_TEMP_2232 with COLUMN_VALUE_IS_NULL

Could you update PR's description according to your changes, please.

MaxGekk · 2024-08-10T19:26:10Z

sql/api/src/main/scala/org/apache/spark/sql/Row.scala

Could you clarify why did you remove it instead of replacing NullPointerException by SparkException, just wonder.

Oops I misunderstood your previous comment. Replaced NPE with SparkException.

And one more thing is need to update the SQL migration guide because this might impact user apps and potentially break them. Please, add an item to sql-migration-guide.md.

[Minor] Update test description

junyuc25 · 2024-08-12T13:02:55Z

Hi @MaxGekk , looks like the current implementation is causing an error when generating docs (job link), because SparkException is declared in the comment but it is not thrown from the codes.

[error] /__w/spark/spark/sql/api/target/java/org/apache/spark/sql/Row.java:135:1:  error: exception not thrown: org.apache.spark.SparkException
[error]    * @throws org.apache.spark.SparkException when value is null.
[error]              ^
[warn] javadoc exited with exit code 1

I found two options to fix this:

Add @throws[SparkException] annotation to the method. This is equivalent to adding throws XXException to a Java method signature (i.e. public boolean getBoolean (int i) throws org.apache.spark.SparkException), and hence Java consumers of this method would need to catch this exception explicitly.

  /**
   * Returns the value at position i as a primitive boolean.
   *
   * @throws ClassCastException when data type does not match.
   * @throws org.apache.spark.SparkException when value is null.
   */
  @throws[SparkException]
  def getBoolean(i: Int): Boolean = getAnyValAs[Boolean](i)

Do not add @throws org.apache.spark.SparkException to the comment, and also remove @throws NullPointerException (because NPE would not be thrown from the codes).

  /**
   * Returns the value at position i as a primitive boolean.
   *
   * @throws ClassCastException when data type does not match.
   */
  def getBoolean(i: Int): Boolean = getAnyValAs[Boolean](i)

I kind of lean towards option #1 as it could provide users with better context about the exception type being thrown, although they would have to handle this exception explicitly in their codes. Let me know your thought on this or if you have other ideas. Thanks :)!

MaxGekk · 2024-08-13T06:22:35Z

@junyuc25 I guess a runtime exception might not force catching the exception. How about to replace SparkException by SparkRuntimeException in valueIsNullError()?

MaxGekk

LGTM waiting for CI.

MaxGekk · 2024-08-13T17:13:45Z

+1, LGTM. Merging to master.
Thank you, @junyuc25 and @allisonwang-db for review.

MaxGekk · 2024-08-13T17:17:42Z

@junyuc25 Do you have an account at the OSS JIRA? If so, leave a comment in SPARK-42307. I will assign the ticket to you.

junyuc25 · 2024-08-14T02:23:42Z

Yes I posted a comment in the Jira ticket. Thanks for reviewing! Sent from Gmail Mobile Maxim Gekk ***@***.***>于2024年8月14日周三01:18写道：

…

@junyuc25 <https://github.com/junyuc25> Do you have an account at the OSS JIRA? If so, leave a comment in SPARK-42307 <https://issues.apache.org/jira/browse/SPARK-42307>. I will assign the ticket to you. — Reply to this email directly, view it on GitHub <#47354 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACS35K5D7DRJXJYHMO7KYV3ZRI5U3AVCNFSM6AAAAABK4I2EH2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEOBWG42DEMJQGM> . You are receiving this because you were mentioned.Message ID: ***@***.***>

github-actions bot added the SQL label Jul 15, 2024

junyuc25 force-pushed the SPARK-42307 branch from 612dd81 to 7db0675 Compare July 15, 2024 14:08

junyuc25 changed the title ~~[WIP][Draft] Assign name for error _LEGACY_ERROR_TEMP_2232~~ [SPARK-42307] Assign name for error _LEGACY_ERROR_TEMP_2232 Jul 16, 2024

junyuc25 marked this pull request as ready for review July 16, 2024 03:32

junyuc25 changed the title ~~[SPARK-42307] Assign name for error _LEGACY_ERROR_TEMP_2232~~ [SPARK-42307][SQL] Assign name for error _LEGACY_ERROR_TEMP_2232 Jul 16, 2024

allisonwang-db approved these changes Jul 18, 2024

View reviewed changes

junyuc25 requested a review from allisonwang-db July 20, 2024 03:14

allisonwang-db reviewed Jul 26, 2024

View reviewed changes

MaxGekk reviewed Aug 9, 2024

View reviewed changes

junyuc25 force-pushed the SPARK-42307 branch from d04b5fa to e69e5cd Compare August 10, 2024 09:39

MaxGekk reviewed Aug 10, 2024

View reviewed changes

junyuc25 added 4 commits August 11, 2024 14:34

Assign name for error _LEGACY_ERROR_TEMP_2232

fe36f58

Add SQL State for the error

e4ec35f

[Minor] Update test description

Update error name based on given comments

0cef78c

Update exception type in comments

e1fb0ea

junyuc25 force-pushed the SPARK-42307 branch from e69e5cd to e1fb0ea Compare August 11, 2024 06:35

Fix doc generation error

70bb7e6

Fix doc generation error by adding @throws[SparkException] to methods

71d7268

junyuc25 added 2 commits August 13, 2024 16:27

Use SparkRuntimeException when row value is null

255a396

Fix unit test in RowTest.scala

58be610

MaxGekk reviewed Aug 13, 2024

View reviewed changes

MaxGekk approved these changes Aug 13, 2024

View reviewed changes

MaxGekk closed this in f081650 Aug 13, 2024

	test("SPARK-42307 - Trying to get the value from a null column should result in error") {
	test("SPARK-42307: get the value from a null column should result in error") {

	test("SPARK-42307 - get the value from a null column should result in error") {
	test("SPARK-42307: get a value from a null column should result in error") {

	"Value at index <index> is null."
	Found NULL in a row at the index <index>, expected a non-NULL value.

Conversation

junyuc25 commented Jul 15, 2024 • 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?

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

Uh oh!

junyuc25 commented Jul 16, 2024

Uh oh!

allisonwang-db left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MaxGekk left a comment • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MaxGekk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

junyuc25 commented Aug 12, 2024

Uh oh!

MaxGekk commented Aug 13, 2024

Uh oh!

MaxGekk left a comment

Choose a reason for hiding this comment

Uh oh!

MaxGekk commented Aug 13, 2024

Uh oh!

MaxGekk commented Aug 13, 2024

Uh oh!

junyuc25 commented Aug 14, 2024 via email

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

junyuc25 commented Jul 15, 2024 •

edited by MaxGekk

Loading

MaxGekk left a comment •

edited

Loading