Skip to content

Conversation

@gengliangwang
Copy link
Member

What changes were proposed in this pull request?

Currently, in ParquetFilters, predicates inside Not operator are considered as unable to perform partial push down.
However, the following cases is still possible for push down:

  1. Not(Or(left, right)) can be conversed as And(Not(left), Not(right))
  2. Not(Not(pred)) can be conversed as pred

Both cases should be quite trivial, since the Not operator should be pushed down by optimization rule BooleanSimplification already.
But I think it should be good to handle such cases in Parquet data source module as well.

How was this patch tested?

New unit test.

@viirya
Copy link
Member

viirya commented Oct 10, 2018

Won't such predicates be simplified at BooleanSimplification rule? If it is simplified during expression optimization, we won't see such predicates in pushed down predicates.

createFilterHelper(nameToParquetField,
sources.And(sources.Not(lhs), sources.Not(rhs)), canPartialPushDownConjuncts = true)

case sources.Not(sources.Not(pred)) if canPartialPushDownConjuncts =>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hm, is this actually reachable?

@gengliangwang
Copy link
Member Author

@viirya @HyukjinKwon I did the code changes and then I found the condition is not reachable, as I have stated in PR description.

Just feel that it won't hurt to have such handling in data source module, the changes in code is short.

I am OK to close this one.

@viirya
Copy link
Member

viirya commented Oct 10, 2018

I prefer not to add code that will not run. Let's see others options too.

@gengliangwang
Copy link
Member Author

It's OK. Close this one.
Thanks for reviewing @viirya @HyukjinKwon

@SparkQA
Copy link

SparkQA commented Oct 10, 2018

Test build #97193 has finished for PR 22687 at commit 0f43db6.

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

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.

4 participants