[SPARK-35347][SQL] Use MethodUtils for looking up methods in Invoke and StaticInvoke #32474

viirya · 2021-05-08T06:05:16Z

What changes were proposed in this pull request?

This patch proposes to use MethodUtils for looking up methods Invoke and StaticInvoke expressions.

Why are the changes needed?

Currently we wrote our logic in Invoke and StaticInvoke expressions for looking up methods. It is tricky to consider all the cases and there is already existing utility package for this purpose. We should reuse the utility package.

Does this PR introduce any user-facing change?

No, internal change only.

How was this patch tested?

Existing tests.

SparkQA · 2021-05-08T07:02:42Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/42802/

SparkQA · 2021-05-08T07:07:37Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/42802/

SparkQA · 2021-05-08T10:51:31Z

Test build #138280 has finished for PR 32474 at commit 7b96cb2.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

dongjoon-hyun · 2021-05-08T11:53:40Z

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

-        } else {
-          m.head
-        }
+    val method = MethodUtils.getMatchingAccessibleMethod(cls, functionName, argClasses: _*)


Technically, this has more code like setAccessibleWorkaround. Let's trigger JDK11 test.

dongjoon-hyun · 2021-05-08T11:54:28Z

Retest this please

SparkQA · 2021-05-08T12:21:19Z

Test build #138291 has finished for PR 32474 at commit 7b96cb2.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2021-05-08T12:50:54Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/42813/

SparkQA · 2021-05-08T12:51:51Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/42813/

viirya · 2021-05-08T16:52:12Z

retest this please

SparkQA · 2021-05-08T17:24:00Z

Test build #138297 has finished for PR 32474 at commit 7b96cb2.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

viirya · 2021-05-08T17:36:35Z

[error] java.io.IOException: Cannot run program "/usr/java/jdk-11.0.1/bin/java" (in directory "/home/jenkins/workspace/SparkPullRequestBuilder/sql/hive-thriftserver"): error=2, No such file or directory

Seems something is wrong with Jenkins?

SparkQA · 2021-05-08T17:49:09Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/42819/

SparkQA · 2021-05-08T17:49:10Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/42819/

dongjoon-hyun · 2021-05-08T20:19:47Z

[error] java.io.IOException: Cannot run program "/usr/java/jdk-11.0.1/bin/java" (in directory "/home/jenkins/workspace/SparkPullRequestBuilder/sql/hive-thriftserver"): error=2, No such file or directory

Seems something is wrong with Jenkins?

Oh, it seems that UCB Amplab Jenkins lab still didn't finish the setup. In that case, let's ignore Jenkins.

Please re-trigger your GitHub Action. It also failing at Java11 compilation. In this case, it looks like a flaky issue.

If you don't mind, please run catalyst module with JDK11 and share the result with us, @viirya .

After transition to run in yours' fork, only the author can re-trigger it.

viirya · 2021-05-08T21:51:41Z

Thanks @dongjoon-hyun. Re-triggered GA and the Java 11 build was passed.

dongjoon-hyun · 2021-05-08T22:07:01Z

Thanks. And, did you run catalyst test with Java 11 locally?

dongjoon-hyun · 2021-05-08T22:16:19Z

I tested it.

[info] All tests passed.
[info] Passed: Total 5175, Failed 0, Errors 0, Passed 5175, Ignored 5, Canceled 1
[success] Total time: 552 s (09:12), completed May 8, 2021, 3:15:52 PM
build/sbt "catalyst/test"  2423.75s user 77.89s system 432% cpu 9:38.15 total

SPARK-PR-32474:SPARK-PR-32474 $ java -version
openjdk version "11.0.11" 2021-04-20 LTS

dongjoon-hyun

+1, LGTM. Merged to master for Apache Spark 3.2.0. Thank you, @viirya .

viirya · 2021-05-08T23:38:12Z

Thank you @dongjoon-hyun!

cloud-fan · 2021-05-12T02:37:04Z

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

-        }
+    val method = MethodUtils.getMatchingAccessibleMethod(cls, functionName, argClasses: _*)
+    if (method == null) {
+      sys.error(s"Couldn't find $functionName on $cls")


shall we throw an exception to only fail the query, instead of sys.error which crashes the driver JVM?

ok, will submit a follow-up.

…on type when cannot find the method instead of sys.error ### What changes were proposed in this pull request? A simple follow-up of #32474 to throw exception instead of sys.error. ### Why are the changes needed? An exception only fails the query, instead of sys.error. ### Does this PR introduce _any_ user-facing change? Yes, if `Invoke` or `StaticInvoke` cannot find the method, instead of original `sys.error` now we only throw an exception. ### How was this patch tested? Existing tests. Closes #32519 from viirya/SPARK-35347-followup. Authored-by: Liang-Chi Hsieh <[email protected]> Signed-off-by: Liang-Chi Hsieh <[email protected]>

Use MethodUtils instead of manually written logic.

7b96cb2

github-actions bot added the SQL label May 8, 2021

dongjoon-hyun reviewed May 8, 2021

View reviewed changes

dongjoon-hyun changed the title ~~[SPARK-35347][SQL] Use MethodUtils for looking up methods in Invoke and StaticInvoke~~ [SPARK-35347][SQL][test-java11] Use MethodUtils for looking up methods in Invoke and StaticInvoke May 8, 2021

dongjoon-hyun changed the title ~~[SPARK-35347][SQL][test-java11] Use MethodUtils for looking up methods in Invoke and StaticInvoke~~ [SPARK-35347][SQL] Use MethodUtils for looking up methods in Invoke and StaticInvoke May 8, 2021

dongjoon-hyun approved these changes May 8, 2021

View reviewed changes

dongjoon-hyun closed this in 5b65d8a May 8, 2021

viirya deleted the invoke-util branch May 8, 2021 23:38

cloud-fan reviewed May 12, 2021

View reviewed changes

viirya mentioned this pull request May 12, 2021

[SPARK-35347][SQL][FOLLOWUP] Throw exception with an explicit exception type when cannot find the method instead of sys.error #32519

Closed

[SPARK-35347][SQL] Use MethodUtils for looking up methods in Invoke and StaticInvoke #32474

[SPARK-35347][SQL] Use MethodUtils for looking up methods in Invoke and StaticInvoke #32474

Uh oh!

Conversation

viirya commented May 8, 2021

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?

Uh oh!

SparkQA commented May 8, 2021

Uh oh!

SparkQA commented May 8, 2021

Uh oh!

SparkQA commented May 8, 2021

Uh oh!

dongjoon-hyun May 8, 2021

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun commented May 8, 2021

Uh oh!

SparkQA commented May 8, 2021

Uh oh!

SparkQA commented May 8, 2021

Uh oh!

SparkQA commented May 8, 2021

Uh oh!

viirya commented May 8, 2021

Uh oh!

SparkQA commented May 8, 2021

Uh oh!

viirya commented May 8, 2021

Uh oh!

SparkQA commented May 8, 2021

Uh oh!

SparkQA commented May 8, 2021

Uh oh!

dongjoon-hyun commented May 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

viirya commented May 8, 2021

Uh oh!

dongjoon-hyun commented May 8, 2021

Uh oh!

dongjoon-hyun commented May 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dongjoon-hyun left a comment

Choose a reason for hiding this comment

Uh oh!

viirya commented May 8, 2021

Uh oh!

cloud-fan May 12, 2021

Choose a reason for hiding this comment

Uh oh!

viirya May 12, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dongjoon-hyun commented May 8, 2021 •

edited

Loading

dongjoon-hyun commented May 8, 2021 •

edited

Loading