[SPARK-23595][SQL] ValidateExternalType should support interpreted execution #20757

maropu · 2018-03-07T05:50:14Z

What changes were proposed in this pull request?

This pr supported interpreted mode for ValidateExternalType.

How was this patch tested?

Added tests in ObjectExpressionsSuite.

SparkQA · 2018-03-07T08:05:01Z

Test build #88037 has finished for PR 20757 at commit d53cfea.

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

hvanhovell · 2018-03-07T09:27:03Z

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

+  private lazy val checkType = expected match {
+    case _: DecimalType =>
+      (value: Any) => {
+        Seq(classOf[java.math.BigDecimal], classOf[scala.math.BigDecimal], classOf[Decimal])


Can we do 3 instanceOf checks instead? This seems a bit expensive.

hvanhovell · 2018-03-07T09:27:47Z

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

+      }
+    case _: ArrayType =>
+      (value: Any) => {
+        value.getClass.isAssignableFrom(classOf[Seq[_]]) || value.getClass.isArray


What is cheaper? isAssignableFrom or isArray? If it is the latter then we should swap the order.

I have no idea for that and what do u know about it in jvm? @kiszk @rednaxelafx I feel isArray seems cheaper...?

Hi guys, sorry I'm late.

In your new code you're doing:

+ case _: ArrayType => + (value: Any) => { + value.getClass.isArray || value.isInstanceOf[Seq[_]] + }

which is good. xxx.getClass().isAssignableFrom(some_class_literal) in the old version of this PR is actually backwards, it should have been some_class_literal.isAssignableFrom(xxx.getClass()), e.g.

scala> classOf[String].isAssignableFrom(classOf[Object]) res0: Boolean = false scala> classOf[Object].isAssignableFrom(classOf[String]) res1: Boolean = true

and the latter is semantically the same as xxx.isInstanceOf[some_class]. isInstanceOf[] is guaranteed to be at least as fast as some_class_literal.isAssignableFrom(xxx.getClass()), and in general isInstanceOf[] is faster.

xxx.getClass().isArray() has a fixed overhead, whereas isInstanceOf[] can have a fast path slightly faster than the isArray and a slow path that can be much slower than isArray. So putting the isArray check first in your new code makes more sense to me.

For those curious:

In HotSpot, the straightforward interpreter/C1 implementation of xxx.getClass().isArray() path is actually something like:

// for getClass() klazz = xxx._klass; // read the hidden klass pointer field from the object header clazz = klazz._java_mirror; // read the java.lang.Class reference from the Klass // for clazz.isArray(): go through JNI and call the native JVM_IsArrayClass() inside HotSpot klazz1 = clazz->_klass; result = klazz1->oop_is_array();

So a JNI native method call is involved and that's not really fast. But C2 will optimize this into something similar to:

klazz = xxx._klass; result = inlined klazz->oop_is_array();

So that's pretty fast. No need to load the java.lang.Class (aka "Java Mirror") reference anymore.

In the xxx.isInstanceOf[Seq[_]] case, again the interpreter version would go through a JNI native method call, whereas the C1/C2 versions will inline a fast path logic and do a quick comparison against a per-type cache. This fast path check has similar overhead to the C2 isArray() overhead, and the slow path is a slow linear search over an array of implemented interfaces of the klass which can be much slower than the simple isArray check.

Thanks for the valuable info., kris! Based on the comment, the current version is ok to me.

hvanhovell · 2018-03-07T09:32:26Z

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

+      }
+    case _ if ScalaReflection.isNativeType(expected) =>
+      (value: Any) => {
+        value.getClass.isAssignableFrom(ScalaReflection.classForNativeTypeOf(expected))


Let's move the ScalaReflection.classForNativeTypeOf(expected) out of the function body and into a val.

It might also be a bit faster if we use isInstance here. All of these classes do not have subclasses.

hvanhovell · 2018-03-07T09:33:48Z

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

+      }
+    case _ =>
+      (value: Any) => {
+        value.getClass.isAssignableFrom(dataType.asInstanceOf[ObjectType].cls)


Similar as above. Let's move dataType.asInstanceOf[ObjectType].cls out of the function and into a val.

Also where is this code path in the code generated version?

Since I thought the previous version is some difficult to read , I cleaned up the code along with the generated version. How about it?

viirya · 2018-03-07T09:39:45Z

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

+      }
+    case _ =>
+      (value: Any) => {
+        value.getClass.isAssignableFrom(dataType.asInstanceOf[ObjectType].cls)


The external type for PythonUserDefinedType may not necessarily be ObjectType.

oh..., great catch.

hvanhovell · 2018-03-07T09:46:13Z

@maropu looks pretty solid. I left a few comments.

maropu · 2018-03-07T14:21:44Z

@hvanhovell @viirya thanks for kindly reviews. All the comments addressed and check again? thanks!

SparkQA · 2018-03-07T14:29:42Z

Test build #88046 has finished for PR 20757 at commit a204197.

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

SparkQA · 2018-03-07T17:25:03Z

Test build #88047 has finished for PR 20757 at commit d951cd7.

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

SparkQA · 2018-03-08T05:59:31Z

Test build #88069 has finished for PR 20757 at commit dbd8b2f.

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

maropu · 2018-03-08T06:00:41Z

@hvanhovell ping

hvanhovell · 2018-03-08T11:27:51Z

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


  private val errMsg = s" is not a valid external type for schema of ${expected.simpleString}"

+  private lazy val dataTypeClazz = if (dataType.isInstanceOf[ObjectType]) {


Can we fold this into the checkType functions. I'd rather not deference a lazy val every time we check a type.

ok, I'll try

hvanhovell · 2018-03-08T11:28:13Z

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

+    ScalaReflection.classForNativeTypeOf(dataType)
+  }
+
+  private lazy val checkType = expected match {


Just for readability add the type here.

hvanhovell · 2018-03-08T12:57:32Z

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

+        value.getClass.isArray || value.isInstanceOf[Seq[_]]
+      }
+    case _ =>
+      val dataTypeClazz = if (dataType.isInstanceOf[ObjectType]) {


Ok, two more things.

Let's just move this logic into the classForNativeTypeOf function.

Where are we dealing with PythonUserDefinedType or UserDefinedType? The classForNativeTypeOf function does not appear to be handling those.

Both types (PythonUserDefinedType or UserDefinedType) are converted into native types or ObjectType in dataType by externalDataTypeForInput. So, IIUC we can handle the two types here, too.

hvanhovell · 2018-03-08T12:59:14Z

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

@@ -121,6 +121,19 @@ object ScalaReflection extends ScalaReflection {
    case _ => false
  }

+  def classForNativeTypeOf(dt: DataType): Class[_] = dt match {


Shouldn't this match CodeGenerator.boxedType in terms of functionality? See my previous comments, but I am also missing complex types (struct, map & array).

IIUC this function would be better to only handles the native types that have the same format between the Spark SQL internals formats and externals (struct, map, and array have different formats between them). https://github.com/apache/spark/blob/master/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/encoders/RowEncoder.scala#L209 So, this function needs to handle more wider types than CodeGenerator.boxedType.

There is a difference between how we implement an expression and how we use an expression. In this case the implementations should behave the same, and not only in the context in which it is being used.

ok, I'll brush up the code.

@hvanhovell How about the latest commit?

SparkQA · 2018-03-08T15:30:47Z

Test build #88088 has finished for PR 20757 at commit 638a8ac.

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

SparkQA · 2018-03-09T03:20:27Z

Test build #88107 has finished for PR 20757 at commit 66141ea.

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

SparkQA · 2018-03-09T07:24:02Z

Test build #88111 has finished for PR 20757 at commit 32a8dae.

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

hvanhovell · 2018-03-09T11:43:36Z

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

+        value.getClass.isArray || value.isInstanceOf[Seq[_]]
+      }
+    case _ =>
+      val dataTypeClazz = RowEncoder.getClassFromExternalType(dataType)


Does this always return the same result as CodeGenerator.boxedType(dataType) in terms of functionality? I don't think it does, since it misses support for DateType, TimestampType, DecimalType, StringType, StructType, MapType, ArrayType and UserDefinedType. The thing is that it does not really matter how this expression is currently used (for datasets), what matters is how the code generated version is implemented.

ok, I'll recheck again based on the comment. Thanks!

ok, could you check again?

viirya · 2018-03-10T09:05:11Z

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

@@ -1440,7 +1463,7 @@ case class ValidateExternalType(child: Expression, expected: DataType)
        Seq(classOf[java.math.BigDecimal], classOf[scala.math.BigDecimal], classOf[Decimal])
          .map(cls => s"$obj instanceof ${cls.getName}").mkString(" || ")
      case _: ArrayType =>
-        s"$obj instanceof ${classOf[Seq[_]].getName} || $obj.getClass().isArray()"
+        s"$obj.getClass().isArray() || $obj instanceof ${classOf[Seq[_]].getName}"


Why we need to change codegen implementation?

Based on the @rednaxelafx 's comment, I changed this codegen, too. #20757 (comment)

SparkQA · 2018-03-12T07:05:02Z

Test build #88169 has finished for PR 20757 at commit 6dc1fc2.

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

kiszk · 2018-03-12T07:07:31Z

retest this please

SparkQA · 2018-03-12T10:31:06Z

Test build #88170 has finished for PR 20757 at commit 6dc1fc2.

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

SparkQA · 2018-03-12T10:36:34Z

Test build #88171 has finished for PR 20757 at commit 6dc1fc2.

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

maropu · 2018-03-20T03:54:19Z

ping

SparkQA · 2018-04-05T05:31:25Z

Test build #88919 has finished for PR 20757 at commit dd1159b.

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

SparkQA · 2018-04-10T07:05:02Z

Test build #89084 has finished for PR 20757 at commit a787f9a.

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

maropu · 2018-04-10T07:22:48Z

retest this please

SparkQA · 2018-04-10T11:15:15Z

Test build #89090 has finished for PR 20757 at commit a787f9a.

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

maropu · 2018-04-11T02:42:58Z

ping

SparkQA · 2018-04-19T07:05:02Z

Test build #89550 has finished for PR 20757 at commit 24f2879.

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

maropu · 2018-04-19T08:59:51Z

retest this please

SparkQA · 2018-04-19T12:55:05Z

Test build #89566 has finished for PR 20757 at commit 24f2879.

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

hvanhovell · 2018-04-19T15:27:16Z

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


  private val errMsg = s" is not a valid external type for schema of ${expected.simpleString}"

+  // This function is corresponding to `CodeGenerator.boxedType`
+  private def boxedType(dt: DataType): Class[_] = dataType match {


Can you move this to ScalaReflection?

hvanhovell · 2018-04-19T15:28:11Z

@maropu can you update the PR? I have one trivial comment. LGTM otherwise.

maropu · 2018-04-19T22:33:51Z

Many thanks for your checks! I'll fix soon!

SparkQA · 2018-04-20T02:16:38Z

Test build #89599 has finished for PR 20757 at commit 7474811.

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

hvanhovell · 2018-04-20T13:02:00Z

LGTM - merging to master. Thanks!

maropu · 2018-04-20T14:30:10Z

Thanks!

hvanhovell reviewed Mar 7, 2018

View reviewed changes

viirya reviewed Mar 7, 2018

View reviewed changes

maropu force-pushed the SPARK-23595 branch from a204197 to d951cd7 Compare March 7, 2018 14:56

maropu force-pushed the SPARK-23595 branch from d951cd7 to dbd8b2f Compare March 8, 2018 02:30

hvanhovell reviewed Mar 8, 2018

View reviewed changes

maropu force-pushed the SPARK-23595 branch from 638a8ac to 66141ea Compare March 9, 2018 00:11

kiszk mentioned this pull request Mar 9, 2018

[SPARK-23582][SQL] StaticInvoke should support interpreted execution #20753

Closed

maropu force-pushed the SPARK-23595 branch 2 times, most recently from 3662fb6 to 32a8dae Compare March 9, 2018 04:02

maropu mentioned this pull request Mar 9, 2018

[SPARK-23584][SQL] NewInstance should support interpreted execution #20778

Closed

hvanhovell reviewed Mar 9, 2018

View reviewed changes

viirya reviewed Mar 10, 2018

View reviewed changes

maropu force-pushed the SPARK-23595 branch from 6dc1fc2 to dd1159b Compare April 5, 2018 02:00

maropu force-pushed the SPARK-23595 branch from dd1159b to a787f9a Compare April 10, 2018 03:30

maropu force-pushed the SPARK-23595 branch from a787f9a to 24f2879 Compare April 19, 2018 04:13

hvanhovell reviewed Apr 19, 2018

View reviewed changes

maropu added 7 commits April 20, 2018 07:39

ValidateExternalType should support interpreted execution

78de6c8

Fix

4bee661

Fix

647df9f

Fix

9b6b314

Fix

8cec382

Use ScalaReflection.typeBoxedJavaMapping

bede101

Fix

7474811

maropu force-pushed the SPARK-23595 branch from 24f2879 to 7474811 Compare April 19, 2018 22:44

asfgit closed this in 0dd97f6 Apr 20, 2018


		private val errMsg = s" is not a valid external type for schema of ${expected.simpleString}"

		private lazy val dataTypeClazz = if (dataType.isInstanceOf[ObjectType]) {

[SPARK-23595][SQL] ValidateExternalType should support interpreted execution #20757

[SPARK-23595][SQL] ValidateExternalType should support interpreted execution #20757

Conversation

maropu commented Mar 7, 2018

What changes were proposed in this pull request?

How was this patch tested?

SparkQA commented Mar 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maropu Mar 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rednaxelafx Mar 7, 2018 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hvanhovell commented Mar 7, 2018

maropu commented Mar 7, 2018

SparkQA commented Mar 7, 2018

SparkQA commented Mar 7, 2018

SparkQA commented Mar 8, 2018

maropu commented Mar 8, 2018

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

maropu Mar 8, 2018 • edited Loading

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

SparkQA commented Mar 8, 2018

SparkQA commented Mar 9, 2018

SparkQA commented Mar 9, 2018

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

kiszk commented Mar 12, 2018

SparkQA commented Mar 12, 2018

SparkQA commented Mar 12, 2018

maropu commented Mar 20, 2018

SparkQA commented Apr 5, 2018

SparkQA commented Apr 10, 2018

maropu commented Apr 10, 2018

SparkQA commented Apr 10, 2018

maropu commented Apr 11, 2018

SparkQA commented Apr 19, 2018

maropu commented Apr 19, 2018

SparkQA commented Apr 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hvanhovell commented Apr 19, 2018

maropu commented Apr 19, 2018

SparkQA commented Apr 20, 2018

hvanhovell commented Apr 20, 2018

maropu commented Apr 20, 2018

maropu Mar 7, 2018 •

edited

Loading

rednaxelafx Mar 7, 2018 •

edited

Loading

maropu Mar 8, 2018 •

edited

Loading