[SPARK-48845][SQL] GenericUDF catch exceptions from children by jackylee-ch · Pull Request #47268 · apache/spark

jackylee-ch · 2024-07-09T12:51:37Z

What changes were proposed in this pull request?

This pr is trying to fix the syntax issues with GenericUDF since 3.5.0. The problem arose from DeferredObject currently passing a value instead of a function, which prevented users from catching exceptions in GenericUDF, resulting in semantic differences.

Here is an example case we encountered. Originally, the semantics were that udf_exception would throw an exception, while udf_catch_exception could catch the exception and return a null value. However, currently, any exception encountered by udf_exception will cause the program to fail.

select udf_catch_exception(udf_exception(col1)) from table

Why are the changes needed?

For before Spark 3.5, we directly made the GenericUDF's DeferredObject lazy and evaluated the children in function.evaluate(deferredObjects).
Now, we would run the children's code first. If an exception is thrown, we would make it lazy to GenericUDF's DeferredObject.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Newly added UT.

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

No.

…xception_from_child_func

LuciferYang · 2024-07-09T13:07:20Z

cc @cloud-fan @yaooqinn @panbingkun FYI

cloud-fan · 2024-07-09T13:56:20Z

Can we explain the execution path before Spark 3.5? Is this PR trying to follow the previous execution path?

jackylee-ch · 2024-07-09T14:59:21Z

Can we explain the execution path before Spark 3.5? Is this PR trying to follow the previous execution path?

For Spark 3.3, the HiveGenericUDF would set lazy functions to deferredObjects and evaluate in function.evaluate, which is same with the non-codegen mode in this pr.

yaooqinn · 2024-07-10T02:27:18Z

Is it possible to combine the code generation and non-code generation code?

jackylee-ch · 2024-07-10T02:40:35Z

Is it possible to combine the code generation and non-code generation code?

AFAIK, it's hard to set lazy function to DeferredObject. I have tried with lambda functions and inner classes to put eval.code in it. The Janino does not support lambda. Additionally, the eval.code relies on other variables, such as columnartorow_batchIdx_0 from BoundReference, which cannot be accessed in an inner class.

Any better ideas?

panbingkun · 2024-07-10T02:57:10Z

 def setArg(index: Int, arg: Any): Unit =
 deferredObjects(index).asInstanceOf[DeferredObjectAdapter].set(arg)

+ def setFuncArg(index: Int, arg: () => Any): Unit =


It seems that setFuncArg and setException can be merged?

It is a little ugly that we use inner class in codegen. If we use setFuncArg, the codegen for exceptions would be follow code.
s""" |try { | ${eval.code} | if (${eval.isNull}) { | $refEvaluator.setArg($i, null); | } else { | $refEvaluator.setArg($i, ${eval.value}); | } |} catch (Exception exp) { | final exception = exp; | $refEvaluator.setFundArg($i, new scala.Function0<Object>() { | public Object apply() { | throw exception; | } | }); |} |""".stripMargin

panbingkun · 2024-07-10T02:59:05Z

+ | $refEvaluator.setArg($i, null);
+ | } else {
+ | $refEvaluator.setArg($i, ${eval.value});
+ | }


It seems that catch (...) is only related to ${eval.code}
Why is it Exception instead of Throwable?

It seems that catch (...) is only related to ${eval.code}
Yes, we should catch the exceptions from child's executions.
Why is it Exception instead of Throwable?
The Exception can catch most of throwable problems. It's ok to be change to Throwable.

panbingkun · 2024-07-10T03:01:23Z

Will it cover multiple nested scenes?
The solution here seems to still bypass the child's lazy execution, right?

jackylee-ch · 2024-07-10T03:18:09Z

Will it cover multiple nested scenes? The solution here seems to still bypass the child's lazy execution, right?

Yes, it cover the multiple nested scenes. And yes the children.evaluate execute first, but we postpone the handling of Exceptions to ensure that the children's exceptions can be obtained during GenericUDF.eval. When facing multiple layers of GenericUDF, if the innermost child throws an exception, we also gradually execute the child GenericUDFs and handle the thrown exceptions layer by layer until the top layer.

yaooqinn

LGTM from myside

cloud-fan · 2024-07-11T12:42:18Z

- |} else {
- | $refEvaluator.setArg($i, ${eval.value});
+ |try {
+ | ${eval.code}


This is slightly different from the interpreted path. In codegen we still eagerly execute the function inputs, but delay the throw of errors. I understand that it's hard to be truly lazy in codegen, can we update the interpreted path instead to make it the same as codegen?

Sure and done. I have updated the code for non-codegen mode and the description for this pr.

cloud-fan · 2024-07-11T13:16:01Z

+ test("SPARK-48845: GenericUDF catch exceptions from child UDFs") {
+ withTable("test_catch_exception") {
+ Seq("9", "9-1").toDF("a").write.saveAsTable("test_catch_exception")
+ sql("create temporary function udf_exception as " +


let's wrap with withUserDefinedFunction to clean up the functions at the end.

panbingkun · 2024-07-12T00:02:16Z

Regarding the failure of GA, I guess you need to rebase master. 😄

…_exception_from_child_func

LuciferYang

LGTM

LuciferYang · 2024-07-12T03:10:47Z

@panbingkun any other comments?

panbingkun · 2024-07-12T03:22:47Z

any other comments?

No, go ahead.

…ThrowException.java

LuciferYang · 2024-07-12T07:25:14Z

[error] /home/runner/work/spark/spark/sql/hive/src/test/java/org/apache/spark/sql/hive/execution/UDFThrowException.java:24:1: error: incompatible types: int cannot be converted to String
[error] return Integer.parseInt(data);
[error]

@jackylee-ch

yaooqinn · 2024-07-12T07:26:57Z

Oh my bad @LuciferYang

### What changes were proposed in this pull request? This pr is trying to fix the syntax issues with GenericUDF since 3.5.0. The problem arose from DeferredObject currently passing a value instead of a function, which prevented users from catching exceptions in GenericUDF, resulting in semantic differences. Here is an example case we encountered. Originally, the semantics were that udf_exception would throw an exception, while udf_catch_exception could catch the exception and return a null value. However, currently, any exception encountered by udf_exception will cause the program to fail. ``` select udf_catch_exception(udf_exception(col1)) from table ``` ### Why are the changes needed? For before Spark 3.5, we directly made the GenericUDF's DeferredObject lazy and evaluated the children in `function.evaluate(deferredObjects)`. Now, we would run the children's code first. If an exception is thrown, we would make it lazy to GenericUDF's DeferredObject. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Newly added UT. ### Was this patch authored or co-authored using generative AI tooling? No. Closes#47268 from jackylee-ch/generic_udf_catch_exception_from_child_func. Lead-authored-by: jackylee-ch <lijunqing@baidu.com> Co-authored-by: Kent Yao <yao@apache.org> Signed-off-by: Kent Yao <yao@apache.org> (cherry picked from commit 236d957) Signed-off-by: Kent Yao <yao@apache.org>

yaooqinn · 2024-07-12T10:18:54Z

Thank you all.

Merged to master and branch-3.5 for the coming 3.5.2

jackylee-ch · 2024-07-12T11:00:50Z

Thanks for your review. @LuciferYang @yaooqinn @cloud-fan @panbingkun

### What changes were proposed in this pull request? This pr is trying to fix the syntax issues with GenericUDF since 3.5.0. The problem arose from DeferredObject currently passing a value instead of a function, which prevented users from catching exceptions in GenericUDF, resulting in semantic differences. Here is an example case we encountered. Originally, the semantics were that udf_exception would throw an exception, while udf_catch_exception could catch the exception and return a null value. However, currently, any exception encountered by udf_exception will cause the program to fail. ``` select udf_catch_exception(udf_exception(col1)) from table ``` ### Why are the changes needed? For before Spark 3.5, we directly made the GenericUDF's DeferredObject lazy and evaluated the children in `function.evaluate(deferredObjects)`. Now, we would run the children's code first. If an exception is thrown, we would make it lazy to GenericUDF's DeferredObject. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Newly added UT. ### Was this patch authored or co-authored using generative AI tooling? No. Closesapache#47268 from jackylee-ch/generic_udf_catch_exception_from_child_func. Lead-authored-by: jackylee-ch <lijunqing@baidu.com> Co-authored-by: Kent Yao <yao@apache.org> Signed-off-by: Kent Yao <yao@apache.org>

### What changes were proposed in this pull request? This pr is trying to fix the syntax issues with GenericUDF since 3.5.0. The problem arose from DeferredObject currently passing a value instead of a function, which prevented users from catching exceptions in GenericUDF, resulting in semantic differences. Here is an example case we encountered. Originally, the semantics were that udf_exception would throw an exception, while udf_catch_exception could catch the exception and return a null value. However, currently, any exception encountered by udf_exception will cause the program to fail. ``` select udf_catch_exception(udf_exception(col1)) from table ``` ### Why are the changes needed? For before Spark 3.5, we directly made the GenericUDF's DeferredObject lazy and evaluated the children in `function.evaluate(deferredObjects)`. Now, we would run the children's code first. If an exception is thrown, we would make it lazy to GenericUDF's DeferredObject. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Newly added UT. ### Was this patch authored or co-authored using generative AI tooling? No. Closes#47268 from jackylee-ch/generic_udf_catch_exception_from_child_func. Lead-authored-by: jackylee-ch <lijunqing@baidu.com> Co-authored-by: Kent Yao <yao@apache.org> Signed-off-by: Kent Yao <yao@apache.org> (cherry picked from commit 236d957) Signed-off-by: Kent Yao <yao@apache.org>

github-actionsBot added the SQL label Jul 9, 2024

generic udf catch exception from child udf
eb2b0a2

jackylee-ch force-pushed the generic_udf_catch_exception_from_child_func branch from f097982 to eb2b0a2Compare July 9, 2024 12:52

jackylee-ch added 2 commits July 9, 2024 20:54

Merge remote-tracking branch 'origin/master' into generic_udf_catch_e…
50e07fc
…xception_from_child_func

restart CI
5fc7497

jackylee-ch mentioned this pull request Jul 9, 2024
[SPARK-42051][SQL] Codegen Support for HiveGenericUDF #39555
Closed

panbingkun reviewed Jul 10, 2024
View reviewed changes

update ut for non-exception
66f7e08

yaooqinn approved these changes Jul 11, 2024
View reviewed changes

cloud-fan reviewed Jul 11, 2024
View reviewed changes

chagne non-codegen mode execution for GenericUDF
f8877e5

cloud-fan reviewed Jul 11, 2024
View reviewed changes

cloud-fan approved these changes Jul 11, 2024
View reviewed changes

add withUserDefinedFunction in UT
b7ed133

panbingkun reviewed Jul 11, 2024
View reviewed changes

Comment threadsql/hive/src/test/java/org/apache/spark/sql/hive/execution/UDFCatchException.java Outdated

panbingkun reviewed Jul 11, 2024
View reviewed changes

Comment threadsql/hive/src/test/java/org/apache/spark/sql/hive/execution/UDFException.java Outdated

panbingkun reviewed Jul 11, 2024
View reviewed changes

Comment threadsql/hive/src/test/java/org/apache/spark/sql/hive/execution/UDFException.java Outdated

jackylee-ch added 3 commits July 12, 2024 09:22

reformat code
0465984

Merge remote-tracking branch 'upstream/master' into generic_udf_catch…
6b1b661
…_exception_from_child_func

reformat license code
d3ef302

LuciferYang approved these changes Jul 12, 2024
View reviewed changes

panbingkun approved these changes Jul 12, 2024
View reviewed changes

yaooqinn reviewed Jul 12, 2024
View reviewed changes

Comment threadsql/hive/src/test/java/org/apache/spark/sql/hive/execution/UDFThrowException.java Outdated

Update sql/hive/src/test/java/org/apache/spark/sql/hive/execution/UDF…
0203bcc
…ThrowException.java

yaooqinn reviewed Jul 12, 2024
View reviewed changes

Comment threadsql/hive/src/test/java/org/apache/spark/sql/hive/execution/UDFThrowException.java Outdated

Update sql/hive/src/test/java/org/apache/spark/sql/hive/execution/UDF…
6c47f98
…ThrowException.java

fix code
c154823

yaooqinn closed this in 236d957Jul 12, 2024

jackylee-ch deleted the generic_udf_catch_exception_from_child_func branch July 12, 2024 11:00

Uh oh!

Conversation

jackylee-ch commented Jul 9, 2024• edited 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!

LuciferYang commented Jul 9, 2024

Uh oh!

cloud-fan commented Jul 9, 2024

Uh oh!

jackylee-ch commented Jul 9, 2024

Uh oh!

yaooqinn commented Jul 10, 2024

Uh oh!

jackylee-ch commented Jul 10, 2024

Uh oh!

panbingkunJul 10, 2024

Choose a reason for hiding this comment

Uh oh!

jackylee-chJul 10, 2024

Choose a reason for hiding this comment

Uh oh!

panbingkunJul 10, 2024• 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!

jackylee-chJul 10, 2024

Choose a reason for hiding this comment

Uh oh!

panbingkun commented Jul 10, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jackylee-ch commented Jul 10, 2024

Uh oh!

yaooqinn left a comment

Choose a reason for hiding this comment

Uh oh!

cloud-fanJul 11, 2024

Choose a reason for hiding this comment

Uh oh!

jackylee-chJul 11, 2024

Choose a reason for hiding this comment

Uh oh!

cloud-fanJul 11, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

panbingkun commented Jul 12, 2024

Uh oh!

LuciferYang left a comment

Choose a reason for hiding this comment

Uh oh!

LuciferYang commented Jul 12, 2024

Uh oh!

panbingkun commented Jul 12, 2024

Uh oh!

Uh oh!

Uh oh!

LuciferYang commented Jul 12, 2024

Uh oh!

yaooqinn commented Jul 12, 2024

Uh oh!

yaooqinn commented Jul 12, 2024

Uh oh!

jackylee-ch commented Jul 12, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jackylee-ch commented Jul 9, 2024•
edited
Loading

panbingkunJul 10, 2024•
edited
Loading

panbingkun commented Jul 10, 2024•
edited
Loading