[SPARK-12439][SQL] Fix toCatalystArray and MapObjects #10391

viirya · 2015-12-19T12:58:18Z

JIRA: https://issues.apache.org/jira/browse/SPARK-12439

In toCatalystArray, we should look at the data type returned by dataTypeFor instead of silentSchemaFor, to determine if the element is native type. An obvious problem is when the element is Option[Int] class, catalsilentSchemaFor will return Int, then we will wrongly recognize the element is native type.

There is another problem when using Option as array element. When we encode data like Seq(Some(1), Some(2), None) with encoder, we will use MapObjects to construct an array for it later. But in MapObjects, we don't check if the return value of lambdaFunction is null or not. That causes a bug that the decoded data for Seq(Some(1), Some(2), None) would be Seq(1, 2, -1), instead of Seq(1, 2, null).

SparkQA · 2015-12-19T14:35:46Z

Test build #48052 has finished for PR 10391 at commit 788a7c6.

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

viirya · 2015-12-21T02:33:36Z

cc @cloud-fan @marmbrus @davies

cloud-fan · 2015-12-21T03:30:00Z

Good catch! One mirror comment, can we write the test in ExpressionEncoderSuite?

marmbrus · 2015-12-21T20:05:05Z

+1 to moving the test

cloud-fan · 2015-12-22T13:32:20Z

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

do we have to add these classes?
I think a simple test could be encodeDecodeTest(Seq(Some(1), None), "seq of option")

We should also test map case.

Besides, encodeDecodeTest(Seq(Some(1), None) may not work because the converted back will be Seq(Some(1), null).

I think I can remove these classes. I will update later.

SparkQA · 2015-12-22T14:04:32Z

Test build #48191 has finished for PR 10391 at commit 180048c.

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

SparkQA · 2015-12-22T14:43:43Z

Test build #48197 has finished for PR 10391 at commit a726fd8.

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

viirya · 2015-12-22T14:45:39Z

retest this please.

cloud-fan · 2015-12-22T15:33:24Z

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

can we use the helper function encodeDecodeTest? It can simplify the test code and give detail error message when it fails, thanks!

Because for Seq(Some(1), None) the converted back result is Seq(Some(1), null), encodeDecodeTest will report it failed. Any suggestion?

That sounds like a bug.

I am not sure about this. Because you can see at ScalaReflectionRelationSuite.scala#L128 where this test is originally coming from, looks like it is expected to have Seq(Some(1), null) converted back for Seq(Some(1), None).

The original implementation in ScalaReflection only had access to the schema as a DataType when it was going to produce external Row objects. This schema is erased in that it can't differentiate Option from null. This is not true for encoders since we know the full type the user is expecting from the provided TypeTag.

OK. I am investigating the now.

SparkQA · 2015-12-22T15:47:32Z

Test build #48198 has finished for PR 10391 at commit a726fd8.

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

viirya · 2015-12-22T16:10:27Z

retest this please.

SparkQA · 2015-12-22T17:52:03Z

Test build #48204 has finished for PR 10391 at commit a726fd8.

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

viirya · 2015-12-23T02:22:52Z

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

~~The part of fixing is from pr #10401. Without this, the following fixing to MapObjects will cause null exception. Can we merge #10401 first then I do rebase here?~~

Waiting #10443 to getting merged too.

SparkQA · 2015-12-23T06:47:46Z

Test build #48226 has finished for PR 10391 at commit 7389e15.

This patch fails from timeout after a configured wait of 250m.
This patch merges cleanly.
This patch adds no public classes.

viirya · 2015-12-23T06:56:55Z

retest this please.

viirya · 2015-12-23T07:14:46Z

retest this please.

SparkQA · 2015-12-23T11:33:24Z

Test build #48232 has finished for PR 10391 at commit 7389e15.

This patch fails from timeout after a configured wait of 250m.
This patch merges cleanly.
This patch adds no public classes.

viirya · 2015-12-23T12:17:52Z

retest this please.

SparkQA · 2015-12-23T13:47:09Z

Test build #48239 has finished for PR 10391 at commit 7389e15.

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

viirya · 2015-12-23T15:34:45Z

retest this please.

SparkQA · 2015-12-23T17:10:37Z

Test build #48245 has finished for PR 10391 at commit f5b9d50.

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

SparkQA · 2015-12-23T17:18:59Z

Test build #48246 has finished for PR 10391 at commit f5b9d50.

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

viirya · 2015-12-24T03:13:40Z

@cloud-fan @marmbrus Are you ok for latest updates?

viirya · 2015-12-28T15:08:59Z

@cloud-fan Can you also review the updates? Thanks.

marmbrus · 2015-12-28T19:52:36Z

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

I'm not sure exactly what is going on here, but this looks like a hack. Ideally this node should not need to know what type its child is to operate correctly.

Let me explain it.

When we pass in an array with None. It will be encoded as null internally. When we decode it back, WrapOption is called to re-construct it.

The logic of MapObjects is to assign an element as null if its given input element is null. So It will not actually go into WrapOption to re-construct a None back. In order to do that, we need to call lambdaFunction even the element is null.

But we can't simply ignore loopVar.isNull and call all kinds of lambdaFunctions. I tried before but for some lambdaFunctions, a null input value causes problematic results.

In the end I can only check if lambdaFunction is WrapOption or not to make the decision here. Do you have other suggestion other than a hack like this here? Thanks.

@marmbrus I have refactored this part and removed the check of WrapOption now. Please take a look it if it is better now. Thanks.

…side CreateStruct.

viirya · 2015-12-29T08:34:24Z

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

#10443 will fix this by using true as default value for propagateNull. As that pr is not merged yet, in order to pass tests now, I set propagateNull as true here. Once #10443 is getting merged, I can remove the two lines here.

viirya · 2015-12-29T08:43:49Z

@cloud-fan Please review this update if you have time to do. Thanks.

SparkQA · 2015-12-29T10:15:29Z

Test build #48407 has finished for PR 10391 at commit 7728124.

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

cloud-fan · 2015-12-29T12:37:18Z

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

same with extractExpressions, we should also strip the top-most If for constructExpression

Unlike extractExpressions, constructExpression hasn't been wrapped with an If here. Because there are two constructorFor methods, and the one wrapped with If is the second one.

cloud-fan · 2015-12-29T12:37:58Z

overall LGTM

viirya · 2015-12-30T03:35:35Z

@marmbrus @cloud-fan If this patch is no problem. Can you merge #10443 first so I can rebase some codes here? Thanks.

viirya · 2015-12-31T03:53:27Z

retest this please.

SparkQA · 2015-12-31T05:38:11Z

Test build #48538 has finished for PR 10391 at commit e541e04.

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

viirya · 2015-12-31T07:37:12Z

@marmbrus I've rebased this for updates. Please take a look if it is ok for you now. Thanks.

viirya · 2016-01-03T16:49:37Z

ping @marmbrus @rxin

viirya · 2016-01-04T23:06:33Z

ping @marmbrus please take a look for the updates, thanks.

marmbrus · 2016-01-05T20:32:38Z

Thanks, merging to master.

JIRA: https://issues.apache.org/jira/browse/SPARK-12439 In toCatalystArray, we should look at the data type returned by dataTypeFor instead of silentSchemaFor, to determine if the element is native type. An obvious problem is when the element is Option[Int] class, catalsilentSchemaFor will return Int, then we will wrongly recognize the element is native type. There is another problem when using Option as array element. When we encode data like Seq(Some(1), Some(2), None) with encoder, we will use MapObjects to construct an array for it later. But in MapObjects, we don't check if the return value of lambdaFunction is null or not. That causes a bug that the decoded data for Seq(Some(1), Some(2), None) would be Seq(1, 2, -1), instead of Seq(1, 2, null). Author: Liang-Chi Hsieh <[email protected]> Closes apache#10391 from viirya/fix-catalystarray.

Fix toCatalystArray and MapObjects.

788a7c6

viirya mentioned this pull request Dec 19, 2015

[SPARK-12293][SQL] Support UnsafeRow in LocalTableScan #10283

Closed

Move test case.

180048c

cloud-fan reviewed Dec 22, 2015
View reviewed changes

Remove unnecessary classes.

a726fd8

cloud-fan reviewed Dec 22, 2015
View reviewed changes

Fix MapObjects.

090b293

viirya reviewed Dec 23, 2015
View reviewed changes

Remove unnecessary import.

7389e15

viirya added 2 commits December 23, 2015 23:12

Add check if genFunction returns an Option.

ae55e01

Check WrapOption.

f5b9d50

marmbrus reviewed Dec 28, 2015
View reviewed changes

Do not check WrapOption. Instead, we should wrap an If expression out…

7728124

…side CreateStruct.

viirya reviewed Dec 29, 2015
View reviewed changes

cloud-fan reviewed Dec 29, 2015
View reviewed changes

viirya added 2 commits December 31, 2015 10:47

Merge remote-tracking branch 'upstream/master' into fix-catalystarray

e0d34a5

Rebase updates.

e541e04

asfgit closed this in d202ad2 Jan 5, 2016

viirya deleted the fix-catalystarray branch December 27, 2023 18:32

[SPARK-12439][SQL] Fix toCatalystArray and MapObjects #10391

[SPARK-12439][SQL] Fix toCatalystArray and MapObjects #10391

Uh oh!

Conversation

viirya commented Dec 19, 2015

Uh oh!

SparkQA commented Dec 19, 2015

Uh oh!

viirya commented Dec 21, 2015

Uh oh!

cloud-fan commented Dec 21, 2015

Uh oh!

marmbrus commented Dec 21, 2015

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 Dec 22, 2015

Uh oh!

SparkQA commented Dec 22, 2015

Uh oh!

viirya commented Dec 22, 2015

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 Dec 22, 2015

Uh oh!

viirya commented Dec 22, 2015

Uh oh!

SparkQA commented Dec 22, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Dec 23, 2015

Uh oh!

viirya commented Dec 23, 2015

Uh oh!

viirya commented Dec 23, 2015

Uh oh!

SparkQA commented Dec 23, 2015

Uh oh!

viirya commented Dec 23, 2015

Uh oh!

SparkQA commented Dec 23, 2015

Uh oh!

viirya commented Dec 23, 2015

Uh oh!

SparkQA commented Dec 23, 2015

Uh oh!

SparkQA commented Dec 23, 2015

Uh oh!

viirya commented Dec 24, 2015

Uh oh!

viirya commented Dec 28, 2015

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!