[SPARK-12335][SPARK-12336][SPARK-12341][SPARK-12342][SQL] Fixes several expression nullablility bugs

[liancheng]

This PR fixes several nullability bugs found while investigationg SPARK-12323.

[liancheng]

cc @cloud-fan

[cloud-fan]

Should we assign a default value for it? Or I'm afraid it can't compile...

[liancheng]

This should work under standard Java. Will double check how Janino behaves though.

Update: it works as expected.

[cloud-fan]

retest this please.

[cloud-fan]

For nested fields, how about improving the GetStructField?

[liancheng]

All ExtractValue expressions should be fixed in similar way.

[SparkQA]

Test build #47696 has finished for PR 10296 at commit de8b442.

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

[SparkQA]

Test build #47727 has finished for PR 10296 at commit fe11ed1.

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

[liancheng]

This fixes SPARK-12335.

[liancheng]

Didn't add separate test cases for SPARK-12335 and SPARK-12336 since they were caught by existing test cases.

[liancheng]

test this please

[cloud-fan]

can we set the corrected join type here?

[cloud-fan]

nvm, seems no need.

[liancheng]

Good catch, thanks!

[SparkQA]

Test build #47729 has finished for PR 10296 at commit 43e5743.

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

[liancheng]

Hm, seems that this change reveals a lot of other existing nullability bugs...

[SparkQA]

Test build #47730 has finished for PR 10296 at commit 43e5743.

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

[cloud-fan]

joined.copy(condition = condition)?

[cloud-fan]

should we rename this to joinedColsFromRight?

[liancheng]

I think it's OK. The first line of the comment above already explained the purpose of this variable.

[SparkQA]

Test build #47736 has finished for PR 10296 at commit fa98ae6.

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

[SparkQA]

Test build #47737 has finished for PR 10296 at commit cfcc6df.

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

[cloud-fan]

LGTM, can we fix the nested fields in a follow-up PR?

[liancheng]

@cloud-fan Yeah, that's the plan. Thanks for the review!

[marmbrus]

Hey guys, I think all this nullability clean up is great, but I afraid that the changes to BoundReference are in conflict with something @nongli and @davies are trying to accomplish. Basically, for performance reasons we should be able to skip null checks for columns that are not nullable since bitset operations and branches are pretty expensive. If we are trying to solve SPARK-12323 then I think the right place to do it is probably in NewInstance.

[marmbrus]

I think ideally we would not do this check at all when nullable = false.

[liancheng]

@marmbrus I also found that NewInstance and MapObjects need to be updated for complete runtime nullability checking, especially for nested fields. As stated in the PR description, planning to fix this part in a follow-up PR.

So how about updating this PR to only fix all the nullability mismatches without touching BoundReference, and then continue fixing NewInstance etc. in another one?

[marmbrus]

That sounds good to me.

[SparkQA]

Test build #47770 has finished for PR 10296 at commit 05c36e5.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds the following public classes (experimental):\n * case class WrapOption(child: Expression, optType: DataType)\n

[davies]

@liancheng I currently working on nullability of expressions, could you hold this PR a little bit?

[liancheng]

@davies As commented above, I'll reshape this PR to only fix those wrong nullability issues without touching BoundReference, so that it won't conflict with you changes.

[liancheng]

Changes in this file fixes SPARK-12336.

[SparkQA]

Test build #47808 has finished for PR 10296 at commit 7e35e37.

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

[SparkQA]

Test build #47807 has finished for PR 10296 at commit d540b86.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds the following public classes (experimental):\n * case class WrapOption(child: Expression, optType: DataType)\n

[liancheng]

retest this please

The last build failure seems to be irrelevant.

[liancheng]

@marmbrus Reshaped this PR to only fix those nullability bugs.

After some more investigation, now I don't think we can resolve SPARK-12323 by fixing NewInstance. The reasons are:

1. ExpressionEncoders are always created using reflection based schema inference, which implies that the only non-nullable fields within a fromRowExpression are those of unboxed primitive types.
2. Unboxed primitive fields are always retrieved using code generated in BoundReference rathar than NewInstance, since NewInstance is only used to build objects.

Since we would like to avoid per row runtime null checking and branching cost (what @davies and @nongli are working on), we'll have to assume the nullability of input data always match the schema of the ExpressionEncoder being used. Another not quite appealing choice is to add an option to generate code with null checking, so that users can use it for debugging purposes.

On the other hand, we can and should ensure nullability of the underlying logical plan is consistent with the Dataset while constructing a Dataset. For example, currently the following case works:

valrowRDD= sqlContext.sparkContext.parallelize(Seq(Row("hello"), Row(null)))
valschema=StructType(Seq(StructField("_1", StringType, nullable =false)))
valdf= sqlContext.createDataFrame(rowRDD, schema)
df.as[Tuple1[String]].collect().foreach(println)
// Output://// (hello)// (null)

This analysis time checking can be done in ExpressionEncoder.resolve by comparing schemata of the logical plan and the encoder. Opened PR #10331 for this check.

[marmbrus]

After some more investigation, now I don't think we can resolve SPARK-12323 by fixing NewInstance.

You could augment NewInstance to understand which arguments are primitive types or you could infer this using reflection on the constructor. Though I think the resulting expression tree would be clearer if you just created the following new expression and inserted it into the tree for primitive types that are the children of a NewInstance.

caseclassAssertNotNull(path: String, child: Expression) ...

I think there may be some confusion about the schema guarantees for encoders and their expressions. When there is a primitive type, the corresponding toRowExpressions will be non-nullable, since the data is coming from an object that can't possible store a null value. In contrast, the fromRowExpression are reading from arbitrary input data, and thus their nullablity has nothing to do with the structure of the target object. Instead this information is coming from the schema that the encoder is resolved/bound to. Therefore, using the nullable bit here is incorrect. nullable should always be thought of as a promise about the input data that we can use for optimization, not a constraint enforcement mechanism.

Another not quite appealing choice is to add an option to generate code with null checking, so that users can use it for debugging purposes.

I think it would actually be very good to have assertions that null data does not appear where it is not expected. When we actually start using this information we are almost certainly going to find more places we are not propagating the information correctly. However, we need to ensure that these are elided in production to avoid invalidating the optimization this information is supposed to enable.

This analysis time checking can be done in ExpressionEncoder.resolve by comparing schemata of the logical plan and the encoder.

I don't agree that you can verify the problem with this code statically. Creating a schema that says that _1 is not nullable is valid, even though a String can be null in the JVM. Again, this is a promise about the data itself, so you can't assert that there is a problem until that promise is violated and you see a null value in this column.

That said, trusting the user to get this right has led to confusion in the past. So I would propose that we do add validations at the sqlContext.createDataX boundaries. Internally, though, we should trust this bit so that we can avoid unnecessary/expensive null checks.

[liancheng]

Closing this one since PR #10333 already covers all the nullability bugs fixed in this one.

[liancheng]

@marmbrus Thanks a lot for the detailed explanation! According to your suggestion, I've added AssertNotNull in PR #10331.