[SPARK-25734][SQL] Literal should have a value corresponding to dataType #22724

maropu · 2018-10-15T10:47:16Z

What changes were proposed in this pull request?

Literal.value should have a value a value corresponding to dataType. This pr added code to verify it and fixed the existing tests to do so.

How was this patch tested?

Modified the existing tests.

cloud-fan · 2018-10-15T13:38:16Z

LGTM

ueshin · 2018-10-15T14:47:29Z

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

+      case StringType => v.isInstanceOf[UTF8String]
+      case _: StructType => v.isInstanceOf[InternalRow]
+      case _: ArrayType => v.isInstanceOf[ArrayData]
+      case _: MapType => v.isInstanceOf[MapData]


Should validate recursively for StructType, ArrayType, and MapType?

ah good point!

yea, good catch ....!

SparkQA · 2018-10-15T14:51:38Z

Test build #97384 has finished for PR 22724 at commit 9115d26.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
s\"but $

dongjoon-hyun · 2018-10-15T19:28:14Z

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

+    }
+    require(v == null || doValidate(v, dataType),
+      s"Literal must have a corresponding value to ${dataType.catalogString}, " +
+      s"but ${if (v != null) s"class ${Utils.getSimpleName(v.getClass)}" else "null"} found.")


but null found cannot happen logically.

oh, yes. I'll update.

SparkQA · 2018-10-16T01:25:13Z

Test build #97419 has finished for PR 22724 at commit b00f4bb.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
s\"but class $

cloud-fan · 2018-10-16T01:29:29Z

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

+      case BinaryType => v.isInstanceOf[Array[Byte]]
+      case StringType => v.isInstanceOf[UTF8String]
+      case st: StructType =>
+        v.isInstanceOf[GenericInternalRow] && {


this is definitely not true. can we do

v.isInstanceOf[InternalRow] && { val row = v.asInstanceOf[InternalRow] st.fields.map(_.dataType).zipWithIndex.foreach { case (dt, i) => doValidate(row.get(i, dt), dt) } }

ah, yes. ok, I'll fix.

ueshin · 2018-10-16T03:38:09Z

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

+            doValidate(map.keyArray.array.head, mt.keyType) &&
+              doValidate(map.valueArray.array.head, mt.valueType)
+          }
+        }


I'm wondering whether we don't need to check the whole elements for ArrayType and MapType.

Since the whole element check seems to be expensive, the current one is ok to me.

HyukjinKwon

LGTM too if the tests pass

ueshin · 2018-10-16T03:53:58Z

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

+        }
+      case ObjectType(cls) => cls.isInstance(v)
+      case udt: UserDefinedType[_] => doValidate(v, udt.sqlType)
+      case _ => false


~~We need to add NullType case?~~
nvm, not needed usually.

SparkQA · 2018-10-16T04:04:10Z

Test build #97427 has finished for PR 22724 at commit a30e9ce.

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

cloud-fan · 2018-10-16T04:21:30Z

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

+      case BinaryType => v.isInstanceOf[Array[Byte]]
+      case StringType => v.isInstanceOf[UTF8String]
+      case st: StructType =>
+        v.isInstanceOf[InternalRow] && {


can we do the same for array and map?

cloud-fan · 2018-10-16T04:22:41Z

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

+      case mt: MapType =>
+        v.isInstanceOf[ArrayBasedMapData] && {
+          val map = v.asInstanceOf[ArrayBasedMapData]
+          map.numElements() == 0 || {


we don't need this. The array validation already consider numElements

you suggested like this?

case mt: MapType => v.isInstanceOf[MapData] && { val map = v.asInstanceOf[MapData] doValidate(map.keyArray(), ArrayType(mt.keyType)) && doValidate(map.valueArray(), ArrayType(mt.valueType)) }

SparkQA · 2018-10-16T05:02:42Z

Test build #97431 has finished for PR 22724 at commit 56b4778.

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

cloud-fan · 2018-10-16T05:07:59Z

LGTM pending jenkins

SparkQA · 2018-10-16T05:13:47Z

Test build #97432 has finished for PR 22724 at commit 4df5abd.

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

maropu · 2018-10-16T05:17:18Z

I'm checking the reason of the test failures...

SparkQA · 2018-10-16T05:28:33Z

Test build #97433 has finished for PR 22724 at commit 1c042f3.

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

SparkQA · 2018-10-16T09:07:00Z

Test build #97439 has finished for PR 22724 at commit 60a7793.

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

SparkQA · 2018-10-16T09:35:58Z

Test build #97443 has finished for PR 22724 at commit 77f65f9.

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

mgaido91 · 2018-10-16T10:48:07Z

LGTM, but this is a breaking change, so I'd suggest to do it only for 3.0 and add a note to the migration guide. WDYT?

maropu · 2018-10-16T10:57:28Z

yea, its ok to merge this into v3.0 only. But, we need to update the guide?

cloud-fan · 2018-10-16T12:34:01Z

Literal is an internal API, and AFAIK end-users can't construct an invalid Literal with public APIs. If they can, then it's a bug and we have a problem...

mgaido91 · 2018-10-16T12:49:06Z

yes, you're right. So we don't need any migration note. But what if a user uses Literal directly? He/she shouldn't, but we cannot exclude this is happening. So I'd still target this for 3.0 only.

cloud-fan · 2018-10-16T12:57:33Z

yea +1 on 3.0 only, this is kind of a developer API, advanced users may use it.

SparkQA · 2018-10-16T13:18:08Z

Test build #97451 has finished for PR 22724 at commit a2f0288.

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

SparkQA · 2018-10-16T17:21:07Z

Test build #97457 has finished for PR 22724 at commit 04a535d.

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

dongjoon-hyun

+1, LGTM.

cloud-fan · 2018-10-17T03:02:53Z

thanks, merging to master!

## What changes were proposed in this pull request? `Literal.value` should have a value a value corresponding to `dataType`. This pr added code to verify it and fixed the existing tests to do so. ## How was this patch tested? Modified the existing tests. Closes apache#22724 from maropu/SPARK-25734. Authored-by: Takeshi Yamamuro <[email protected]> Signed-off-by: Wenchen Fan <[email protected]>

Fix

9115d26

maropu changed the title ~~[SPARK-25734][SQL}~~ [SPARK-25734][SQL] Literal should have a value corresponding to dataType Oct 15, 2018

maropu mentioned this pull request Oct 15, 2018

[SPARK-25498][SQL] InterpretedMutableProjection should handle UnsafeRow #22512

Closed

ueshin reviewed Oct 15, 2018

View reviewed changes

dongjoon-hyun reviewed Oct 15, 2018

View reviewed changes

Fix

b00f4bb

cloud-fan reviewed Oct 16, 2018

View reviewed changes

Fix

a30e9ce

ueshin reviewed Oct 16, 2018

View reviewed changes

HyukjinKwon approved these changes Oct 16, 2018

View reviewed changes

ueshin reviewed Oct 16, 2018

View reviewed changes

cloud-fan reviewed Oct 16, 2018

View reviewed changes

maropu force-pushed the SPARK-25734 branch from e590fe5 to 56b4778 Compare October 16, 2018 04:35

Fix

4df5abd

maropu force-pushed the SPARK-25734 branch from 56b4778 to 4df5abd Compare October 16, 2018 04:38

Fix

1c042f3

maropu force-pushed the SPARK-25734 branch from 60a7793 to 77f65f9 Compare October 16, 2018 07:32

maropu force-pushed the SPARK-25734 branch from 77f65f9 to a2f0288 Compare October 16, 2018 10:29

Fix

04a535d

maropu force-pushed the SPARK-25734 branch from a2f0288 to 04a535d Compare October 16, 2018 13:33

dongjoon-hyun approved these changes Oct 16, 2018

View reviewed changes

asfgit closed this in a9f685b Oct 17, 2018

[SPARK-25734][SQL] Literal should have a value corresponding to dataType #22724

[SPARK-25734][SQL] Literal should have a value corresponding to dataType #22724

Uh oh!

Conversation

maropu commented Oct 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

cloud-fan commented Oct 15, 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!

SparkQA commented Oct 15, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Oct 16, 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!

HyukjinKwon left a comment

Choose a reason for hiding this comment

Uh oh!

ueshin Oct 16, 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!

SparkQA commented Oct 16, 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!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Oct 16, 2018

Uh oh!

cloud-fan commented Oct 16, 2018

Uh oh!

SparkQA commented Oct 16, 2018

Uh oh!

maropu commented Oct 16, 2018

Uh oh!

SparkQA commented Oct 16, 2018

Uh oh!

SparkQA commented Oct 16, 2018

Uh oh!

SparkQA commented Oct 16, 2018

Uh oh!

mgaido91 commented Oct 16, 2018

Uh oh!

maropu commented Oct 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cloud-fan commented Oct 16, 2018

Uh oh!

mgaido91 commented Oct 16, 2018

Uh oh!

cloud-fan commented Oct 16, 2018

maropu commented Oct 15, 2018 •

edited

Loading

ueshin Oct 16, 2018 •

edited

Loading

maropu commented Oct 16, 2018 •

edited

Loading