[SPARK-35278][SQL] Invoke should find the method with correct number of parameters by viirya · Pull Request #32404 · apache/spark

viirya · 2021-04-30T03:46:22Z

What changes were proposed in this pull request?

This patch fixes Invoke expression when the target object has more than one method with the given method name.

Why are the changes needed?

Invoke will find out the method on the target object with given method name. If there are more than one method with the name, currently it is undeterministic which method will be used. We should add the condition of parameter number when finding the method.

Does this PR introduce any user-facing change?

Yes, fixed a bug when using Invoke on a object where more than one method with the given method name.

How was this patch tested?

Unit test.

viirya · 2021-04-30T03:54:26Z

cc @cloud-fan @maropu

dongjoon-hyun

+1, LGTM. Nice catch!
Thank you, @viirya .

SparkQA · 2021-04-30T04:30:06Z

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

SparkQA · 2021-04-30T04:34:21Z

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

sunchao · 2021-04-30T04:42:59Z

 case ObjectType(cls) =>
- val m = cls.getMethods.find(_.getName == encodedFunctionName)
+ val m = cls.getMethods.find { m =>
+ m.getName == encodedFunctionName && m.getParameterCount == arguments.length


should we check parameter types too? also curious why we don't use cls.getMethod for the purpose.

+1 to use getMethod

Actually Invoke uses getMethod for target object with data types other than ObjectType. I'm not sure why we need to do a separate getMethod during evaluation for the cases.

And note method uses encodedFunctionName to find method, but in eval the original functionName is used.

It should be safer to keep original pattern. But switched to use getMethod and see what CI tells.

It looks like a very ancient bug from Spark 1.x.

dongjoon-hyun

BTW, @viirya . According to the Affected Version of SPARK-35278, are we going to cut another RC (Apache Spark 2.4.8 RC4) for this?

SparkQA · 2021-04-30T07:08:09Z

Test build #138103 has finished for PR 32404 at commit 9fc152c.

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

viirya · 2021-04-30T07:13:18Z

Caused by: sbt.ForkMain$ForkError: java.lang.RuntimeException: Couldn't find apply on interface scala.Function1
at scala.sys.package$.error(package.scala:30)
at org.apache.spark.sql.catalyst.expressions.objects.Invoke.liftedTree1$1(objects.scala:333)
at org.apache.spark.sql.catalyst.expressions.objects.Invoke.method$lzycompute(objects.scala:329)
at org.apache.spark.sql.catalyst.expressions.objects.Invoke.method(objects.scala:327)
at org.apache.spark.sql.catalyst.expressions.objects.Invoke.doGenCode(objects.scala:359)
at org.apache.spark.sql.catalyst.expressions.Expression.$anonfun$genCode$3(Expression.scala:147)

Switched to getMethod seems causing some test failures.

viirya · 2021-04-30T07:15:44Z

BTW, @viirya . According to the Affected Version of SPARK-35278, are we going to cut another RC (Apache Spark 2.4.8 RC4) for this?

Ideally, yes. Though seems no one complains this, and this is an ancient bug...

SparkQA · 2021-04-30T07:36:23Z

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

SparkQA · 2021-04-30T07:36:24Z

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

viirya · 2021-04-30T07:44:02Z

Let me know if you prefer to skip 2.4 backport.

viirya · 2021-04-30T07:54:35Z

Oh, the problem of using getMethod is for some cases, the parameter types is not known in advance. Like the failed tests, the argument types from arguments is java.lang.Object. So we cannot reliably using name + parameter types to get method.

SparkQA · 2021-04-30T08:16:43Z

Test build #138097 has finished for PR 32404 at commit 3829aaa.

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

cloud-fan · 2021-04-30T08:41:17Z

 @transient lazy val method = targetObject.dataType match {
 case ObjectType(cls) =>
- val m = cls.getMethods.find(_.getName == encodedFunctionName)
+ val m = cls.getMethods.find { m =>


shall we use filter instead of find, and fail if more than one match is found?

How about:
getMethod with param types
If not found, turn to find one method with parameter number
If more than one is found, or no one can be found, fails it

+1 for that improved one.

+1. Now I'm wondering whether StaticInvoke has the same issue since it uses getDeclaredMethod.

StaticInvoke calls getDeclaredMethod with argClasses. I think it may also suffer the issue if the arg class is Object.
Let me see if I can come out a test case and fix it (might be in other PR).
As said above, I think it is safer to try to combine two method looking up in master only.

SparkQA · 2021-04-30T09:23:23Z

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

SparkQA · 2021-04-30T09:27:37Z

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

SparkQA · 2021-04-30T13:00:33Z

Test build #138107 has finished for PR 32404 at commit 8eed4d1.

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

dongjoon-hyun · 2021-04-30T15:30:02Z

Let me know if you prefer to skip 2.4 backport.

For that, it's totally up to your decision, @viirya , because you are the release manager of Apache Spark 2.4.8. I support your decision.

SparkQA · 2021-04-30T20:23:31Z

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

SparkQA · 2021-04-30T20:27:58Z

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

SparkQA · 2021-05-01T04:25:28Z

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

SparkQA · 2021-05-01T04:25:29Z

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

SparkQA · 2021-05-01T05:43:11Z

Test build #138121 has finished for PR 32404 at commit 176b103.

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

maropu · 2021-05-01T05:42:37Z

+ sys.error(s"Couldn't find $encodedFunctionName 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)


We cannot filter out synthetic ones in L336?

I don't know if it is possible that there is only one method and it is also a synthetic one. If we filter synthetic ones at L336, we may miss it?

maropu · 2021-05-01T05:47:55Z

Looks fine otherwise.

SparkQA · 2021-05-01T08:16:31Z

Test build #138122 has finished for PR 32404 at commit bfaaf32.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
abstract class BaseClass[T]
class ConcreteClass extends BaseClass[Int] with Serializable

viirya · 2021-05-01T17:20:16Z

Thanks @cloud-fan @dongjoon-hyun @maropu. Merging to master/3.1/3.0.

There is conflicts in 2.4. Will manually backport.

…of parameters ### What changes were proposed in this pull request? This patch fixes `Invoke` expression when the target object has more than one method with the given method name. ### Why are the changes needed? `Invoke` will find out the method on the target object with given method name. If there are more than one method with the name, currently it is undeterministic which method will be used. We should add the condition of parameter number when finding the method. ### Does this PR introduce _any_ user-facing change? Yes, fixed a bug when using `Invoke` on a object where more than one method with the given method name. ### How was this patch tested? Unit test. Closes#32404 from viirya/verify-invoke-param-len. Authored-by: Liang-Chi Hsieh <viirya@gmail.com> Signed-off-by: Liang-Chi Hsieh <viirya@gmail.com> (cherry picked from commit 6ce1b16) Signed-off-by: Liang-Chi Hsieh <viirya@gmail.com>

…mber of parameters ### What changes were proposed in this pull request? This patch fixes `Invoke` expression when the target object has more than one method with the given method name. This is 2.4 backport of #32404. ### Why are the changes needed? `Invoke` will find out the method on the target object with given method name. If there are more than one method with the name, currently it is undeterministic which method will be used. We should add the condition of parameter number when finding the method. ### Does this PR introduce _any_ user-facing change? Yes, fixed a bug when using `Invoke` on a object where more than one method with the given method name. ### How was this patch tested? Unit test. Closes#32412 from viirya/SPARK-35278-2.4. Authored-by: Liang-Chi Hsieh <viirya@gmail.com> Signed-off-by: Liang-Chi Hsieh <viirya@gmail.com>

dongjoon-hyun · 2021-05-01T22:51:50Z

+1, late LGTM with the last commit.

cloud-fan · 2021-05-05T02:50:44Z

@viirya can we look into which callers do not provide the concrete parameter types when building Invoke? I think it's better to fix them, so that we can do a proper method resolution in Invoke, as a Java class may have several methods with the same name and number of arguments, but different argument types.

viirya · 2021-05-05T03:18:38Z

@cloud-fan Okay. Let me find them out and fix them.

viirya · 2021-05-05T03:32:20Z

@cloud-fan Just to confirm. So you mean we only allow Invoke to run the method with exact parameter type matches, right?

For example, calling a method func(input: Object) with argument Tuple2 will be disallowed.

viirya · 2021-05-05T08:20:02Z

@cloud-fan Seems to me we still need to support it. One example is UserDefinedType.

For example, VectorUDT extends UserDefinedType[Vector]. VectorUDT is responsible for serialization of dense and sparse vectors. It overrides def serialize(obj: Vector) that takes both SparseVector and DenseVector as input parameters.

cloud-fan · 2021-05-06T02:15:01Z

For example, calling a method func(input: Object) with argument Tuple2 will be disallowed.

This should be allowed, because we do provide the concrete parameter type (Tuple2). What should be forbidden is something like func(s: String) and the parameter type is Object.

I think I get your point, we should probably use a util function to look up the method, instead of writing the code by ourselves, to handle cases like search the inheritance hierarchy, argument type casting, etc.

How about https://commons.apache.org/proper/commons-lang/apidocs/org/apache/commons/lang3/reflect/MethodUtils.html#getMatchingMethod-java.lang.Class-java.lang.String-java.lang.Class...-

viirya · 2021-05-06T04:05:30Z

For example, calling a method func(input: Object) with argument Tuple2 will be disallowed.
This should be allowed, because we do provide the concrete parameter type (Tuple2). What should be forbidden is something like func(s: String) and the parameter type is Object.
I think I get your point, we should probably use a util function to look up the method, instead of writing the code by ourselves, to handle cases like search the inheritance hierarchy, argument type casting, etc.
How about https://commons.apache.org/proper/commons-lang/apidocs/org/apache/commons/lang3/reflect/MethodUtils.html#getMatchingMethod-java.lang.Class-java.lang.String-java.lang.Class...-

It looks promising. Not aware there is an util function we can use. So my question would be should we use it in backports? Or only replacing with our code with the util in master only?

cloud-fan · 2021-05-06T06:33:34Z

I think this PR itself is pretty conservative and is good to backport. The new code is clearly better than the old code without regression. I'd suggest to only change to the util method in the master branch, for both invoke and static invoke.

cloud-fan · 2021-05-06T06:35:22Z

BTW, AFAIK invoke and static invoke are only used by internal places such as the encoder and UDT. It's not really a bug for <= 3.1, but a bug for 3.2 as we use it to execute the new UDF.

viirya · 2021-05-06T06:46:59Z

I think this PR itself is pretty conservative and is good to backport. The new code is clearly better than the old code without regression. I'd suggest to only change to the util method in the master branch, for both invoke and static invoke.

I see. Then I don't touch invoke in old branches again and keep this backport as it is. Let's use the util method in master only.

viirya · 2021-05-06T06:52:02Z

BTW, AFAIK invoke and static invoke are only used by internal places such as the encoder and UDT. It's not really a bug for <= 3.1, but a bug for 3.2 as we use it to execute the new UDF.

Rarely but maybe possibly developers will use invoke/static invoke for custom encoder? As this is already backport to 2.4/3.0/3.1, I think it should be fine to keep as it is? For static invoke #32413, it is also conservative, so I guess it may be also good for backport?

cloud-fan · 2021-05-06T07:15:44Z

For static invoke #32413, it is also conservative, so I guess it may be also good for backport?

Yea I think so

…of parameters This patch fixes `Invoke` expression when the target object has more than one method with the given method name. `Invoke` will find out the method on the target object with given method name. If there are more than one method with the name, currently it is undeterministic which method will be used. We should add the condition of parameter number when finding the method. Yes, fixed a bug when using `Invoke` on a object where more than one method with the given method name. Unit test. Closesapache#32404 from viirya/verify-invoke-param-len. Authored-by: Liang-Chi Hsieh <viirya@gmail.com> Signed-off-by: Liang-Chi Hsieh <viirya@gmail.com> (cherry picked from commit 6ce1b16) Signed-off-by: Liang-Chi Hsieh <viirya@gmail.com>

…of parameters ### What changes were proposed in this pull request? This patch fixes `Invoke` expression when the target object has more than one method with the given method name. ### Why are the changes needed? `Invoke` will find out the method on the target object with given method name. If there are more than one method with the name, currently it is undeterministic which method will be used. We should add the condition of parameter number when finding the method. ### Does this PR introduce _any_ user-facing change? Yes, fixed a bug when using `Invoke` on a object where more than one method with the given method name. ### How was this patch tested? Unit test. Closesapache#32404 from viirya/verify-invoke-param-len. Authored-by: Liang-Chi Hsieh <viirya@gmail.com> Signed-off-by: Liang-Chi Hsieh <viirya@gmail.com> (cherry picked from commit 6ce1b16) Signed-off-by: Liang-Chi Hsieh <viirya@gmail.com>

Invoke should find the method with correct number of parameters
3829aaa

github-actionsBot added the SQL label Apr 30, 2021

dongjoon-hyun approved these changes Apr 30, 2021
View reviewed changes

sunchao reviewed Apr 30, 2021
View reviewed changes

Use getMethod.
9fc152c

cloud-fan reviewed Apr 30, 2021
View reviewed changes

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

dongjoon-hyun reviewed Apr 30, 2021
View reviewed changes

cloud-fan approved these changes Apr 30, 2021
View reviewed changes

Revert "Use getMethod."
8eed4d1

cloud-fan reviewed Apr 30, 2021
View reviewed changes

maropu approved these changes Apr 30, 2021
View reviewed changes

dongjoon-hyun mentioned this pull request Apr 30, 2021
[SPARK-35280][K8S] Promote KubernetesUtils to DeveloperApi #32406
Closed

Improve function lookup.
d03318a

cloud-fan approved these changes May 1, 2021
View reviewed changes

maropu reviewed May 1, 2021
View reviewed changes

viirya closed this in 6ce1b16May 1, 2021

viirya mentioned this pull request May 1, 2021
[SPARK-35278][SQL][2.4] Invoke should find the method with correct number of parameters #32412
Closed

cloud-fan mentioned this pull request May 6, 2021
[SPARK-35288][SQL] StaticInvoke should find the method without exact argument classes match #32413
Closed

viirya deleted the verify-invoke-param-len branch December 27, 2023 18:25

Uh oh!

Conversation

viirya commented Apr 30, 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!

viirya commented Apr 30, 2021

Uh oh!

dongjoon-hyun left a comment

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Apr 30, 2021

Uh oh!

SparkQA commented Apr 30, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

viiryaApr 30, 2021• 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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dongjoon-hyun left a comment

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Apr 30, 2021

Uh oh!

viirya commented Apr 30, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

viirya commented Apr 30, 2021

Uh oh!

SparkQA commented Apr 30, 2021

Uh oh!

SparkQA commented Apr 30, 2021

Uh oh!

viirya commented Apr 30, 2021

Uh oh!

viirya commented Apr 30, 2021

Uh oh!

SparkQA commented Apr 30, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Apr 30, 2021

Uh oh!

SparkQA commented Apr 30, 2021

Uh oh!

SparkQA commented Apr 30, 2021

Uh oh!

dongjoon-hyun commented Apr 30, 2021

Uh oh!

SparkQA commented Apr 30, 2021

Uh oh!

SparkQA commented Apr 30, 2021

Uh oh!

SparkQA commented May 1, 2021

Uh oh!

SparkQA commented May 1, 2021

Uh oh!

viiryaApr 30, 2021•
edited
Loading

viirya commented Apr 30, 2021•
edited
Loading

viirya commented May 1, 2021•
edited
Loading