[SPARK-16284][SQL] Implement reflect SQL function by petermaxlee · Pull Request #13969 · apache/spark

petermaxlee · 2016-06-29T09:19:22Z

What changes were proposed in this pull request?

This patch implements reflect SQL function, which can be used to invoke a Java method in SQL. Slightly different from Hive, this implementation requires the class name and the method name to be literals. This implementation also supports only a smaller number of data types.

java_method is an alias for reflect, so this should also resolve SPARK-16277.

How was this patch tested?

Added expression unit tests and an end-to-end test.

petermaxlee · 2016-06-29T09:19:39Z

cc @rxin @cloud-fan @dongjoon-hyun

cloud-fan · 2016-06-29T09:24:46Z

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

is it similar to StaticInvoke?

Thanks for pointing out. It looks similar, but has some subtle differences:

This one can invoke non-static methods.

This one does type conversion, and as a result is more user facing. StaticInvoke seems to be used in internal implementations?

This is a SQL function - why was StaticInvoke a "nonSQL" function?

This one supports non-codegen.

Perhaps we can push this in, and I can look into whether it'd make sense to consolidate the two?

this one can invoke non-static methods? How do we pass in the object reference?

I'm ok to leave them separated as this is one is userfacing and StaticInvoke is used internally.

It assumes there is a no-arg constructor and creates an instance of the class automatically. That's what reflect does in Hive.

SparkQA · 2016-06-29T11:28:35Z

Test build #3146 has finished for PR 13969 at commit dfe3480.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- case class JavaMethodReflect(children: Seq[Expression])

SparkQA · 2016-06-30T07:16:55Z

Test build #3152 has finished for PR 13969 at commit 0e43c95.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- case class JavaMethodReflect(children: Seq[Expression])

cloud-fan · 2016-06-30T14:51:38Z

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

What's hive's rule? This looks reasonable but I wanna make sure we don't miss anything.

Hive follows the same thing for the subset we are supporting here. Hive however also supports timestamps, decimals, etc, that this one is not supporting yet.

petermaxlee · 2016-07-06T06:51:53Z

I brought this up to date. Any comments on the pull request?

cloud-fan · 2016-07-06T06:56:50Z

cc @dongjoon-hyun to take a look

dongjoon-hyun · 2016-07-06T09:25:38Z

Oh, sure. @cloud-fan .

dongjoon-hyun · 2016-07-06T09:36:51Z

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

You can use eval() instead of eval(null).

dongjoon-hyun · 2016-07-06T10:28:38Z

By the way, could we change the expression name?
I know the reason why you choose JavaMethodReflect, but we can call Scala functions, too.

select reflect('scala.Console','println', 'a')

I prefer something simple like just case class Reflect. How do you think about this, @cloud-fan and @petermaxlee ?

dongjoon-hyun · 2016-07-06T10:42:42Z

In general, the noticeable big difference from Hive seems to be the limitation on first and second arguments. Hive supports columns for them, too.
IMHO, we can achieve that by putting all the @transient private lazy val into eval function and moving the following feature into findMethod or other places.

    } else if (method == null) {
      TypeCheckFailure("cannot find a method that matches the argument types")

dongjoon-hyun · 2016-07-06T10:43:08Z

Sorry for late to the party. I've done my first pass.
This PR looks very valuable and nice.

rxin · 2016-07-06T17:44:29Z

@dongjoon-hyun it's ok to not support the hive case here. Majority of the use cases will be calling some literal functions anyway.

dongjoon-hyun · 2016-07-06T17:57:05Z

Oh, if then, no problem. :)

petermaxlee · 2016-07-07T06:29:51Z

JavaReflectMethod isn't the best, but calling it Reflect is pretty bad because there are many "Reflect" classes in various libraries -- just within Spark dependencies there are 3 classes called Reflect.

petermaxlee · 2016-07-07T06:51:29Z

Alright - I've updated it. The expression is now called Reflect, and it prints different messages depending on whether it is a class not found or a method not found.

petermaxlee · 2016-07-08T00:44:35Z

Ping!

dongjoon-hyun · 2016-07-08T08:37:53Z

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

Could you add a single space after newline, e.g. '\n->\n `?
In many cases, we do that.

dongjoon-hyun · 2016-07-08T09:02:02Z

Oh, sorry, @petermaxlee .
It seems we are calling Utils.classForName(className) three places.
Could we reduce the number of calling? I'll leave a comment about this in code, too.

dongjoon-hyun · 2016-07-08T09:04:46Z

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

Currently, this is a boolean. Can we use this for val clazz: Class[_] instead?
For false, it could be null.

What I mean is the following.

- } else if (!classExists) { + } else if (clazz.getOrElse(null) == null) { ... - @transient private lazy val classExists = Reflect.classExists(className) + @transient private lazy val clazz = Reflect.findClass(className) ... - private def classExists(className: String): Boolean = { ... } + private def findClass(className: String): Try[Class[_]] = Try(Utils.classForName(className)) ... - Reflect.findMethod(className, methodName, ... + Reflect.findMethod(clazz.get, methodName, ... ... - Reflect.instantiate(className).orNull.asInstanceOf[Object] + Reflect.instantiate(clazz.get).orNull.asInstanceOf[Object] ... - def findMethod(className: String, methodName: String, ... + def findMethod(clazz: Class[_], methodName: String, ... ... - def instantiate(className: String): Option[Any] = { + def instantiate(clazz: Class[_]): Option[Any] = {

It'd make unit test more annoying to write. I kind of prefer doing it this way, since the cost of creating a class 3 times is very small given it's created only once.

Let's forget about Try. It's not a good style, too.

BTW, do you mean Utils.classForName is called once in this PR?

given it's created only once.

hvanhovell · 2016-07-08T13:43:38Z

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

What about timestamps, dates, decimals, arrays, maps? I suppose structs are entirely out of the question? If they are please document this.

Let me add a comment saying only string is supported for now.

hvanhovell · 2016-07-08T14:38:07Z

@petermaxlee are you sure that we shouldn't implement this using RuntimeReplaceable, using NewInstance -> Invoke or a StaticInvoke? The added value is that this support code generation, and reduces code duplication.

petermaxlee · 2016-07-08T16:55:49Z

@hvanhovell in its current form we'd need some refactoring to StaticInvoke to work with this, due to Hive allowing both static invocation and dynamic invocation.

Also - does StaticInvoke do type conversion? If not, it seems like extra work is needed in order to work with RuntimeReplaceable, due to a limitation that it does not really support proper type checking for expressions.

As for String, I remember @rxin making a comment somewhere that string type is sufficient for now.

dongjoon-hyun · 2016-07-08T17:08:44Z

If you mean the following mention, what @rxin said was not about the return type. It's about function and method name, isn't it? Is there any other mentions about String?

Majority of the use cases will be calling some literal functions anyway.

petermaxlee · 2016-07-08T18:20:25Z

I've pushed a new commit that addresses the review comments.

SparkQA · 2016-07-09T23:14:21Z

Test build #3174 has finished for PR 13969 at commit d24ca97.

This patch passes all tests.
This patch does not merge cleanly.
This patch adds the following public classes (experimental):
- case class ParseUrl(children: Seq[Expression])

cloud-fan · 2016-07-11T03:47:24Z

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

+  usage = "_FUNC_(class,method[,arg1[,arg2..]]) calls method with reflection",
+  extended = "> SELECT _FUNC_('java.util.UUID', 'randomUUID');\n c33fb387-8500-4bfa-81d2-6e0e3e930df2")
+// scalastyle:on line.size.limit
+case class Reflect(children: Seq[Expression])


Reflect is really ambiguous, how about CallMethod?

I actually named it JavaMethodReflect before but @dongjoon-hyun asked to use Reflect.

It is also annoying if we search for reflect (based on the name) and then doesn't find an expression with reflect in the name.

Ya. It's my fault. Sorry for that.

So what's a good name? I am not attached to Reflect, but I think Reflect should be in the name, if the function is called reflect.

CallMethodUsingReflect?

cloud-fan · 2016-07-11T05:20:47Z

what's hive's behaviour if calling a non-static method but the class doesn't have no-arg constructor? null or exception?

rxin · 2016-07-11T05:28:38Z

One thing ... it might be better to remove the ability to call non-static methods. At least to me it'd make the things slightly simpler and more clear.

rxin · 2016-07-11T05:29:10Z

You can also remove half of the test cases.

petermaxlee · 2016-07-11T18:05:40Z

I submitted a new pull request #14138 and in that version only static methods are supported.

## What changes were proposed in this pull request? This patch implements reflect SQL function, which can be used to invoke a Java method in SQL. Slightly different from Hive, this implementation requires the class name and the method name to be literals. This implementation also supports only a smaller number of data types, and requires the function to be static, as suggested by rxin in #13969. java_method is an alias for reflect, so this should also resolve SPARK-16277. ## How was this patch tested? Added expression unit tests and an end-to-end test. Author: petermaxlee <petermaxlee@gmail.com> Closes #14138 from petermaxlee/reflect-static.

## What changes were proposed in this pull request? This patch implements reflect SQL function, which can be used to invoke a Java method in SQL. Slightly different from Hive, this implementation requires the class name and the method name to be literals. This implementation also supports only a smaller number of data types, and requires the function to be static, as suggested by rxin in #13969. java_method is an alias for reflect, so this should also resolve SPARK-16277. ## How was this patch tested? Added expression unit tests and an end-to-end test. Author: petermaxlee <petermaxlee@gmail.com> Closes #14138 from petermaxlee/reflect-static. (cherry picked from commit 56bd399) Signed-off-by: Wenchen Fan <wenchen@databricks.com>

petermaxlee · 2016-07-26T02:16:43Z

I am going to close this one since #14138 has been merged.

cloud-fan reviewed Jun 29, 2016
View reviewed changes

petermaxlee force-pushed the reflect branch from dfe3480 to 0e43c95 Compare June 30, 2016 05:14

cloud-fan reviewed Jun 30, 2016
View reviewed changes

petermaxlee force-pushed the reflect branch from 0e43c95 to 06b7b10 Compare July 1, 2016 04:54

dongjoon-hyun reviewed Jul 6, 2016
View reviewed changes

dongjoon-hyun reviewed Jul 8, 2016
View reviewed changes

hvanhovell reviewed Jul 8, 2016
View reviewed changes

petermaxlee added 2 commits July 8, 2016 09:57

Rebase

4c9b975

code review

3fec161

petermaxlee force-pushed the reflect branch from f5becbf to 3fec161 Compare July 8, 2016 16:58

code review

1c7bcbe

Merge remote-tracking branch 'apache/master' into reflect

d24ca97

cloud-fan reviewed Jul 11, 2016
View reviewed changes

petermaxlee mentioned this pull request Jul 11, 2016

[SPARK-16284][SQL] Implement reflect SQL function #14138

Closed

petermaxlee closed this Jul 26, 2016

Conversation

petermaxlee commented Jun 29, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

petermaxlee commented Jun 29, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

petermaxlee Jun 29, 2016 • 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!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jun 29, 2016

Uh oh!

SparkQA commented Jun 30, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

petermaxlee commented Jul 6, 2016

Uh oh!

cloud-fan commented Jul 6, 2016

Uh oh!

dongjoon-hyun commented Jul 6, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun commented Jul 6, 2016

Uh oh!

dongjoon-hyun commented Jul 6, 2016

Uh oh!

dongjoon-hyun commented Jul 6, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rxin commented Jul 6, 2016

Uh oh!

dongjoon-hyun commented Jul 6, 2016

Uh oh!

petermaxlee commented Jul 7, 2016

Uh oh!

petermaxlee commented Jul 7, 2016

Uh oh!

petermaxlee commented Jul 8, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun Jul 8, 2016 • 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!

Choose a reason for hiding this comment

Uh oh!

hvanhovell Jul 8, 2016 • 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!

hvanhovell commented Jul 8, 2016

Uh oh!

petermaxlee commented Jul 8, 2016

Uh oh!

dongjoon-hyun commented Jul 8, 2016

Uh oh!

petermaxlee commented Jun 29, 2016 •

edited

Loading

petermaxlee Jun 29, 2016 •

edited

Loading

dongjoon-hyun commented Jul 6, 2016 •

edited

Loading

dongjoon-hyun commented Jul 8, 2016 •

edited

Loading

dongjoon-hyun Jul 8, 2016 •

edited

Loading

hvanhovell Jul 8, 2016 •

edited

Loading