[SPARK-17683][SQL] Support ArrayType in Literal.apply #15257

maropu · 2016-09-27T08:33:55Z

What changes were proposed in this pull request?

This pr is to add pattern-matching entries for array data in Literal.apply.

How was this patch tested?

Added tests in LiteralExpressionSuite.

SparkQA · 2016-09-27T08:39:40Z

Test build #65958 has finished for PR 15257 at commit c46e14c.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

jaceklaskowski · 2016-09-27T10:37:36Z

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

    case i: CalendarInterval => Literal(i, CalendarIntervalType)
    case null => Literal(null, NullType)
    case v: Literal => v
    case _ =>
      throw new RuntimeException("Unsupported literal type " + v.getClass + " " + v)
  }

+  private def componentTypeToDataType(clz: Class[_]): DataType = clz match {


It looks so similar to the other cases where Spark has to map from Scala types to Spark SQL's, like https://github.com/apache/spark/blob/master/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/ScalaReflection.scala#L65. When comparing them I noticed that CalendarInterval is not included in your list. Why?

just my bad. I'll try use the func. thanks!

@jaceklaskowski I checked these similar code though, I couldn't find better ideas to use reflection stuffs here in ScalaReflection because Literal#apply cannot catch type signatures in runtime. If possible, any better idea?

I can't find either, but thought I'd ask.

thx, I keep trying.

SparkQA · 2016-09-27T10:51:38Z

Test build #65960 has finished for PR 15257 at commit e495ff5.

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

SparkQA · 2016-09-28T05:34:10Z

Test build #66022 has finished for PR 15257 at commit c57d93e.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2016-09-28T07:45:30Z

Test build #66023 has finished for PR 15257 at commit c9cdd29.

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

jodersky · 2016-09-28T22:42:32Z

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

+   */
+  private[this] def componentTypeToDataType(clz: Class[_]): DataType = clz match {
+    // primitive types
+    case c: Class[_] if c == jShort.TYPE => ShortType


the instance matching is not required here, it should be sufficient to use case jShort.TYPE.

Btw, this is a nitpick but I would recommend that classes should start with a capital letter, i.e. java.lang.Short could be imported as JShort rather that jShort. In this case it really doesn't matter, but lower case classes can bite you when your working with pattern matching

Aha, I see. I'll fix this. thx!

@jodersky BTW, what' a concrete example when the lower case issue in pattern matching occurs? (This is a ask from my interests :) )

Here is an example:

case object container {def value: Int = 42} case object Container {def value: Int = 42} def extract(wrapper: Any) = wrapper match { case container => container.value // error: value "value" is not a member of Any case Container => Container.value // ok }

It's not a very good example, maybe something like http://stackoverflow.com/a/6576663/917519 will give you a better use-case

aha, many thanks!

SparkQA · 2016-09-29T02:20:44Z

Test build #66075 has finished for PR 15257 at commit c843840.

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

viirya · 2016-09-29T05:21:39Z

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

      Literal(Decimal(d), DecimalType(Math.max(d.precision, d.scale), d.scale()))
    case d: Decimal => Literal(d, DecimalType(Math.max(d.precision, d.scale), d.scale))
    case t: Timestamp => Literal(DateTimeUtils.fromJavaTimestamp(t), TimestampType)
    case d: Date => Literal(DateTimeUtils.fromJavaDate(d), DateType)
    case a: Array[Byte] => Literal(a, BinaryType)
+    case a: Array[_] =>


Can we use schemaFor for this? Looks like we can remove the type inference codes added in this PR.

yea, I tried though, I couldn't use it here because scala Array types are mapped into java ones in run-time, e.g., Array[Int]=>int[].

I may not understand your point. Where it will be mapped to java type? You use Array[_] here, right?

Sorry for my bad explanation.
For example, I added check codes below;

case a: Array[_] => println("Array[] class in runtime:" + a.getClass.getSimpleName()) ...

And, then run this code;

scala> Literal(scala.Array(1, 2, 3)) Array[] class in runtime:int[]

IIUC, int[] is a java native array type and there is no type signature here.
Since schemaFor needs type signatures, I'm afraid we cannot this reflection func here.
There are other workarounds though, I couldn't find them.
Any better idea?

This seems work:

val t = scala.Array(1, 2, 3) val classSymbol = runtimeMirror(getClass.getClassLoader).classSymbol(t.getClass) val tpe = classSymbol.selfType ScalaReflection.schemaFor(tpe).dataType

If it holds TypeTag, seems we can write this;

def create[T : TypeTag](v: T): Literal = { val ScalaReflection.Schema(dataType, _) = ScalaReflection.schemaFor[T] val convert = CatalystTypeConverters.createToCatalystConverter(dataType) Literal(convert(v), dataType) }

This certainly works.

@maropu ya, we will get a generic type back and schemaFor needs to know what the element type is.

@maropu, I tested my solution in a shell and it worked. To fix the compilation error, simply make the overloaded method ScalaReflection.dataTypeFor public. I don't see any reason for it being private in the first place, apart from it just being a default.
Basically, just remove the "private" from the following in ScalaReflection.scala:
private def dataTypeFor(tpe:Type): DataType = //...

@jodersky Can you check the returned DataType? From the code, looks like it will return ObjectType instead of ArrayType for an array type.

oh, you're right. I didn't realize you specifically wanted an ArrayType

jodersky · 2016-09-29T19:15:37Z

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

+   */
+  private[this] def componentTypeToDataType(clz: Class[_]): DataType = clz match {
+    // primitive types
+    case c: Class[_] if c == JavaShort.TYPE => ShortType


matching against an instance of Class[_] is not necessary. A simpler solution is case JavaShort.TYPE => ShortType

I just wrote along with HiveInspectors#javaClassToDataType. Both are okay to me and I'll fix.

cool, I saw you updated the match. However you can remove the instance check everywhere, including further down. Basically a case c: Class[_] is "equivalent" to an c.isInstanceOf[Class[_]], however that is redundant since your parameter clz already specifies the type to be Class[_]

How about the latest commit?

hmm, I didn't think about my own advice: matching agains lowercase classOf[X] doesn't work. So either case _ if clz==classOf[] or case c if c == classOf[] are good

SparkQA · 2016-09-30T01:56:25Z

Test build #66137 has finished for PR 15257 at commit 41b9276.

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

SparkQA · 2016-09-30T03:06:04Z

Test build #66141 has finished for PR 15257 at commit ebee7e4.

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

rxin · 2016-11-02T07:13:46Z

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

@@ -17,14 +17,25 @@

 package org.apache.spark.sql.catalyst.expressions

+import java.lang.{Boolean => JavaBoolean}


I personally think it is more clear to just say java.lang.Boolean in the code, but this is fine.

We have conventionally used JLong etc rather than JavaLong when doing this kind of rename.

I just named these along with other classes such as https://github.com/apache/spark/blob/master/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/CatalystTypeConverters.scala#L20. Nitpicking though, should we fix these name, too (e.g., JavaDecimal=>JDecimal)?

rxin · 2016-11-02T07:14:00Z

Merging in master/branch-2.1. Thanks!

rxin · 2016-11-02T07:16:43Z

Actually I didn't end up merging it. The original ticket wanted Seq support as well. Can you take a look to see how difficult it is to add support for both Seq and java.util.List? Then we should be good to go.

maropu · 2016-11-02T07:50:53Z

I think it is difficult to support Seq and JList because of type erasure. If it is possible to add a new interface below, we can support these types via ScalaReflection.schemaFor[T].

  def create[T : TypeTag](v: T): Literal = {
    val ScalaReflection.Schema(dataType, _) = ScalaReflection.schemaFor[T]
    val convert = CatalystTypeConverters.createToCatalystConverter(dataType)
    Literal(convert(v), dataType)
  }

rxin · 2016-11-02T18:28:31Z

OK thanks for looking at this. I'm going to merge this into master/branch-2.1.

## What changes were proposed in this pull request? This pr is to add pattern-matching entries for array data in `Literal.apply`. ## How was this patch tested? Added tests in `LiteralExpressionSuite`. Author: Takeshi YAMAMURO <linguin.m.s@gmail.com> Closes #15257 from maropu/SPARK-17683. (cherry picked from commit 4af0ce2) Signed-off-by: Reynold Xin <rxin@databricks.com>

## What changes were proposed in this pull request? This pr is to add pattern-matching entries for array data in `Literal.apply`. ## How was this patch tested? Added tests in `LiteralExpressionSuite`. Author: Takeshi YAMAMURO <linguin.m.s@gmail.com> Closes apache#15257 from maropu/SPARK-17683.

Support ArrayType in Literal.apply

c46e14c

fix syntax errors

e495ff5

jaceklaskowski reviewed Sep 27, 2016

View reviewed changes

Add comments

c026826

Suport CalendarInterval

c9cdd29

maropu force-pushed the SPARK-17683 branch from c57d93e to c9cdd29 Compare September 28, 2016 05:40

jodersky reviewed Sep 28, 2016

View reviewed changes

Rename aliased java class names

c843840

viirya reviewed Sep 29, 2016

View reviewed changes

jodersky reviewed Sep 29, 2016

View reviewed changes

maropu added 2 commits September 30, 2016 08:39

Simplify entries

41b9276

Simplify patten matching

ebee7e4

rxin reviewed Nov 2, 2016

View reviewed changes

asfgit closed this in 4af0ce2 Nov 2, 2016

		@@ -17,14 +17,25 @@

		package org.apache.spark.sql.catalyst.expressions

		import java.lang.{Boolean => JavaBoolean}

[SPARK-17683][SQL] Support ArrayType in Literal.apply #15257

[SPARK-17683][SQL] Support ArrayType in Literal.apply #15257

Conversation

maropu commented Sep 27, 2016

What changes were proposed in this pull request?

How was this patch tested?

SparkQA commented Sep 27, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Sep 27, 2016

SparkQA commented Sep 28, 2016

SparkQA commented Sep 28, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maropu Sep 29, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Sep 29, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jodersky Sep 30, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jodersky Sep 30, 2016 • edited

Choose a reason for hiding this comment

SparkQA commented Sep 30, 2016

SparkQA commented Sep 30, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rxin commented Nov 2, 2016

rxin commented Nov 2, 2016

maropu commented Nov 2, 2016

rxin commented Nov 2, 2016

maropu Sep 29, 2016 •

edited

jodersky Sep 30, 2016 •

edited

jodersky Sep 30, 2016 •

edited