Skip to content

Conversation

@MaxGekk
Copy link
Member

@MaxGekk MaxGekk commented Aug 30, 2023

What changes were proposed in this pull request?

In the PR, I propose to assign the name NON_FOLDABLE_ARGUMENT to the legacy error class _LEGACY_ERROR_TEMP_1100, and improve the error message format: make it less restrictive.

Why are the changes needed?

  1. To don't confuse users by slightly restrictive error message about literals.
  2. To assign proper name as a part of activity in SPARK-37935

Does this PR introduce any user-facing change?

No. Only if user's code depends on error class name and message parameters.

How was this patch tested?

By running the modified and affected tests:

$ build/sbt "test:testOnly *.StringFunctionsSuite"
$ PYSPARK_PYTHON=python3 build/sbt "sql/testOnly org.apache.spark.sql.SQLQueryTestSuite"
$ build/sbt "core/testOnly *SparkThrowableSuite"

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

No.

},
"NON_FOLDABLE_ARGUMENT" : {
"message" : [
"The function <funcName> requires the parameter <paramName> to be a foldable expression of the type <paramType>, but the actual argument is a non-foldable."
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit concerned, does the user know what is foldable and what is non-foldable?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How would you improve the error message?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After thinking about it, I really didn't find a better way to express it.😂

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ya, in this case, new message looks reasonable to me. :)

@MaxGekk
Copy link
Member Author

MaxGekk commented Aug 31, 2023

@dongjoon-hyun @HyukjinKwon Could you review this PR, please.

@dongjoon-hyun
Copy link
Member

Sure, @MaxGekk .


### NON_FOLDABLE_ARGUMENT

[SQLSTATE: 22024](sql-error-conditions-sqlstates.html#class-22-data-exception)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Data exception category is also reasonable to me if there is no other proper one.

Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, LGTM. Thank you, @MaxGekk .

@MaxGekk
Copy link
Member Author

MaxGekk commented Aug 31, 2023

Merging to master. Thank you, @dongjoon-hyun and @Hisoka-X for review.

@MaxGekk MaxGekk closed this in e72ce91 Aug 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants