[SPARK-46036][SQL] Removing error-class from raise_error function #44004

dbatomic · 2023-11-24T13:48:15Z

What changes were proposed in this pull request?

Reverting raise_error to single param variant that is enforced through ExpressionBuilder interface (vs. reflection based lookup).
Keeping internal RaiseError class untouched with ability to throw custom exception class. The only way to call it as of this change is from internal code. Open question is whether we want to clean up this code as well, given that currently no one is using it and this code seems too complex if we don't need to expose RaiseError in surface layer. Looking for feedback on this one.

Why are the changes needed?

This PR is a follow up of raise_error improvement PR. As described in jira ticket, raise_error with custom error-class parameter was deemed to be too powerful with concern that user might throw internal system errors which shouldn't be exposed to external layers.

With this change, from external perspective, we revert to the old behaviour, where raise_error only accepts single string argument which will represent message of USER_RAISED_EXCEPTION exception object.

Does this PR introduce any user-facing change?

Yes, we introduce change in raise_error signature. With this change it is no longer allowed to call:

SELECT raise_error('VIEW_NOT_FOUND', Map('relationName', '`v`'))

Instead, only allowed version will be:

SELECT raise_error('custom error message'))

Which will result in USER_RAISED_EXCEPTION being thrown with provided message.

How was this patch tested?

The change updates existing sql tests under misc-functions.sql and adds some new checks.

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

No.

dbatomic · 2023-11-24T15:04:34Z

sql/core/src/test/resources/sql-tests/inputs/misc-functions.sql


-SELECT raise_error(NULL, NULL);
+-- TODO: Need feedback on proper behaviour here:


Looking for feedback here.

The implicit cast does not support converting complex type to string type. Actually we can skip these 3 tests as implicit cast is orthogonal to the raise_error function

beliefer · 2023-11-25T11:12:20Z

sql/core/src/main/scala/org/apache/spark/sql/functions.scala

-   * @group misc_funcs
-   * @since 4.0.0
-   */
-  def raise_error(c: Column, e: Column): Column = Column.fn("raise_error", c, e)


It seems this API added no long.

Not sure that I understand the comment. Can you please elaborate?

cc @srielau

beliefer · 2023-11-25T11:13:06Z

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

+  since = "3.1.0",
+  group = "misc_funcs")
+// scalastyle:on line.size.limit
+object RaiseErrorExpressionBuilder extends ExpressionBuilder {


RaiseError works good, why you create the builder?

In my offline discussion with @cloud-fan we tried to come with a way to achieve following:

Leave RaiseError class intact so internally we can throw errors of custom class.

Externally allow only raise_error('message') API.

My understanding is that expression[RaiseError]("raise_error") ends up doing reflection that would expose a function for every constructor signature in RaiseError, meaning that we will expose both def this(str: Expression) and def this(errorClass: Expression, errorParms: Expression) from RaiseError.

Instead of doing this, we take ExpressionBuilder path that will expose only single argument version.

Got it. It hears good to me. We can use this builder to wrap the visibility.

cloud-fan · 2023-11-25T14:03:10Z

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

+// scalastyle:on line.size.limit
+object RaiseErrorExpressionBuilder extends ExpressionBuilder {
+  override def build(funcName: String, expressions: Seq[Expression]): Expression = {
+    // for some reason pattern matching doesn't work here...


I think explicit length check is better than pattern match here. We don't need this comment.

cloud-fan · 2023-11-25T14:05:12Z

cc @srielau

cloud-fan · 2023-11-27T21:26:02Z

sql/core/src/main/scala/org/apache/spark/sql/functions.scala

-   * @group misc_funcs
-   * @since 4.0.0
-   */
-  def raise_error(c: Column, e: Column): Column = Column.fn("raise_error", c, e)


We should remove this API from connector/connect/client/jvm/src/main/scala/org/apache/spark/sql/functions.scala

@cloud-fan This PR will forbid to expose the errorParms?

done.

@beliefer Yes, idea is to forbid errorParams exposure, given that we will always throw USER_RAISED_EXCEPTION with single arg (string representing the message). @cloud-fan, please confirm if I got things right.

@dbatomic Thank you for the explanation. Please wait the confirm.

beliefer

LGTM except some comments.

beliefer · 2023-11-28T03:10:31Z

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

+  since = "3.1.0",
+  group = "misc_funcs")
+// scalastyle:on line.size.limit
+object RaiseErrorExpressionBuilder extends ExpressionBuilder {


Got it. It hears good to me. We can use this builder to wrap the visibility.

beliefer · 2023-11-28T03:13:43Z

sql/core/src/main/scala/org/apache/spark/sql/functions.scala

-   * @group misc_funcs
-   * @since 4.0.0
-   */
-  def raise_error(c: Column, e: Column): Column = Column.fn("raise_error", c, e)


@cloud-fan This PR will forbid to expose the errorParms?

…-error update of master

cloud-fan · 2023-11-29T11:51:10Z

thanks, merging to master!

dbatomic added 2 commits November 23, 2023 16:17

initial change, stable repl testing

3ddc2f5

test fixes

c1f83cf

github-actions bot added SQL DOCS labels Nov 24, 2023

Reverting sql-error-conditions

0ecc9c4

github-actions bot removed the DOCS label Nov 24, 2023

dbatomic commented Nov 24, 2023

View reviewed changes

fixing failing tests

de16905

beliefer reviewed Nov 25, 2023

View reviewed changes

cloud-fan reviewed Nov 25, 2023

View reviewed changes

Removing redundant tests

90a00db

cloud-fan approved these changes Nov 27, 2023

View reviewed changes

cloud-fan reviewed Nov 27, 2023

View reviewed changes

beliefer approved these changes Nov 28, 2023

View reviewed changes

removing raise_error with class type argument

d2f4b2c

github-actions bot added the CONNECT label Nov 28, 2023

Merge remote-tracking branch 'upstream/master' into SPARK-46036-raise…

cfe2f00

…-error update of master

cloud-fan approved these changes Nov 29, 2023

View reviewed changes

cloud-fan closed this in fa4096e Nov 29, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-46036][SQL] Removing error-class from raise_error function #44004

[SPARK-46036][SQL] Removing error-class from raise_error function #44004

dbatomic commented Nov 24, 2023

dbatomic Nov 24, 2023

cloud-fan Nov 25, 2023

dbatomic Nov 27, 2023

beliefer Nov 25, 2023

dbatomic Nov 27, 2023

beliefer Nov 28, 2023

beliefer Nov 28, 2023

beliefer Nov 25, 2023

dbatomic Nov 27, 2023

beliefer Nov 28, 2023

cloud-fan Nov 25, 2023

dbatomic Nov 27, 2023

cloud-fan commented Nov 25, 2023

cloud-fan Nov 27, 2023

beliefer Nov 28, 2023

dbatomic Nov 28, 2023

beliefer Nov 28, 2023

beliefer left a comment

beliefer Nov 28, 2023

beliefer Nov 28, 2023

cloud-fan commented Nov 29, 2023


		SELECT raise_error(NULL, NULL);
		-- TODO: Need feedback on proper behaviour here:

[SPARK-46036][SQL] Removing error-class from raise_error function #44004

[SPARK-46036][SQL] Removing error-class from raise_error function #44004

Conversation

dbatomic commented Nov 24, 2023

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?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cloud-fan commented Nov 25, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beliefer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cloud-fan commented Nov 29, 2023