[SPARK-35288][SQL] StaticInvoke should find the method without exact argument classes match #32413

viirya · 2021-05-01T20:43:09Z

What changes were proposed in this pull request?

This patch proposes to make StaticInvoke able to find method with given method name even the parameter types do not exactly match to argument classes.

Why are the changes needed?

Unlike Invoke, StaticInvoke only tries to get the method with exact argument classes. If the calling method's parameter types are not exactly matched with the argument classes, StaticInvoke cannot find the method.

StaticInvoke should be able to find the method under the cases too.

Does this PR introduce any user-facing change?

Yes. StaticInvoke can find a method even the argument classes are not exactly matched.

How was this patch tested?

Unit test.

viirya · 2021-05-01T20:45:28Z

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

@@ -236,7 +236,32 @@ case class StaticInvoke(
  override def children: Seq[Expression] = arguments

  lazy val argClasses = ScalaReflection.expressionJavaClasses(arguments)
-  @transient lazy val method = cls.getDeclaredMethod(functionName, argClasses : _*)
+  @transient lazy val method = {


This is duplicated with Invoke. But this will be backported, so I will leave the merging of method only to master branch.

Even though it's for backporting, shall we still avoid duplicated code and add a def findMethod(cls: Class[_]) in the base class?

SparkQA · 2021-05-01T21:30:58Z

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

SparkQA · 2021-05-01T21:30:59Z

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

maropu · 2021-05-01T22:46:48Z

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

+          sys.error(s"Couldn't find $functionName on $cls")
+        } else if (m.length > 1) {
+          // More than one matched method signature. Exclude synthetic one, e.g. generic one.
+          val realMethods = m.filter(!_.isSynthetic)


This check is needed for the static case, too?

Just test it. We can remove it.

...talyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ObjectExpressionsSuite.scala

maropu · 2021-05-01T23:07:13Z

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

-  @transient lazy val method = cls.getDeclaredMethod(functionName, argClasses : _*)
+  @transient lazy val method = {
+    try {
+      cls.getDeclaredMethod(functionName, argClasses: _*)


btw, java_method can support more cases via the same resolution logic?

Took a rough look at how java_method resolves the calling method. java_method only calls Java static method, but StaticInvoke covers Scala object method too. java_method's argument types seem only for catalyst primitive data types, so complext type and ObjectType is not supported.

We may extend java_method later.

SparkQA · 2021-05-02T01:08:46Z

Test build #138134 has finished for PR 32413 at commit cb7e903.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class AddJarsCommand(paths: Seq[String]) extends LeafRunnableCommand
case class AddFilesCommand(paths: Seq[String]) extends LeafRunnableCommand
case class AddArchivesCommand(paths: Seq[String]) extends LeafRunnableCommand

viirya · 2021-05-03T03:27:10Z

...talyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ObjectExpressionsSuite.scala

+    val input = (1, 2)
    checkObjectExprEvaluation(
-      Invoke(Literal(obj, clsType), "testFunc", IntegerType, Seq(Literal(1))), 0)
+      Invoke(Literal(obj, clsType), "testFunc", IntegerType,
+        Seq(Literal(input, ObjectType(input.getClass)))), 2)


Fix this test from SPARK-35278. Original one doesn't produce synthetic method. I may miss it when I changed the code.

HyukjinKwon

The changes look making sense to me

srowen · 2021-05-03T13:55:36Z

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

+          // Ambiguous case, we don't know which method to choose, just fail it.
+          sys.error(s"Found ${m.length} $functionName on $cls")
+        } else {
+          m.head


I get it, but this may not match. I understand that if I want to pass a String, then a method taking an Object "works" but a method taking a Long does not. Does that just cause other problems, or, are we assuming that because it already fails, we just get a different failure, so whatever? you could inspect the args to see if each isAssignableFrom or something I suppose, but maybe not worth it

I guess we work best here to find a method for given method name and arguments. For a method taking Object, seems it works to take a String. Adding isAssignableFrom check may work for some cases to catch a failure earily. Although as I test, isAssignableFrom is unable to catch the case if argument class is int and parameter type is Object too.

As I plan to backport this, for further improvement I will leave for master only. This change is only for the missing case. Is any update I should do for this change?

I think it's fine to stay conservative for a change you will backport. This will only cause cases that failed completely to possibly work now, right? so I think that's OK.

I think it's OK for 2.4.8. It will only cause code that failed outright before to possibly succeed, right? that's OK then as a fix.

SparkQA · 2021-05-05T02:23:05Z

Test build #138155 has finished for PR 32413 at commit d1aa94d.

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

SparkQA · 2021-05-05T03:17:33Z

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

SparkQA · 2021-05-05T03:24:21Z

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

viirya · 2021-05-05T21:30:54Z

retest this please

dongjoon-hyun

+1, LGTM. Thank you, @viirya .

SparkQA · 2021-05-05T22:38:01Z

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

SparkQA · 2021-05-05T22:38:02Z

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

viirya · 2021-05-06T01:20:10Z

Thanks @dongjoon-hyun @srowen. @maropu @cloud-fan Need to take another look? I may merge this before this week and backport so I can begin to cut RC4 of 2.4.8.

dongjoon-hyun · 2021-05-06T01:39:04Z

I think you can merge this, @viirya . All comments seems to be addressed (including this, #32413 (comment))

dongjoon-hyun · 2021-05-06T01:40:41Z

Also, it's okay to wait for @cloud-fan 's comment if this is not that urgent.

viirya · 2021-05-06T01:56:47Z

Thanks @dongjoon-hyun @srowen @maropu. Let me wait more a few days. I will merge before the end of this week.

cloud-fan · 2021-05-06T02:17:09Z

shall we consider #32404 (comment) ?

SparkQA · 2021-05-06T02:31:45Z

Test build #138181 has finished for PR 32413 at commit d1aa94d.

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

viirya · 2021-05-06T03:58:02Z

shall we consider #32404 (comment) ?

It looks promising. Do you think we should use it even in backports? Or just use it in master only?

viirya · 2021-05-06T06:53:41Z

@cloud-fan If we decide to leave using of the util method in master, this is pretty conservative fix, shall we backport this, and then open new PRs for util method in master?

cloud-fan

LGTM except for one comment: https://github.com/apache/spark/pull/32413/files#r627148233

viirya · 2021-05-07T03:23:44Z

Added the shared method.

viirya · 2021-05-07T03:57:13Z

Thanks @cloud-fan @dongjoon-hyun. I will merge once CI passes.

SparkQA · 2021-05-07T04:06:51Z

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

SparkQA · 2021-05-07T04:06:52Z

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

SparkQA · 2021-05-07T07:59:55Z

Test build #138230 has finished for PR 32413 at commit 0ec8117.

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

viirya · 2021-05-07T16:07:11Z

Thanks al! Merging to master/3.1/3.0/2.4.

…argument classes match ### What changes were proposed in this pull request? This patch proposes to make StaticInvoke able to find method with given method name even the parameter types do not exactly match to argument classes. ### Why are the changes needed? Unlike `Invoke`, `StaticInvoke` only tries to get the method with exact argument classes. If the calling method's parameter types are not exactly matched with the argument classes, `StaticInvoke` cannot find the method. `StaticInvoke` should be able to find the method under the cases too. ### Does this PR introduce _any_ user-facing change? Yes. `StaticInvoke` can find a method even the argument classes are not exactly matched. ### How was this patch tested? Unit test. Closes #32413 from viirya/static-invoke. Authored-by: Liang-Chi Hsieh <viirya@gmail.com> Signed-off-by: Liang-Chi Hsieh <viirya@gmail.com> (cherry picked from commit 33fbf56) Signed-off-by: Liang-Chi Hsieh <viirya@gmail.com>

viirya · 2021-05-07T16:12:57Z

Then I will prepare a master only PR to use the util method.

…argument classes match ### What changes were proposed in this pull request? This patch proposes to make StaticInvoke able to find method with given method name even the parameter types do not exactly match to argument classes. ### Why are the changes needed? Unlike `Invoke`, `StaticInvoke` only tries to get the method with exact argument classes. If the calling method's parameter types are not exactly matched with the argument classes, `StaticInvoke` cannot find the method. `StaticInvoke` should be able to find the method under the cases too. ### Does this PR introduce _any_ user-facing change? Yes. `StaticInvoke` can find a method even the argument classes are not exactly matched. ### How was this patch tested? Unit test. Closes apache#32413 from viirya/static-invoke. Authored-by: Liang-Chi Hsieh <viirya@gmail.com> Signed-off-by: Liang-Chi Hsieh <viirya@gmail.com> (cherry picked from commit 33fbf56) Signed-off-by: Liang-Chi Hsieh <viirya@gmail.com>

viirya added 8 commits April 29, 2021 20:42

Invoke should find the method with correct number of parameters

3829aaa

Use getMethod.

9fc152c

Revert "Use getMethod."

8eed4d1

Improve function lookup.

d03318a

Consider generic method.

176b103

Add test.

bfaaf32

StaticInvoke should find methid without exact param type match.

ec57a98

Merge remote-tracking branch 'upstream/master' into static-invoke

cb7e903

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

viirya commented May 1, 2021

View reviewed changes

maropu reviewed May 1, 2021

View reviewed changes

dongjoon-hyun reviewed May 1, 2021

View reviewed changes

...talyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ObjectExpressionsSuite.scala Outdated Show resolved Hide resolved

maropu reviewed May 1, 2021

View reviewed changes

Address comments.

d1aa94d

viirya commented May 3, 2021

View reviewed changes

HyukjinKwon reviewed May 3, 2021

View reviewed changes

srowen reviewed May 3, 2021

View reviewed changes

dongjoon-hyun approved these changes May 5, 2021

View reviewed changes

maropu approved these changes May 6, 2021

View reviewed changes

viirya mentioned this pull request May 6, 2021

[SPARK-35278][SQL] Invoke should find the method with correct number of parameters #32404

Closed

cloud-fan approved these changes May 6, 2021

View reviewed changes

Add method.

0ec8117

cloud-fan approved these changes May 7, 2021

View reviewed changes

dongjoon-hyun approved these changes May 7, 2021

View reviewed changes

viirya closed this in 33fbf56 May 7, 2021

viirya deleted the static-invoke branch December 27, 2023 18:25

[SPARK-35288][SQL] StaticInvoke should find the method without exact argument classes match #32413

[SPARK-35288][SQL] StaticInvoke should find the method without exact argument classes match #32413

Conversation

viirya commented May 1, 2021 • edited Loading

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?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented May 1, 2021

SparkQA commented May 1, 2021

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

SparkQA commented May 2, 2021

Choose a reason for hiding this comment

HyukjinKwon left a 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

SparkQA commented May 5, 2021

SparkQA commented May 5, 2021

SparkQA commented May 5, 2021

viirya commented May 5, 2021

dongjoon-hyun left a comment

Choose a reason for hiding this comment

SparkQA commented May 5, 2021

SparkQA commented May 5, 2021

viirya commented May 6, 2021

dongjoon-hyun commented May 6, 2021 • edited Loading

dongjoon-hyun commented May 6, 2021

viirya commented May 6, 2021

cloud-fan commented May 6, 2021

SparkQA commented May 6, 2021

viirya commented May 6, 2021

viirya commented May 6, 2021

cloud-fan left a comment

Choose a reason for hiding this comment

viirya commented May 7, 2021

viirya commented May 7, 2021

SparkQA commented May 7, 2021

SparkQA commented May 7, 2021

SparkQA commented May 7, 2021

viirya commented May 7, 2021

viirya commented May 7, 2021

viirya commented May 1, 2021 •

edited

Loading

dongjoon-hyun commented May 6, 2021 •

edited

Loading