[SPARK-25559][SQL] Remove the unsupported predicates in Parquet when possible #22574

dbtsai · 2018-09-27T22:16:55Z

What changes were proposed in this pull request?

Currently, in ParquetFilters, if one of the children predicates is not supported by Parquet, the entire predicates will be thrown away. In fact, if the unsupported predicate is in the top level And condition or in the child before hitting Not or Or condition, it can be safely removed.

How was this patch tested?

Tests are added.

dbtsai · 2018-09-27T22:18:06Z

cc @gatorsmile @cloud-fan @HyukjinKwon @dongjoon-hyun @viirya

SparkQA · 2018-09-28T02:16:43Z

Test build #96724 has finished for PR 22574 at commit d49d63b.

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

dongjoon-hyun · 2018-09-28T02:30:57Z

sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/parquet/ParquetFilters.scala

+        // If the unsupported predicate is in the top level `And` condition or in the child
+        // `And` condition before hitting `Not` or `Or` condition, it can be safely removed.
+        (createFilterHelper(nameToParquetField, lhs, canRemoveOneSideInAnd = true),
+          createFilterHelper(nameToParquetField, rhs, canRemoveOneSideInAnd = true)) match {


Thank you for pinging me, @dbtsai .

Here, instead of canRemoveOneSideInAnd = true, we need to use the received canRemoveOneSideInAnd, don't we? Otherwise, this will cause a bug in the nested And clauses. So, could you review and merge the test case, dbtsai#5, and fix the PR accordingly?

Thanks for catching this. I just fixed it.

Add a nested `And` test case

SparkQA · 2018-09-28T03:01:33Z

Test build #96725 has finished for PR 22574 at commit 8c76e31.

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

SparkQA · 2018-09-28T05:40:27Z

Test build #96731 has finished for PR 22574 at commit 29a9aa0.

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

cloud-fan · 2018-09-28T06:27:52Z

sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/parquet/ParquetFilters.scala

-        } yield FilterApi.and(lhsFilter, rhsFilter)
+        // If the unsupported predicate is in the top level `And` condition or in the child
+        // `And` condition before hitting `Not` or `Or` condition, it can be safely removed.
+        (createFilterHelper(nameToParquetField, lhs, canRemoveOneSideInAnd = true),


a bit about style

val lhs = createFilterHelper... val rhs = createFilterHelper... (lhs, rhs) match { ... }

Addressed. Thanks.

SparkQA · 2018-09-28T07:05:02Z

Test build #96735 has finished for PR 22574 at commit d37b2e8.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

dbtsai · 2018-09-28T07:09:07Z

test this again.

cloud-fan · 2018-09-28T07:11:06Z

LGTM, pending jenkins

viirya · 2018-09-28T07:12:55Z

sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/parquet/ParquetFilters.scala

-        // convert b in ('1'). If we only convert a = 2, we will end up with a filter
-        // NOT(a = 2), which will generate wrong results.
-        // Pushing one side of AND down is only safe to do at the top level.
-        // You can see ParquetRelation's initializeLocalJobFunc method as an example.


This example is good to show the cases we can't remove one side. Can we still keep it?

addressed and added more tests.

viirya · 2018-09-28T10:18:45Z

retest this please.

SparkQA · 2018-09-28T14:06:29Z

Test build #96749 has finished for PR 22574 at commit d37b2e8.

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

dongjoon-hyun · 2018-09-28T21:42:49Z

Can we have a more clear title which is described as in the description? Currently, the claim is too broader than the description.

dongjoon-hyun · 2018-09-28T21:48:38Z

Also, cc @rdblue

dbtsai · 2018-09-28T23:56:27Z

I changed the title, and hopefully, it's much more clear now.

SparkQA · 2018-09-29T00:36:10Z

Test build #96775 has finished for PR 22574 at commit 9a9e47f.

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

dongjoon-hyun

+1, LGTM.

dongjoon-hyun · 2018-09-29T00:45:11Z

Merged to master.

…possible ## What changes were proposed in this pull request? Currently, in `ParquetFilters`, if one of the children predicates is not supported by Parquet, the entire predicates will be thrown away. In fact, if the unsupported predicate is in the top level `And` condition or in the child before hitting `Not` or `Or` condition, it can be safely removed. ## How was this patch tested? Tests are added. Closes apache#22574 from dbtsai/removeUnsupportedPredicatesInParquet. Lead-authored-by: DB Tsai <[email protected]> Co-authored-by: Dongjoon Hyun <[email protected]> Co-authored-by: DB Tsai <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]>

…ts in Parquet ## What changes were proposed in this pull request? This is a follow up of #22574. Renamed the parameter and added comments. ## How was this patch tested? N/A Closes #22679 from gatorsmile/followupSPARK-25559. Authored-by: gatorsmile <[email protected]> Signed-off-by: DB Tsai <[email protected]>

…possible ## What changes were proposed in this pull request? Currently, in `ParquetFilters`, if one of the children predicates is not supported by Parquet, the entire predicates will be thrown away. In fact, if the unsupported predicate is in the top level `And` condition or in the child before hitting `Not` or `Or` condition, it can be safely removed. ## How was this patch tested? Tests are added. Closes apache#22574 from dbtsai/removeUnsupportedPredicatesInParquet. Lead-authored-by: DB Tsai <[email protected]> Co-authored-by: Dongjoon Hyun <[email protected]> Co-authored-by: DB Tsai <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]>

…ts in Parquet ## What changes were proposed in this pull request? This is a follow up of apache#22574. Renamed the parameter and added comments. ## How was this patch tested? N/A Closes apache#22679 from gatorsmile/followupSPARK-25559. Authored-by: gatorsmile <[email protected]> Signed-off-by: DB Tsai <[email protected]>

## What changes were proposed in this pull request? Inspired by apache#22574 . We can partially push down top level conjunctive predicates to Orc. This PR improves Orc predicate push down in both SQL and Hive module. ## How was this patch tested? New unit test. Closes apache#22684 from gengliangwang/pushOrcFilters. Authored-by: Gengliang Wang <[email protected]> Signed-off-by: DB Tsai <[email protected]>

Remove unsupported predicates in parquet

8c76e31

dbtsai force-pushed the removeUnsupportedPredicatesInParquet branch from d49d63b to 8c76e31 Compare September 27, 2018 23:10

dbtsai changed the title ~~[SPARK-25556][SQL] Just remove the unsupported predicates in Parquet~~ [SPARK-25559][SQL] Just remove the unsupported predicates in Parquet Sep 27, 2018

Add a nested And test case

7792e99

dongjoon-hyun reviewed Sep 28, 2018

View reviewed changes

Merge pull request #5 from dongjoon-hyun/PR-22574

29a9aa0

Add a nested `And` test case

cloud-fan reviewed Sep 28, 2018

View reviewed changes

address feedback

d37b2e8

viirya reviewed Sep 28, 2018

View reviewed changes

Added more test and doc

9a9e47f

dbtsai changed the title ~~[SPARK-25559][SQL] Just remove the unsupported predicates in Parquet~~ [SPARK-25559][SQL] Remove the unsupported predicates in Parquet when possible Sep 28, 2018

dongjoon-hyun approved these changes Sep 29, 2018

View reviewed changes

asfgit closed this in 5d726b8 Sep 29, 2018

dbtsai deleted the removeUnsupportedPredicatesInParquet branch October 8, 2018 19:28

gatorsmile mentioned this pull request Oct 9, 2018

[SPARK-25559] [FOLLOW-UP] Add comments for partial pushdown of conjuncts in Parquet #22679

Closed

gengliangwang mentioned this pull request Oct 10, 2018

[SPARK-25699][SQL] Partially push down conjunctive predicated in ORC #22684

Closed

[SPARK-25559][SQL] Remove the unsupported predicates in Parquet when possible #22574

[SPARK-25559][SQL] Remove the unsupported predicates in Parquet when possible #22574

Uh oh!

Conversation

dbtsai commented Sep 27, 2018

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

dbtsai commented Sep 27, 2018

Uh oh!

SparkQA commented Sep 28, 2018

Uh oh!

dongjoon-hyun Sep 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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 Sep 28, 2018

Uh oh!

SparkQA commented Sep 28, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Sep 28, 2018

Uh oh!

dbtsai commented Sep 28, 2018

Uh oh!

cloud-fan commented Sep 28, 2018

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!

viirya commented Sep 28, 2018

Uh oh!

SparkQA commented Sep 28, 2018

Uh oh!

dongjoon-hyun commented Sep 28, 2018

Uh oh!

dongjoon-hyun commented Sep 28, 2018

Uh oh!

dbtsai commented Sep 28, 2018

Uh oh!

SparkQA commented Sep 29, 2018

Uh oh!

dongjoon-hyun left a comment

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun commented Sep 29, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

dongjoon-hyun Sep 28, 2018 •

edited

Loading