[SPARK-11954][SQL] Encoder for JavaBeans #9937

cloud-fan · 2015-11-24T15:34:58Z

create java version of constructorFor and extractorFor in JavaTypeInference

SparkQA · 2015-11-24T15:45:58Z

Test build #46608 has finished for PR 9937 at commit d7a4bf9.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):\n * case class InitializeJavaBean(n: NewInstance, fields: Seq[Expression], setters: Seq[String])\n

marmbrus · 2015-11-24T19:03:03Z

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

could this take Map[String, Expression]? zipping always seems awkward when we can avoid it.

Also, I would not make it take a NewInstance as it would be nice to use a Literal sometimes for object reuse.

Seems we can not use Map here, as the fields: Seq[Expression] needs resolution and transformation.

We could probably just extend this logic to recurse into maps similar to how we handle traversables.

rxin · 2015-11-24T22:26:31Z

I think Java beans is different from pojo. Beans only care about fields that have getters and setters, while pojo should care about all the fiedls.

cloud-fan · 2015-11-25T00:41:42Z

A POJO is not rigorously defined, and I'm not sure how to detect it and re-construct it...

rxin · 2015-11-25T00:52:57Z

Use reflection to find all the public and private fields?

marmbrus · 2015-11-25T01:17:48Z

Perhaps we should start with Java Beans. They are well defined, we have existing reflection code, and I've never heard a java user complain that was not sufficient (this is what we support for creating java data frames).

rxin · 2015-11-25T03:38:19Z

Sounds good (supporting only beans for now).

cloud-fan · 2015-11-25T15:10:28Z

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

I came up with this by intuition and verified it by experiment, cannot find any doc for it, very hacky...

SparkQA · 2015-11-25T15:24:53Z

Test build #46691 has finished for PR 9937 at commit a31ea94.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):\n * trait ScalaReflection\n * case class InitializeJavaBean(n: NewInstance, fields: Seq[Expression], setters: Seq[String])\n

cloud-fan · 2015-11-25T15:42:54Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/encoders/ExpressionEncoder.scala

this is not right, we may have attribute loopVar in MapObjects.

cloud-fan · 2015-11-25T15:50:32Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/encoders/ExpressionEncoder.scala

I depend on name to skip the attribute loopVar in MapObjects, need a better way...
cc @marmbrus

Hmm, this does feel kind of hacky, but I'm not sure if its really a problem in practice. This assertion is only to catch our own bugs, not user mistakes.

SparkQA · 2015-11-25T18:11:05Z

Test build #46693 has finished for PR 9937 at commit 58d1cc1.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):\n * trait ScalaReflection\n * case class InitializeJavaBean(n: NewInstance, fields: Seq[Expression], setters: Seq[String])\n

cloud-fan · 2015-11-26T09:43:54Z

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

I moved the schemaFor stuff to object ScalaReflection, because of the need of mirror.runtimeClass in getJavaBeanFields

SparkQA · 2015-11-26T11:37:01Z

Test build #46763 has finished for PR 9937 at commit 40433ca.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):\n * trait ScalaReflection\n * case class InitializeJavaBean(n: NewInstance, fields: Seq[Expression], setters: Seq[String])\n

marmbrus · 2015-11-26T18:31:55Z

Thanks for working on this. I'm not sure that this is the right tactic though. I don't think that we want to make java bean the default fall back in our scala reflection code path. Instead I think that we only want to create a java bean encoder when the user explicitly asks for it and thus this should be a separate code path. The only common code should be ExpressionEncoder probably.

Also, instead of hacking this into scala's reflection, I think we should use the builtin java support for introspecting on beans. I would probably start with the code in JavaTypeInference and use that to construct expressions.

cloud-fan · 2015-11-27T03:45:54Z

@marmbrus do you mean we should write java version of extractorsFor and constructorFor in JavaTypeInference?

cloud-fan · 2015-11-30T07:24:49Z

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

The problem is that, for java map, if we get the keys and values by keySet and values, we can not guarantee they have same iteration order(which is different from scala map). A possible solution is creating a new MapObjects that can iterate a map directly.

cc @marmbrus

SparkQA · 2015-11-30T07:48:05Z

Test build #46871 has finished for PR 9937 at commit c634aa4.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):\n * case class InitializeJavaBean(n: NewInstance, setters: Map[String, Expression])\n

SparkQA · 2015-11-30T11:51:12Z

Test build #46880 has finished for PR 9937 at commit e852710.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):\n * case class InitializeJavaBean(n: NewInstance, setters: Map[String, Expression])\n

SparkQA · 2015-11-30T12:37:03Z

Test build #46882 has finished for PR 9937 at commit 7940ec7.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):\n * case class InitializeJavaBean(n: NewInstance, setters: Map[String, Expression])\n

SparkQA · 2015-11-30T14:22:44Z

Test build #46883 has finished for PR 9937 at commit fb9a898.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):\n * case class InitializeJavaBean(n: NewInstance, setters: Map[String, Expression])\n

marmbrus · 2015-12-01T01:30:16Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/trees/TreeNode.scala

.view.force seems a little odd. toList?

we need to return a Map here, maybe toList.toMap?

oh... hmmm, what happens if we don't have this? is it lazy? I guess view.force is fine. It was just a little confusing when reading.

Also, can you add a test to TreeNodeSuite?

yea, mapValues is lazy, I'll add a comment to explain it, also add test in TreeNodeSuite

marmbrus · 2015-12-01T01:35:18Z

Some minor comments, but this is looking really good!

SparkQA · 2015-12-01T06:00:13Z

Test build #46939 has finished for PR 9937 at commit 0d95daf.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):\n * case class InitializeJavaBean(beanInstance: Expression, setters: Map[String, Expression])\n

marmbrus · 2015-12-01T18:27:25Z

Thanks! I'm going to try and fix conflicts while merging to master and 1.6.

create java version of `constructorFor` and `extractorFor` in `JavaTypeInference` Author: Wenchen Fan <[email protected]> This patch had conflicts when merged, resolved by Committer: Michael Armbrust <[email protected]> Closes #9937 from cloud-fan/pojo. (cherry picked from commit fd95eea) Signed-off-by: Michael Armbrust <[email protected]>

marmbrus reviewed Nov 24, 2015
View reviewed changes

cloud-fan force-pushed the pojo branch from d7a4bf9 to a31ea94 Compare November 25, 2015 15:07

cloud-fan changed the title ~~[SPARK-11954][SQL][WIP] Encoder for JavaBeans / POJOs~~ [SPARK-11954][SQL] Encoder for JavaBeans Nov 25, 2015

cloud-fan reviewed Nov 25, 2015
View reviewed changes

cloud-fan force-pushed the pojo branch from a31ea94 to 58d1cc1 Compare November 25, 2015 15:49

cloud-fan reviewed Nov 25, 2015
View reviewed changes

cloud-fan reviewed Nov 26, 2015
View reviewed changes

cloud-fan force-pushed the pojo branch from 40433ca to 8d10477 Compare November 30, 2015 07:15

support java bean encoder

c634aa4

cloud-fan force-pushed the pojo branch from 8d10477 to c634aa4 Compare November 30, 2015 07:19

cloud-fan reviewed Nov 30, 2015
View reviewed changes

fix test

e852710

cloud-fan added 2 commits November 30, 2015 19:51

add doc and todo

7940ec7

fix test

fb9a898

marmbrus reviewed Dec 1, 2015
View reviewed changes

cloud-fan added 2 commits December 1, 2015 11:45

address comments

4abac68

improve test

0d95daf

asfgit closed this in fd95eea Dec 1, 2015

[SPARK-11954][SQL] Encoder for JavaBeans #9937

[SPARK-11954][SQL] Encoder for JavaBeans #9937

Uh oh!

Conversation

cloud-fan commented Nov 24, 2015

Uh oh!

SparkQA commented Nov 24, 2015

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!

rxin commented Nov 24, 2015

Uh oh!

cloud-fan commented Nov 25, 2015

Uh oh!

rxin commented Nov 25, 2015

Uh oh!

marmbrus commented Nov 25, 2015

Uh oh!

rxin commented Nov 25, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Nov 25, 2015

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 Nov 25, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Nov 26, 2015

Uh oh!

marmbrus commented Nov 26, 2015

Uh oh!

cloud-fan commented Nov 27, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Nov 30, 2015

Uh oh!

SparkQA commented Nov 30, 2015

Uh oh!

SparkQA commented Nov 30, 2015

Uh oh!

SparkQA commented Nov 30, 2015

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!

marmbrus commented Dec 1, 2015

Uh oh!

SparkQA commented Dec 1, 2015

Uh oh!

marmbrus commented Dec 1, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants