[SPARK-23583][SQL] Invoke should support interpreted execution #20797

kiszk · 2018-03-11T20:09:39Z

What changes were proposed in this pull request?

This pr added interpreted execution for Invoke.

How was this patch tested?

Added tests in ObjectExpressionsSuite.

SparkQA · 2018-03-11T23:35:33Z

Test build #88164 has finished for PR 20797 at commit 3dbea12.

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

viirya · 2018-03-12T02:06:19Z

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

Do we need to do explicitly cast? Doesn't Scala cast it if it's needed?

It would work without this cast in Scala. I tried to do the same check that may throw an exception.
WDYT?

For interpreted execution, I think it is less meaningful to check that. cc @hvanhovell

I think that the point is where we want to cause a cast exception. With this code, an exception will occur at this class. Without this code, an exception will occur anywhere in the code.

viirya · 2018-03-12T02:07:27Z

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

We should also check if obj is null too.

SparkQA · 2018-03-12T22:17:25Z

Test build #88181 has finished for PR 20797 at commit d81c950.

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

kiszk · 2018-03-19T19:34:25Z

ping @hvanhovell

kiszk · 2018-03-26T00:59:13Z

ping @hvanhovell

SparkQA · 2018-03-26T11:58:41Z

Test build #88583 has finished for PR 20797 at commit 6e2d136.

This patch fails to build.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2018-03-26T16:09:19Z

Test build #88590 has finished for PR 20797 at commit 38d1af2.

This patch fails to build.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2018-03-26T20:11:07Z

Test build #88591 has finished for PR 20797 at commit 22cd70c.

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

kiszk · 2018-03-27T03:21:47Z

@hvanhovell sorry for ping again

hvanhovell · 2018-03-27T13:19:14Z

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

I basically have the same comments here as for StaticInvoke: Why do we need to resolve the method at runtime? Why are you using CodeGenerator.defaultValue(dataType) to determine if something is a primitive?

O, btw: We should reuse the method resolution logic in StaticInvoke

Since obj should be evaluated at each call of eval, it is hard to reuse method resolution logic in StaticInvoke. On the other hand, we can reuse logic for casting a return value.

I refactored the code as possible as the common code is used.

SparkQA · 2018-03-27T20:26:13Z

Test build #88631 has finished for PR 20797 at commit e32014e.

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

SparkQA · 2018-03-28T06:31:29Z

Test build #88649 has finished for PR 20797 at commit 4493909.

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

kiszk · 2018-03-28T07:30:49Z

retest this please

SparkQA · 2018-03-28T10:05:40Z

Test build #88655 has finished for PR 20797 at commit 4493909.

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

kiszk · 2018-03-29T01:16:35Z

retest this please

SparkQA · 2018-03-29T04:33:14Z

Test build #88686 has finished for PR 20797 at commit 4493909.

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

kiszk · 2018-04-02T17:59:28Z

retest this please

SparkQA · 2018-04-02T21:45:48Z

Test build #88821 has finished for PR 20797 at commit 4493909.

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

hvanhovell · 2018-04-02T22:40:36Z

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

Why are we resolving during evaluation time? This seems to be expensive?

Unlike StaticInvoke, this is because obj is dynamically evaluated with a value in InternalRow. We cannot resolve beforehand when the case class is created.

We should know the type before this right? Otherwise whole stage code generation doesn't work either.

We can know the base class beforehand. The obj class may be a subclass. I think this is a kind of virtual table lookup based on the literal name?
Since JVM class resolution converts the literal name to a constant offset, the code generation can perform a fast lookup based on the offset.

Ah, this method may be applied to the base class. let me try this optimization.

Good catch. I also use method that was already defined for interpreted execution.

SparkQA · 2018-04-03T19:02:42Z

Test build #88847 has finished for PR 20797 at commit 4185f09.

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

hvanhovell · 2018-04-03T23:46:03Z

LGTM @kiszk can you update your branch so we can merge?

SparkQA · 2018-04-04T07:05:01Z

Test build #88869 has finished for PR 20797 at commit c568944.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

kiszk · 2018-04-04T07:36:45Z

retest this please

SparkQA · 2018-04-04T09:28:14Z

Test build #88879 has finished for PR 20797 at commit c568944.

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

kiszk · 2018-04-04T10:39:48Z

retest this please

SparkQA · 2018-04-04T14:20:29Z

Test build #88886 has finished for PR 20797 at commit c568944.

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

hvanhovell · 2018-04-04T16:35:43Z

LGTM - merging to master. Thanks!

viirya · 2018-04-05T05:41:30Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/ScalaReflection.scala

+      case ObjectType(cls) => cls
+      case _ => typeJavaMapping.getOrElse(dt, classOf[java.lang.Object])
+    }
+  }


cc @maropu We can now use this instead of CallMethodViaReflection.typeMapping.

## What changes were proposed in this pull request? This pr added interpreted execution for `Invoke`. ## How was this patch tested? Added tests in `ObjectExpressionsSuite`. Author: Kazuaki Ishizaki <[email protected]> Closes apache#20797 from kiszk/SPARK-28583.

viirya reviewed Mar 12, 2018

View reviewed changes

kiszk mentioned this pull request Mar 26, 2018

[SPARK-23593][SQL] Add interpreted execution for InitializeJavaBean expression #20756

Closed

kiszk force-pushed the SPARK-28583 branch from 6e2d136 to 38d1af2 Compare March 26, 2018 15:58

hvanhovell reviewed Mar 27, 2018

View reviewed changes

hvanhovell reviewed Apr 2, 2018

View reviewed changes

kiszk added 6 commits April 4, 2018 12:28

initial commit

8c25bf0

address review comment

b0a9748

move utility methods to ScalaReflection

4894725

fix build error

d0177c2

address review comments

1b1d843

fix test failures

c8ffbcd

address review comment

c568944

kiszk force-pushed the SPARK-28583 branch from 4185f09 to c568944 Compare April 4, 2018 03:34

asfgit closed this in a355236 Apr 4, 2018

viirya reviewed Apr 5, 2018

View reviewed changes

[SPARK-23583][SQL] Invoke should support interpreted execution #20797

[SPARK-23583][SQL] Invoke should support interpreted execution #20797

Uh oh!

Conversation

kiszk commented Mar 11, 2018

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

SparkQA commented Mar 11, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kiszk Mar 12, 2018 • 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!

kiszk Mar 13, 2018 • 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 Mar 12, 2018

Uh oh!

kiszk commented Mar 19, 2018

Uh oh!

kiszk commented Mar 26, 2018

Uh oh!

SparkQA commented Mar 26, 2018

Uh oh!

SparkQA commented Mar 26, 2018

Uh oh!

SparkQA commented Mar 26, 2018

Uh oh!

kiszk commented Mar 27, 2018

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 Mar 27, 2018

Uh oh!

SparkQA commented Mar 28, 2018

Uh oh!

kiszk commented Mar 28, 2018

Uh oh!

SparkQA commented Mar 28, 2018

Uh oh!

kiszk commented Mar 29, 2018

Uh oh!

SparkQA commented Mar 29, 2018

Uh oh!

kiszk commented Apr 2, 2018

Uh oh!

SparkQA commented Apr 2, 2018

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 3, 2018

Uh oh!

hvanhovell commented Apr 3, 2018

Uh oh!

kiszk Mar 12, 2018 •

edited

Loading

kiszk Mar 13, 2018 •

edited

Loading