[SPARK-13996][SQL] Add more not null attributes for Filter codegen by viirya · Pull Request #11810 · apache/spark

viirya · 2016-03-18T03:42:36Z

What changes were proposed in this pull request?

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

Filter codegen finds the attributes not null by checking IsNotNull(a) expression with a condition if child.output.contains(a). However, the current approach to checking it is not comprehensive. We can improve it.

E.g., for this plan:

val rdd = sqlContext.sparkContext.makeRDD(Seq(Row(1, "1"), Row(null, "1"), Row(2, "2")))
val schema = new StructType().add("k", IntegerType).add("v", StringType)
val smallDF = sqlContext.createDataFrame(rdd, schema)
val df = smallDF.filter("isnotnull(k + 1)")

The code snippet generated without this patch:

/* 031 */ protected void processNext() throws java.io.IOException {
/* 032 */ /*** PRODUCE: Filter isnotnull((k#0 + 1)) */
/* 033 */
/* 034 */ /*** PRODUCE: INPUT */
/* 035 */
/* 036 */ while (!shouldStop() && inputadapter_input.hasNext()) {
/* 037 */ InternalRow inputadapter_row = (InternalRow) inputadapter_input.next();
/* 038 */ /*** CONSUME: Filter isnotnull((k#0 + 1)) */
/* 039 */ /* input[0, int] */
/* 040 */ boolean filter_isNull = inputadapter_row.isNullAt(0);
/* 041 */ int filter_value = filter_isNull ? -1 : (inputadapter_row.getInt(0));
/* 042 */
/* 043 */ /* isnotnull((input[0, int] + 1)) */
/* 044 */ /* (input[0, int] + 1) */
/* 045 */ boolean filter_isNull3 = true;
/* 046 */ int filter_value3 = -1;
/* 047 */ /* 048 */ if (!filter_isNull) {
/* 049 */ filter_isNull3 = false; // resultCode could change nullability.
/* 050 */ filter_value3 = filter_value + 1;
/* 051 */ /* 052 */ }
/* 053 */ if (!(!(filter_isNull3))) continue;
/* 054 */ /* 055 */ filter_metricValue.add(1);

With this patch:

/* 031 */ protected void processNext() throws java.io.IOException {
/* 032 */ /*** PRODUCE: Filter isnotnull((k#0 + 1)) */
/* 033 */
/* 034 */ /*** PRODUCE: INPUT */
/* 035 */
/* 036 */ while (!shouldStop() && inputadapter_input.hasNext()) {
/* 037 */ InternalRow inputadapter_row = (InternalRow) inputadapter_input.next();
/* 038 */ /*** CONSUME: Filter isnotnull((k#0 + 1)) */
/* 039 */ /* input[0, int] */
/* 040 */ boolean filter_isNull = inputadapter_row.isNullAt(0);
/* 041 */ int filter_value = filter_isNull ? -1 : (inputadapter_row.getInt(0));
/* 042 */
/* 043 */ if (filter_isNull) continue;
/* 044 */
/* 045 */ filter_metricValue.add(1);

How was this patch tested?

Existing tests.

SparkQA · 2016-03-18T04:59:26Z

Test build #53498 has finished for PR 11810 at commit 8e59a74.

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

SparkQA · 2016-03-18T07:32:42Z

Test build #53511 has finished for PR 11810 at commit 376c6f0.

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

SparkQA · 2016-03-18T09:31:53Z

Test build #53523 has finished for PR 11810 at commit 3d73249.

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

… attributes from conditions.

viirya · 2016-03-19T02:26:52Z


 // The columns that will filtered out by `IsNotNull` could be considered as not nullable.
- private val notNullAttributes = notNullPreds.flatMap(_.references)
+ private val notNullAttributes = notNullPreds.flatMap(_.references).distinct.map(_.exprId)


The attributes from conditions can have different qualifiers than child.output, which makes later indexOf call failed. So here use exprId instead.

SparkQA · 2016-03-19T04:01:22Z

Test build #53592 has finished for PR 11810 at commit 662c4e3.

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

viirya · 2016-03-21T04:34:17Z

cc @davies

viirya · 2016-03-23T08:12:01Z

ping @davies Please take a look at this to see if it is good for you. Thanks.

davies · 2016-03-23T17:01:09Z

 // Split out all the IsNotNulls from condition.
 private val (notNullPreds, otherPreds) = splitConjunctivePredicates(condition).partition {
- case IsNotNull(a) if child.output.contains(a) => true
+ case IsNotNull(a) if a.references.subsetOf(child.outputSet) => true


This is not correct, for example, IsNotNull(IsNull(a)) does not means a can not be null.

ah, you are right. With the NullIntolerant trait introduced in #11809, we can solve this easily. Wait for that PR to be ready then we can re-visit this. Thanks!

…-attrs Conflicts: sql/core/src/main/scala/org/apache/spark/sql/execution/basicOperators.scala

…-attrs

SparkQA · 2016-04-02T08:56:32Z

Test build #54767 has finished for PR 11810 at commit 0c7afe7.

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

viirya · 2016-04-02T10:07:36Z

retest this please.

SparkQA · 2016-04-02T11:31:41Z

Test build #54771 has finished for PR 11810 at commit 0c7afe7.

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

viirya · 2016-04-02T13:24:06Z

retest this please.

viirya · 2016-04-02T13:28:21Z

Looks like StateStoreSuite a bit flaky...

SparkQA · 2016-04-02T14:45:03Z

Test build #54774 has finished for PR 11810 at commit 0c7afe7.

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

viirya · 2016-04-02T23:45:17Z

retest this please.

SparkQA · 2016-04-03T01:16:42Z

Test build #54786 has finished for PR 11810 at commit 0c7afe7.

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

davies · 2016-04-03T02:34:10Z

LGTM

davies · 2016-04-03T02:34:17Z

Merging this into master, thanks!

viirya · 2016-04-03T02:36:13Z

Thanks for reviewing this.

Add more not null attributes for Filter codegen.
8e59a74

Fix bug.
376c6f0

Fix it.
3d73249

Use exprId to deal with different qualifiers between child.outout and…
662c4e3
… attributes from conditions.

viirya reviewed Mar 19, 2016
View reviewed changes

davies reviewed Mar 23, 2016
View reviewed changes

viirya added 2 commits April 2, 2016 07:07

Merge remote-tracking branch 'upstream/master' into add-more-not-null…
65ea0dd
…-attrs Conflicts: sql/core/src/main/scala/org/apache/spark/sql/execution/basicOperators.scala

Merge remote-tracking branch 'upstream/master' into add-more-not-null…
0c7afe7
…-attrs

asfgit closed this in c2f25b1Apr 3, 2016

JoshRosen mentioned this pull request Jun 1, 2019
[SPARK-27915][SQL][WIP] Update logical Filter's output nullability based on IsNotNull conditions #24765
Closed

viirya deleted the add-more-not-null-attrs branch December 27, 2023 18:33

Uh oh!

Conversation

viirya commented Mar 18, 2016

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

SparkQA commented Mar 18, 2016

Uh oh!

SparkQA commented Mar 18, 2016

Uh oh!

SparkQA commented Mar 18, 2016

Uh oh!

viiryaMar 19, 2016

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Mar 19, 2016

Uh oh!

viirya commented Mar 21, 2016

Uh oh!

viirya commented Mar 23, 2016

Uh oh!

daviesMar 23, 2016

Choose a reason for hiding this comment

Uh oh!

viiryaMar 24, 2016

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Apr 2, 2016

Uh oh!

viirya commented Apr 2, 2016

Uh oh!

SparkQA commented Apr 2, 2016

Uh oh!

viirya commented Apr 2, 2016

Uh oh!

viirya commented Apr 2, 2016

Uh oh!

SparkQA commented Apr 2, 2016

Uh oh!

viirya commented Apr 2, 2016

Uh oh!

SparkQA commented Apr 3, 2016

Uh oh!

davies commented Apr 3, 2016

Uh oh!

davies commented Apr 3, 2016

Uh oh!

viirya commented Apr 3, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants