You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "dbatomic (via GitHub)" <gi...@apache.org> on 2023/11/24 13:48:17 UTC
[PR] [SPARK-46036][SQL] Removing error-class from raise_error function [spark]
dbatomic opened a new pull request, #44004:
URL: https://github.com/apache/spark/pull/44004
### What changes were proposed in this pull request?
<!--
Please clarify what changes you are proposing. The purpose of this section is to outline the changes and how this PR fixes the issue.
If possible, please consider writing useful notes for better and faster reviews in your PR. See the examples below.
1. If you refactor some codes with changing classes, showing the class hierarchy will help reviewers.
2. If you fix some SQL features, you can provide some references of other DBMSes.
3. If there is design documentation, please add the link.
4. If there is a discussion in the mailing list, please add the link.
-->
- 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?
<!--
Please clarify why the changes are needed. For instance,
1. If you propose a new API, clarify the use case for a new API.
2. If you fix a bug, you can clarify why it is a bug.
-->
This PR is a follow up of [raise_error improvement PR](https://github.com/apache/spark/pull/42985). As described in jira [ticket](https://issues.apache.org/jira/browse/SPARK-46036), 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?
<!--
Note that it means *any* user-facing change including all aspects such as the documentation fix.
If yes, please clarify the previous behavior and the change this PR proposes - provide the console output, description and/or an example to show the behavior difference if possible.
If possible, please also clarify if this is a user-facing change compared to the released Spark versions or within the unreleased branches such as master.
If no, write 'No'.
-->
Yes, we introduce change in raise_error signature. With this change it is no longer allowed to call:
```sql
SELECT raise_error('VIEW_NOT_FOUND', Map('relationName', '`v`'))
```
Instead, only allowed version will be:
```sql
SELECT raise_error('custom error message'))
```
Which will result in `USER_RAISED_EXCEPTION` being thrown with provided message.
### How was this patch tested?
<!--
If tests were added, say they were added here. Please make sure to add some test cases that check the changes thoroughly including negative and positive cases if possible.
If it was tested in a way different from regular unit tests, please clarify how you tested step by step, ideally copy and paste-able, so that other reviewers can test and check, and descendants can verify in the future.
If tests were not added, please describe why they were not added and/or why it was difficult to add.
If benchmark tests were added, please run the benchmarks in GitHub Actions for the consistent environment, and the instructions could accord to: https://spark.apache.org/developer-tools.html#github-workflow-benchmarks.
-->
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?
<!--
If generative AI tooling has been used in the process of authoring this patch, please include the
phrase: 'Generated-by: ' followed by the name of the tool and its version.
If no, write 'No'.
Please refer to the [ASF Generative Tooling Guidance](https://www.apache.org/legal/generative-tooling.html) for details.
-->
No.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-46036][SQL] Removing error-class from raise_error function [spark]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on code in PR #44004:
URL: https://github.com/apache/spark/pull/44004#discussion_r1407509463
##########
sql/core/src/main/scala/org/apache/spark/sql/functions.scala:
##########
@@ -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)
Review Comment:
@dbatomic Thank you for the explanation. Please wait the confirm.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-46036][SQL] Removing error-class from raise_error function [spark]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on code in PR #44004:
URL: https://github.com/apache/spark/pull/44004#discussion_r1404849612
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/misc.scala:
##########
@@ -139,6 +128,31 @@ 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 {
Review Comment:
`RaiseError` works good, why you create the builder?
##########
sql/core/src/main/scala/org/apache/spark/sql/functions.scala:
##########
@@ -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)
Review Comment:
It seems this API added no long.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-46036][SQL] Removing error-class from raise_error function [spark]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on code in PR #44004:
URL: https://github.com/apache/spark/pull/44004#discussion_r1407033432
##########
sql/core/src/main/scala/org/apache/spark/sql/functions.scala:
##########
@@ -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)
Review Comment:
cc @srielau
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-46036][SQL] Removing error-class from raise_error function [spark]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on code in PR #44004:
URL: https://github.com/apache/spark/pull/44004#discussion_r1407032537
##########
sql/core/src/main/scala/org/apache/spark/sql/functions.scala:
##########
@@ -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)
Review Comment:
https://github.com/apache/spark/pull/42985.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-46036][SQL] Removing error-class from raise_error function [spark]
Posted by "dbatomic (via GitHub)" <gi...@apache.org>.
dbatomic commented on code in PR #44004:
URL: https://github.com/apache/spark/pull/44004#discussion_r1404447346
##########
sql/core/src/test/resources/sql-tests/inputs/misc-functions.sql:
##########
@@ -21,21 +21,30 @@ 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();
+
+-- Passing null as message
+SELECT raise_error(NULL);
+
+-- Passing non-string type
+SELECT raise_error(1);
--- Empty parameter list
-SELECT raise_error('ALL_PARTITION_COLUMNS_NOT_ALLOWED', Map());
-SELECT raise_error('ALL_PARTITION_COLUMNS_NOT_ALLOWED', NULL);
+-- Passing expression
+SELECT raise_error(1 + 1);
-SELECT raise_error(NULL, NULL);
+-- TODO: Need feedback on proper behaviour here:
Review Comment:
Looking for feedback here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-46036][SQL] Removing error-class from raise_error function [spark]
Posted by "dbatomic (via GitHub)" <gi...@apache.org>.
dbatomic commented on code in PR #44004:
URL: https://github.com/apache/spark/pull/44004#discussion_r1406453367
##########
sql/core/src/test/resources/sql-tests/inputs/misc-functions.sql:
##########
@@ -21,21 +21,30 @@ 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();
+
+-- Passing null as message
+SELECT raise_error(NULL);
+
+-- Passing non-string type
+SELECT raise_error(1);
--- Empty parameter list
-SELECT raise_error('ALL_PARTITION_COLUMNS_NOT_ALLOWED', Map());
-SELECT raise_error('ALL_PARTITION_COLUMNS_NOT_ALLOWED', NULL);
+-- Passing expression
+SELECT raise_error(1 + 1);
-SELECT raise_error(NULL, NULL);
+-- TODO: Need feedback on proper behaviour here:
Review Comment:
done
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/misc.scala:
##########
@@ -139,6 +128,31 @@ 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 {
+ override def build(funcName: String, expressions: Seq[Expression]): Expression = {
+ // for some reason pattern matching doesn't work here...
Review Comment:
right
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-46036][SQL] Removing error-class from raise_error function [spark]
Posted by "dbatomic (via GitHub)" <gi...@apache.org>.
dbatomic commented on code in PR #44004:
URL: https://github.com/apache/spark/pull/44004#discussion_r1406452281
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/misc.scala:
##########
@@ -139,6 +128,31 @@ 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 {
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-46036][SQL] Removing error-class from raise_error function [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #44004:
URL: https://github.com/apache/spark/pull/44004#issuecomment-1826332136
cc @srielau
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-46036][SQL] Removing error-class from raise_error function [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan closed pull request #44004: [SPARK-46036][SQL] Removing error-class from raise_error function
URL: https://github.com/apache/spark/pull/44004
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-46036][SQL] Removing error-class from raise_error function [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #44004:
URL: https://github.com/apache/spark/pull/44004#discussion_r1405037748
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/misc.scala:
##########
@@ -139,6 +128,31 @@ 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 {
+ override def build(funcName: String, expressions: Seq[Expression]): Expression = {
+ // for some reason pattern matching doesn't work here...
Review Comment:
I think explicit length check is better than pattern match here. We don't need this comment.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-46036][SQL] Removing error-class from raise_error function [spark]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on code in PR #44004:
URL: https://github.com/apache/spark/pull/44004#discussion_r1407026169
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/misc.scala:
##########
@@ -139,6 +128,31 @@ 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 {
Review Comment:
Got it. It hears good to me. We can use this builder to wrap the visibility.
##########
sql/core/src/main/scala/org/apache/spark/sql/functions.scala:
##########
@@ -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)
Review Comment:
@cloud-fan This PR will forbid to expose the `errorParms`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-46036][SQL] Removing error-class from raise_error function [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #44004:
URL: https://github.com/apache/spark/pull/44004#discussion_r1406766213
##########
sql/core/src/main/scala/org/apache/spark/sql/functions.scala:
##########
@@ -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)
Review Comment:
We should remove this API from `connector/connect/client/jvm/src/main/scala/org/apache/spark/sql/functions.scala`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-46036][SQL] Removing error-class from raise_error function [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #44004:
URL: https://github.com/apache/spark/pull/44004#discussion_r1405045604
##########
sql/core/src/test/resources/sql-tests/inputs/misc-functions.sql:
##########
@@ -21,21 +21,30 @@ 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();
+
+-- Passing null as message
+SELECT raise_error(NULL);
+
+-- Passing non-string type
+SELECT raise_error(1);
--- Empty parameter list
-SELECT raise_error('ALL_PARTITION_COLUMNS_NOT_ALLOWED', Map());
-SELECT raise_error('ALL_PARTITION_COLUMNS_NOT_ALLOWED', NULL);
+-- Passing expression
+SELECT raise_error(1 + 1);
-SELECT raise_error(NULL, NULL);
+-- TODO: Need feedback on proper behaviour here:
Review Comment:
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-46036][SQL] Removing error-class from raise_error function [spark]
Posted by "dbatomic (via GitHub)" <gi...@apache.org>.
dbatomic commented on code in PR #44004:
URL: https://github.com/apache/spark/pull/44004#discussion_r1407438638
##########
sql/core/src/main/scala/org/apache/spark/sql/functions.scala:
##########
@@ -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)
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-46036][SQL] Removing error-class from raise_error function [spark]
Posted by "dbatomic (via GitHub)" <gi...@apache.org>.
dbatomic commented on code in PR #44004:
URL: https://github.com/apache/spark/pull/44004#discussion_r1406456439
##########
sql/core/src/main/scala/org/apache/spark/sql/functions.scala:
##########
@@ -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)
Review Comment:
Not sure that I understand the comment. Can you please elaborate?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-46036][SQL] Removing error-class from raise_error function [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #44004:
URL: https://github.com/apache/spark/pull/44004#issuecomment-1831748410
thanks, merging to master!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org