Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
dbatomic wants to merge 7 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
3ddc2f5
initial change, stable repl testing
dbatomic Nov 23, 2023
c1f83cf
test fixes
dbatomic Nov 24, 2023
0ecc9c4
Reverting sql-error-conditions
dbatomic Nov 24, 2023
de16905
fixing failing tests
dbatomic Nov 25, 2023
90a00db
Removing redundant tests
dbatomic Nov 27, 2023
d2f4b2c
removing raise_error with class type argument
dbatomic Nov 28, 2023
cfe2f00
Merge remote-tracking branch 'upstream/master' into SPARK-46036-raise…
dbatomic Nov 29, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 0 additions & 8 deletions connector/connect/client/jvm/src/main/scala/org/apache/spark/sql/functions.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3318,14 +3318,6 @@ object functions {
*/
def raise_error(c: Column): Column = Column.fn("raise_error", c)

/**
* Throws an exception with the provided error message.
*
* @group misc_funcs
* @since 4.0.0
*/
def raise_error(c: Column, e: Column): Column = Column.fn("raise_error", c, e)

/**
* Returns the estimated number of unique values given the binary representation of a
* Datasketches HllSketch.
Expand Down
2 changes: 1 addition & 1 deletion sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionRegistry.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -722,7 +722,7 @@ object FunctionRegistry {

// misc functions
expression[AssertTrue]("assert_true"),
expression[RaiseError]("raise_error"),
expressionBuilder("raise_error", RaiseErrorExpressionBuilder),
expression[Crc32]("crc32"),
expression[Md5]("md5"),
expression[Uuid]("uuid"),
Expand Down
43 changes: 28 additions & 15 deletions sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/misc.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,13 @@ package org.apache.spark.sql.catalyst.expressions

import org.apache.spark.{SPARK_REVISION, SPARK_VERSION_SHORT}
import org.apache.spark.sql.catalyst.InternalRow
import org.apache.spark.sql.catalyst.analysis.UnresolvedSeed
import org.apache.spark.sql.catalyst.analysis.{ExpressionBuilder, UnresolvedSeed}
import org.apache.spark.sql.catalyst.expressions.codegen._
import org.apache.spark.sql.catalyst.expressions.codegen.Block._
import org.apache.spark.sql.catalyst.expressions.objects.StaticInvoke
import org.apache.spark.sql.catalyst.trees.TreePattern.{CURRENT_LIKE, TreePattern}
import org.apache.spark.sql.catalyst.util.{MapData, RandomUUIDGenerator}
import org.apache.spark.sql.errors.QueryCompilationErrors
import org.apache.spark.sql.errors.QueryExecutionErrors.raiseError
import org.apache.spark.sql.internal.SQLConf
import org.apache.spark.sql.types._
Expand Down Expand Up @@ -61,21 +62,9 @@ case class PrintToStderr(child: Expression) extends UnaryExpression {

/**
* Throw with the result of an expression (used for debugging).
* Caller can specify the errorClass to be thrown and parameters to be passed to this error class.
* Default is to throw USER_RAISED_EXCEPTION with provided string literal.
*/
// scalastyle:off line.size.limit
@ExpressionDescription(
usage = "_FUNC_( expr [, errorParams ]) - Throws a USER_RAISED_EXCEPTION with `expr` as message, or a defined error class in `expr` with a parameter map. A `null` errorParms is equivalent to an empty map.",
examples = """
Examples:
> SELECT _FUNC_('custom error message');
[USER_RAISED_EXCEPTION] custom error message

> SELECT _FUNC_('VIEW_NOT_FOUND', Map('relationName' -> '`V1`'));
[VIEW_NOT_FOUND] The view `V1` cannot be found. ...
""",
since = "3.1.0",
group = "misc_funcs")
// scalastyle:on line.size.limit
case class RaiseError(errorClass: Expression, errorParms: Expression, dataType: DataType)
extends BinaryExpression with ImplicitCastInputTypes {

Expand Down Expand Up @@ -139,6 +128,30 @@ object RaiseError {
new RaiseError(errorClass, parms)
}

/**
* Throw with the result of an expression (used for debugging).
*/
// scalastyle:off line.size.limit
@ExpressionDescription(
usage = "_FUNC_( expr ) - Throws a USER_RAISED_EXCEPTION with `expr` as message.",
examples = """
Examples:
> SELECT _FUNC_('custom error message');
[USER_RAISED_EXCEPTION] custom error message
""",
since = "3.1.0",
group = "misc_funcs")
// scalastyle:on line.size.limit
object RaiseErrorExpressionBuilder extends ExpressionBuilder {
Copy link
Copy Markdown
Contributor

@beliefer beliefer Nov 25, 2023

Choose a reason for hiding this comment

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

RaiseError works good, why you create the builder?

Copy link
Copy Markdown
Contributor Author

@dbatomic dbatomic Nov 27, 2023

Choose a reason for hiding this comment

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

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

  1. Leave RaiseError class intact so internally we can throw errors of custom class.
  2. 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.

Copy link
Copy Markdown
Contributor

@beliefer beliefer Nov 28, 2023

Choose a reason for hiding this comment

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

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

override def build(funcName: String, expressions: Seq[Expression]): Expression = {
if (expressions.length != 1) {
throw QueryCompilationErrors.wrongNumArgsError(funcName, Seq(1), expressions.length)
} else {
RaiseError(expressions.head)
}
}
}

/**
* A function that throws an exception if 'condition' is not true.
*/
Expand Down
8 changes: 0 additions & 8 deletions sql/core/src/main/scala/org/apache/spark/sql/functions.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3269,14 +3269,6 @@ object functions {
*/
def raise_error(c: Column): Column = Column.fn("raise_error", c)

/**
* Throws an exception with the provided error class and parameter map.
*
* @group misc_funcs
* @since 4.0.0
*/
def raise_error(c: Column, e: Column): Column = Column.fn("raise_error", c, e)
Copy link
Copy Markdown
Contributor

@beliefer beliefer Nov 25, 2023

Choose a reason for hiding this comment

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

It seems this API added no long.

Copy link
Copy Markdown
Contributor Author

@dbatomic dbatomic Nov 27, 2023

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor

@beliefer beliefer Nov 28, 2023

Choose a reason for hiding this comment

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

#42985.

Copy link
Copy Markdown
Contributor

@beliefer beliefer Nov 28, 2023

Choose a reason for hiding this comment

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

cc @srielau

Copy link
Copy Markdown
Contributor

@cloud-fan cloud-fan Nov 27, 2023

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor

@beliefer beliefer Nov 28, 2023

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor Author

@dbatomic dbatomic Nov 28, 2023

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor

@beliefer beliefer Nov 28, 2023

Choose a reason for hiding this comment

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

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


/**
* Returns the estimated number of unique values given the binary representation
* of a Datasketches HllSketch.
Expand Down
131 changes: 79 additions & 52 deletions sql/core/src/test/resources/sql-tests/analyzer-results/misc-functions.sql.out
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,58 +120,85 @@ Project [if ((v#x > 5)) cast(raise_error(USER_RAISED_EXCEPTION, map(errorMessage


-- !query
SELECT raise_error('VIEW_NOT_FOUND', Map('relationName', '`v`'))
-- !query analysis
Project [raise_error(VIEW_NOT_FOUND, map(relationName, `v`), NullType) AS raise_error(VIEW_NOT_FOUND, map(relationName, `v`))#x]
+- OneRowRelation


-- !query
SELECT raise_error('VIEW_NOT_FOund', Map('relationName', '`v`'))
-- !query analysis
Project [raise_error(VIEW_NOT_FOund, map(relationName, `v`), NullType) AS raise_error(VIEW_NOT_FOund, map(relationName, `v`))#x]
+- OneRowRelation


-- !query
SELECT raise_error('VIEW_NOT_FOund', Map('relationNAME', '`v`'))
-- !query analysis
Project [raise_error(VIEW_NOT_FOund, map(relationNAME, `v`), NullType) AS raise_error(VIEW_NOT_FOund, map(relationNAME, `v`))#x]
+- OneRowRelation


-- !query
SELECT raise_error('VIEW_NOT_FOUND', Map())
-- !query analysis
Project [raise_error(VIEW_NOT_FOUND, cast(map() as map<string,string>), NullType) AS raise_error(VIEW_NOT_FOUND, map())#x]
+- OneRowRelation


-- !query
SELECT raise_error('VIEW_NOT_FOUND', Map('relationName', '`v`', 'totallymadeup', '5'))
-- !query analysis
Project [raise_error(VIEW_NOT_FOUND, map(relationName, `v`, totallymadeup, 5), NullType) AS raise_error(VIEW_NOT_FOUND, map(relationName, `v`, totallymadeup, 5))#x]
+- OneRowRelation


-- !query
SELECT raise_error('ALL_PARTITION_COLUMNS_NOT_ALLOWED', Map())
-- !query analysis
Project [raise_error(ALL_PARTITION_COLUMNS_NOT_ALLOWED, cast(map() as map<string,string>), NullType) AS raise_error(ALL_PARTITION_COLUMNS_NOT_ALLOWED, map())#x]
+- OneRowRelation


-- !query
SELECT raise_error('ALL_PARTITION_COLUMNS_NOT_ALLOWED', NULL)
-- !query analysis
Project [raise_error(ALL_PARTITION_COLUMNS_NOT_ALLOWED, cast(null as map<string,string>), NullType) AS raise_error(ALL_PARTITION_COLUMNS_NOT_ALLOWED, NULL)#x]
+- OneRowRelation


-- !query
SELECT raise_error(NULL, NULL)
-- !query analysis
Project [raise_error(cast(null as string), cast(null as map<string,string>), NullType) AS raise_error(NULL, NULL)#x]
SELECT raise_error('error message', Map())
-- !query analysis
org.apache.spark.sql.AnalysisException
{
"errorClass" : "WRONG_NUM_ARGS.WITHOUT_SUGGESTION",
"sqlState" : "42605",
"messageParameters" : {
"actualNum" : "2",
"docroot" : "https://spark.apache.org/docs/latest",
"expectedNum" : "1",
"functionName" : "`raise_error`"
},
"queryContext" : [ {
"objectType" : "",
"objectName" : "",
"startIndex" : 8,
"stopIndex" : 42,
"fragment" : "raise_error('error message', Map())"
} ]
}


-- !query
SELECT raise_error('error message', 'some args')
-- !query analysis
org.apache.spark.sql.AnalysisException
{
"errorClass" : "WRONG_NUM_ARGS.WITHOUT_SUGGESTION",
"sqlState" : "42605",
"messageParameters" : {
"actualNum" : "2",
"docroot" : "https://spark.apache.org/docs/latest",
"expectedNum" : "1",
"functionName" : "`raise_error`"
},
"queryContext" : [ {
"objectType" : "",
"objectName" : "",
"startIndex" : 8,
"stopIndex" : 48,
"fragment" : "raise_error('error message', 'some args')"
} ]
}


-- !query
SELECT raise_error()
-- !query analysis
org.apache.spark.sql.AnalysisException
{
"errorClass" : "WRONG_NUM_ARGS.WITHOUT_SUGGESTION",
"sqlState" : "42605",
"messageParameters" : {
"actualNum" : "0",
"docroot" : "https://spark.apache.org/docs/latest",
"expectedNum" : "1",
"functionName" : "`raise_error`"
},
"queryContext" : [ {
"objectType" : "",
"objectName" : "",
"startIndex" : 8,
"stopIndex" : 20,
"fragment" : "raise_error()"
} ]
}


-- !query
SELECT raise_error(NULL)
-- !query analysis
Project [raise_error(USER_RAISED_EXCEPTION, cast(map(errorMessage, null) as map<string,string>), NullType) AS raise_error(USER_RAISED_EXCEPTION, map(errorMessage, NULL))#x]
+- OneRowRelation


-- !query
SELECT raise_error(1)
-- !query analysis
Project [raise_error(USER_RAISED_EXCEPTION, cast(map(errorMessage, 1) as map<string,string>), NullType) AS raise_error(USER_RAISED_EXCEPTION, map(errorMessage, 1))#x]
+- OneRowRelation


Expand Down
23 changes: 11 additions & 12 deletions sql/core/src/test/resources/sql-tests/inputs/misc-functions.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,21 +21,20 @@ CREATE TEMPORARY VIEW tbl_misc AS SELECT * FROM (VALUES (1), (8), (2)) AS T(v);
SELECT raise_error('error message');
SELECT if(v > 5, raise_error('too big: ' || v), v + 1) FROM tbl_misc;

SELECT raise_error('VIEW_NOT_FOUND', Map('relationName', '`v`'));
-- Error class is case insensitive
SELECT raise_error('VIEW_NOT_FOund', Map('relationName', '`v`'));
-- parameters are case sensitive
SELECT raise_error('VIEW_NOT_FOund', Map('relationNAME', '`v`'));
-- Too few parameters
SELECT raise_error('VIEW_NOT_FOUND', Map());
-- Too many parameters
SELECT raise_error('VIEW_NOT_FOUND', Map('relationName', '`v`', 'totallymadeup', '5'));
SELECT raise_error('error message', Map());

-- Too many parameters
SELECT raise_error('error message', 'some args');

-- Too few parameters
SELECT raise_error();

-- Empty parameter list
SELECT raise_error('ALL_PARTITION_COLUMNS_NOT_ALLOWED', Map());
SELECT raise_error('ALL_PARTITION_COLUMNS_NOT_ALLOWED', NULL);
-- Passing null as message
SELECT raise_error(NULL);

SELECT raise_error(NULL, NULL);
-- Passing non-string type
SELECT raise_error(1);

-- Check legacy config disables printing of [USER_RAISED_EXCEPTION]
SET spark.sql.legacy.raiseErrorWithoutErrorClass=true;
Expand Down
Loading