[SPARK-14139][SQL] RowEncoder should preserve schema nullability #12364

cloud-fan · 2016-04-13T17:05:21Z

What changes were proposed in this pull request?

The problem is: In RowEncoder, we use Invoke to get the field of an external row, which lose the nullability information. This PR creates a GetExternalRowField expression, so that we can preserve the nullability info.

TODO: simplify the null handling logic in RowEncoder, to remove so many if branches, in follow-up PR.

How was this patch tested?

new tests in RowEncoderSuite

Note that, This PR takes over #11980, with a little simplification, so all credits should go to @koertkuipers

…ng extractors

…test RowEncoderSuite:encode/decode:Product

…ernal

SparkQA · 2016-04-13T18:36:21Z

Test build #55729 has finished for PR 12364 at commit f8c0bfe.

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

SparkQA · 2016-04-13T18:38:29Z

Test build #55730 has finished for PR 12364 at commit 2976d26.

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

yhuai · 2016-04-13T18:58:27Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/encoders/RowEncoderSuite.scala

+    val encoder = RowEncoder(schema)
+    assert(encoder.serializer.length == 1)
+    assert(encoder.serializer.head.dataType == IntegerType)
+    assert(encoder.serializer.head.nullable == false)


Will we throw an exception if there is a null in the data?

This is a good point, actually we will, we should add the runtime null check like we did for product encoder

SparkQA · 2016-04-14T19:03:48Z

Test build #55831 has finished for PR 12364 at commit ad8c9ef.

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

SparkQA · 2016-04-14T19:34:02Z

Test build #55828 has finished for PR 12364 at commit 2976d26.

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

rxin · 2016-04-16T07:32:38Z

Is the TODO in the pr description still valid?

cloud-fan · 2016-04-18T03:44:59Z

yea, it's still valid, but I'd like to do it in follow-ups, as this PR is taken over from other people, I don't want to enlarge the scope too much.

rxin · 2016-04-18T03:49:26Z

OK please label it as a todo for future pr; otherwise it is difficult to tell if it is meant to be done as the current one.

SparkQA · 2016-04-18T19:19:46Z

Test build #56083 has finished for PR 12364 at commit 4ee75e9.

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

SparkQA · 2016-04-19T02:35:34Z

Test build #56165 has finished for PR 12364 at commit 645f0a0.

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

yhuai · 2016-04-19T17:53:59Z

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

-    val inputObject = BoundReference(0, ObjectType(cls), nullable = true)
-    // We use an If expression to wrap extractorsFor result of StructType
-    val serializer = serializerFor(inputObject, schema).asInstanceOf[If].falseValue
+    val inputObject = BoundReference(0, ObjectType(cls), nullable = false)


Reason of this change?

The input object should never be null, we also use this assumption in ExpressionEncoder

Actually ExpressionEncoder allows null input now. But I agree that for RowEncoder this assumption is reasonable.

Probably add a comment here. It's not super intuitive.

SparkQA · 2016-05-03T07:15:33Z

Test build #57607 has finished for PR 12364 at commit b1e12b5.

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

SparkQA · 2016-05-03T13:23:09Z

Test build #57633 has finished for PR 12364 at commit 7c4c91c.

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

SparkQA · 2016-05-04T06:33:29Z

Test build #57716 has finished for PR 12364 at commit 8870650.

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

cloud-fan · 2016-05-04T06:50:12Z

cc @liancheng

liancheng · 2016-05-05T08:02:34Z

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

-    // We use an If expression to wrap extractorsFor result of StructType
-    val serializer = serializerFor(inputObject, schema).asInstanceOf[If].falseValue
+    val inputObject = BoundReference(0, ObjectType(cls), nullable = false)
+    val serializer = serializerFor(inputObject, schema)


This change is also because we don't allow null input Rows now, right?

yea, there is no if anymore for the top row object.

liancheng · 2016-05-05T10:07:52Z

Mostly LGTM except a few minor comments.

SparkQA · 2016-05-05T13:30:55Z

Test build #57879 has finished for PR 12364 at commit 98e1463.

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

SparkQA · 2016-05-05T16:06:19Z

Test build #57894 has finished for PR 12364 at commit 18aa126.

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

liancheng · 2016-05-05T17:06:53Z

LGTM, merging to master and branch-2.0.

## What changes were proposed in this pull request? The problem is: In `RowEncoder`, we use `Invoke` to get the field of an external row, which lose the nullability information. This PR creates a `GetExternalRowField` expression, so that we can preserve the nullability info. TODO: simplify the null handling logic in `RowEncoder`, to remove so many if branches, in follow-up PR. ## How was this patch tested? new tests in `RowEncoderSuite` Note that, This PR takes over #11980, with a little simplification, so all credits should go to koertkuipers Author: Wenchen Fan <[email protected]> Author: Koert Kuipers <[email protected]> Closes #12364 from cloud-fan/nullable. (cherry picked from commit 55cc1c9) Signed-off-by: Cheng Lian <[email protected]>

…Encoder ## What changes were proposed in this pull request? SPARK-15241: We now support java decimal and catalyst decimal in external row, it makes sense to also support scala decimal. SPARK-15242: This is a long-standing bug, and is exposed after #12364, which eliminate the `If` expression if the field is not nullable: ``` val fieldValue = serializerFor( GetExternalRowField(inputObject, i, externalDataTypeForInput(f.dataType)), f.dataType) if (f.nullable) { If( Invoke(inputObject, "isNullAt", BooleanType, Literal(i) :: Nil), Literal.create(null, f.dataType), fieldValue) } else { fieldValue } ``` Previously, we always use `DecimalType.SYSTEM_DEFAULT` as the output type of converted decimal field, which is wrong as it doesn't match the real decimal type. However, it works well because we always put converted field into `If` expression to do the null check, and `If` use its `trueValue`'s data type as its output type. Now if we have a not nullable decimal field, then the converted field's output type will be `DecimalType.SYSTEM_DEFAULT`, and we will write wrong data into unsafe row. The fix is simple, just use the given decimal type as the output type of converted decimal field. These 2 issues was found at #13008 ## How was this patch tested? new tests in RowEncoderSuite Author: Wenchen Fan <[email protected]> Closes #13019 from cloud-fan/encoder-decimal. (cherry picked from commit d8935db) Signed-off-by: Davies Liu <[email protected]>

koertkuipers and others added 11 commits March 26, 2016 16:14

change RowEncoder to respect nullable for struct fields when generati…

847d7c7

…ng extractors

merge from master

b500c8b

make scalastyle happy

3e32b6a

Merge branch 'master' into feat-rowencoder-nullable

15adc9c

first try at using GetExternalRowField instead of Invoke. fails unit …

2600a2e

…test RowEncoderSuite:encode/decode:Product

Merge branch 'master' into feat-rowencoder-nullable

50c0005

merge from master

05e5f19

fix for extractsFor renamed to serializerFor

4b140e5

fix pattern match in GetExternalRowField where the data types are ext…

e9a9a30

…ernal

Merge remote-tracking branch 'origin/master' into nullable

8231a15

some simplification

2976d26

cloud-fan force-pushed the nullable branch from f8c0bfe to 2976d26 Compare April 13, 2016 17:09

yhuai reviewed Apr 13, 2016
View reviewed changes

cloud-fan closed this Apr 14, 2016

cloud-fan reopened this Apr 14, 2016

cloud-fan added 2 commits April 15, 2016 02:09

Merge remote-tracking branch 'origin/master' into nullable

7a1877a

add runtime null check

ad8c9ef

add test

4ee75e9

cloud-fan added 2 commits April 19, 2016 08:58

Merge remote-tracking branch 'origin/master' into nullable

ded8800

rebase

645f0a0

yhuai reviewed Apr 19, 2016
View reviewed changes

Merge remote-tracking branch 'origin/master' into nullable

7c4c91c

cloud-fan force-pushed the nullable branch from b1e12b5 to 7c4c91c Compare May 3, 2016 11:55

Merge remote-tracking branch 'origin/master' into nullable

8870650

liancheng reviewed May 5, 2016
View reviewed changes

address comments

98e1463

oops...

18aa126

asfgit closed this in 55cc1c9 May 5, 2016

cloud-fan mentioned this pull request May 10, 2016

[SPARK-15241][SPARK-15242][SQL] fix 2 decimal-related issues in RowEncoder #13019

Closed

[SPARK-14139][SQL] RowEncoder should preserve schema nullability #12364

[SPARK-14139][SQL] RowEncoder should preserve schema nullability #12364

Uh oh!

Conversation

cloud-fan commented Apr 13, 2016 • 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!

SparkQA commented Apr 13, 2016

Uh oh!

SparkQA commented Apr 13, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Apr 14, 2016

Uh oh!

SparkQA commented Apr 14, 2016

Uh oh!

rxin commented Apr 16, 2016

Uh oh!

cloud-fan commented Apr 18, 2016

Uh oh!

rxin commented Apr 18, 2016

Uh oh!

SparkQA commented Apr 18, 2016

Uh oh!

SparkQA commented Apr 19, 2016

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 May 3, 2016

Uh oh!

SparkQA commented May 3, 2016

Uh oh!

SparkQA commented May 4, 2016

Uh oh!

cloud-fan commented May 4, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liancheng commented May 5, 2016

Uh oh!

SparkQA commented May 5, 2016

Uh oh!

SparkQA commented May 5, 2016

Uh oh!

liancheng commented May 5, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

cloud-fan commented Apr 13, 2016 •

edited

Loading