Skip to content

Conversation

@viirya
Copy link
Member

@viirya viirya commented Oct 27, 2016

What changes were proposed in this pull request?

In Optimizer, we will infer additional conditions from Filter's constraints which include IsNotNull. Some inferred IsNotNulls are useful because they can be short circuit for other predicates which refer the same attributes. But for an IsNotNull condition, we also infer other IsNotNull from it. Those IsNotNulls are redundant. We don't need to include them.

E.g., for a condition like IsNotNull(a + Rand()), an additional IsNotNull(a) will be added into Filter's condition. So we need to evaluate IsNotNull(a + Rand()) && IsNotNull(a). Actually we don't need IsNotNull(a) because it is guaranteed by the original IsNotNull(a + Rand()).

How was this patch tested?

Jenkins tests.

Please review https://cwiki.apache.org/confluence/display/SPARK/Contributing+to+Spark before opening a pull request.

@viirya
Copy link
Member Author

viirya commented Oct 27, 2016

This is not a serious issue. And it doesn't affect correctness. I would left this to @cloud-fan or @hvanhovell decide if we want to fix it or not.

@viirya viirya changed the title [SQL] Don't add inferred redundant isnotnull condition from constraints [SPARK-18130][SQL] Don't add inferred redundant isnotnull condition from constraints Oct 27, 2016
@SparkQA
Copy link

SparkQA commented Oct 27, 2016

Test build #67613 has finished for PR 15653 at commit 86443c4.

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

@viirya
Copy link
Member Author

viirya commented Nov 7, 2016

Close it for now since no interest from committers.

@viirya viirya closed this Nov 7, 2016
@viirya viirya deleted the fix-inferred-isnotnull-cond branch December 27, 2023 18:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants