[SPARK-18717][SQL] Make code generation for Scala Map work with immutable.Map also #16161

aray · 2016-12-05T23:15:47Z

What changes were proposed in this pull request?

Fixes compile errors in generated code when user has case class with a scala.collections.immutable.Map instead of a scala.collections.Map. Since ArrayBasedMapData.toScalaMap returns the immutable version we can make it work with both.

How was this patch tested?

Additional unit tests.

SparkQA · 2016-12-06T00:43:10Z

Test build #69696 has finished for PR 16161 at commit 699cc75.

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

srowen · 2016-12-06T02:22:59Z

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

      StaticInvoke(
        ArrayBasedMapData.getClass,
-        ObjectType(classOf[Map[_, _]]),
+        ObjectType(classOf[scala.collection.immutable.Map[_, _]]),


I don't know this bit well, but the comments on the JIRA indicate that immutable maps aren't supported. However, this change should be a no-op because Predef.Map is already immutable.Map right -- how does this address it?

@srowen scala.collection.Map is imported at the beginning of RowEncoder and ScalaReflection.

rxin · 2016-12-06T07:52:57Z

cc @cloud-fan

cloud-fan · 2016-12-06T08:03:03Z

can you highlight your approach to fix this bug? It looks to me that this is not a general fix, e.g. mutable.Map will still fail.

aray · 2016-12-06T19:25:26Z

The approach is to change the deserializer (via ScalaReflection#deserializerFor) to return the more specific type scala.collections.immutable.Map instead of scala.collections.Map as it does now (no change to ArrayBasedMapData#toScalaMap is needed as it already returns an immutable map). This allows the generated code for case classes using either of those Map's to work as they are both assignable from scala.collections.immutable.Map. This PR does not address scala.collections.mutable.Map, it still wont work.

SparkQA · 2016-12-06T21:16:49Z

Test build #69741 has finished for PR 16161 at commit f325e75.

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

srowen · 2016-12-06T22:22:19Z

OK, it sounds like what you're doing is unsupported at the moment and this doesn't change that, and you have a workaround. Is this worth merging?

aray · 2016-12-06T23:03:23Z

Right now it's not supported to have the following:

case class Foo(a: Map[Int, Int])

(using the scala Predef version of Map)

The documented way to do this is:

case class Foo(a: scala.collections.Map[Int, Int])

and it works fine.

However if someone did not read the documentation carefully and used the former, instead of a reasonable error they get a compile error on the code spark generates. Therefore IMHO there are two options:

Make the generated code work with either version of Map (this PR)
Intercept definitions that use the wrong version of Map and issue a proper error.

If the consensus is that this PR is not worth it then I'll be happy to work on option 2. But in my opinion as a Spark user option 1 is better.

cloud-fan · 2016-12-07T08:42:43Z

option 1 is better, but this PR only adds the support for scala.collection.immutable.Map, with a very specific way: we can't extend it to support scala.collection.mutable.Map or List.

I think we need a general solution to support custom collection type.

aray · 2016-12-07T22:45:51Z

I would be happy to create a seperate PR for adding support for mutable.Map (and List) if that is wanted. But there is no generic solution as there is no type that is assignable to both mutable.Map and immutable.Map.

cloud-fan · 2016-12-08T14:50:14Z

there is no type that is assignable to both mutable.Map and immutable.Map

Yea, and a general solution would be, remember the original type and create that type when converting catalyst value to external value.

But this may be hard to implement, it may worth to merge this simple PR, as the scala.collection.immutable.Map is so common in scala. cc @rxin @yhuai @liancheng

…-codegen # Conflicts: # sql/core/src/test/scala/org/apache/spark/sql/DatasetSuite.scala

SparkQA · 2016-12-12T18:15:52Z

Test build #70027 has finished for PR 16161 at commit e0b3e76.

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

cloud-fan · 2016-12-13T07:49:43Z

thanks, merging to master!

…able.Map also ## What changes were proposed in this pull request? Fixes compile errors in generated code when user has case class with a `scala.collections.immutable.Map` instead of a `scala.collections.Map`. Since ArrayBasedMapData.toScalaMap returns the immutable version we can make it work with both. ## How was this patch tested? Additional unit tests. Author: Andrew Ray <[email protected]> Closes apache#16161 from aray/fix-map-codegen.

liancheng · 2017-02-10T21:43:27Z

Shall we backport this to branch-2.1? I'd consider this as a bug because the following snippet fail in Spark 2.1:

case class Wrapper1(value: Option[Map[String, String]])
case class Wrapper2(value: Map[String, String])

val ds1 = Seq.empty[Wrapper1].toDS()  // Fail
val ds2 = Seq.empty[Wrapper2].toDS()  // Succeed

cloud-fan · 2017-02-10T22:07:57Z

I'm fine to backport this

liancheng · 2017-02-11T00:10:14Z

Thanks. Backported to branch-2.1.

…able.Map also ## What changes were proposed in this pull request? Fixes compile errors in generated code when user has case class with a `scala.collections.immutable.Map` instead of a `scala.collections.Map`. Since ArrayBasedMapData.toScalaMap returns the immutable version we can make it work with both. ## How was this patch tested? Additional unit tests. Author: Andrew Ray <[email protected]> Closes #16161 from aray/fix-map-codegen. (cherry picked from commit 46d30ac) Signed-off-by: Cheng Lian <[email protected]>

aray added 3 commits December 5, 2016 15:41

the fix

3aa7371

unit test

c070509

unused import

699cc75

srowen reviewed Dec 6, 2016

View reviewed changes

revert changes to RowEncoder.scala

f325e75

Merge branch 'master' of https://github.com/apache/spark into fix-map…

e0b3e76

…-codegen # Conflicts: # sql/core/src/test/scala/org/apache/spark/sql/DatasetSuite.scala

asfgit closed this in 46d30ac Dec 13, 2016

gaborbarna mentioned this pull request Feb 13, 2018

Add support for a Flatten annotation BenFradet/struct-type-encoder#20

Merged

[SPARK-18717][SQL] Make code generation for Scala Map work with immutable.Map also #16161

[SPARK-18717][SQL] Make code generation for Scala Map work with immutable.Map also #16161

Uh oh!

Conversation

aray commented Dec 5, 2016

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

SparkQA commented Dec 6, 2016

Uh oh!

srowen Dec 6, 2016

Choose a reason for hiding this comment

Uh oh!

viirya Dec 6, 2016

Choose a reason for hiding this comment

Uh oh!

rxin commented Dec 6, 2016

Uh oh!

cloud-fan commented Dec 6, 2016

Uh oh!

aray commented Dec 6, 2016

Uh oh!

SparkQA commented Dec 6, 2016

Uh oh!

srowen commented Dec 6, 2016

Uh oh!

aray commented Dec 6, 2016

Uh oh!

cloud-fan commented Dec 7, 2016

Uh oh!

aray commented Dec 7, 2016

Uh oh!

cloud-fan commented Dec 8, 2016

Uh oh!

SparkQA commented Dec 12, 2016

Uh oh!

cloud-fan commented Dec 13, 2016

Uh oh!

liancheng commented Feb 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cloud-fan commented Feb 10, 2017

Uh oh!

liancheng commented Feb 11, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

liancheng commented Feb 10, 2017 •

edited

Loading