-
Notifications
You must be signed in to change notification settings - Fork 435
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[GLUTEN-3582][CORE][VL][CH] Refactor filter pushdown logic #4582
[GLUTEN-3582][CORE][VL][CH] Refactor filter pushdown logic #4582
Conversation
Thanks for opening a pull request! Could you open an issue for this pull request on Github Issues? https://github.com/oap-project/gluten/issues Then could you also rename commit message and pull request title in the following format?
See also: |
Run Gluten Clickhouse CI |
1ae57ab
to
51d0c2c
Compare
@liujiayi771 please review it. |
Run Gluten Clickhouse CI |
51d0c2c
to
df72fa7
Compare
Run Gluten Clickhouse CI |
2a104e5
to
7fcdee7
Compare
Run Gluten Clickhouse CI |
1 similar comment
Run Gluten Clickhouse CI |
7fcdee7
to
932d2ca
Compare
Run Gluten Clickhouse CI |
/Benchmark Velox |
===== Performance report for TPCH SF2000 with Velox backend, for reference only ====
|
932d2ca
to
4f3f4e0
Compare
Run Gluten Clickhouse CI |
4f3f4e0
to
3eb78f2
Compare
Run Gluten Clickhouse CI |
Changes to common module looks good to me. Thanks. |
@@ -384,30 +384,27 @@ object FilterHandler extends PredicateHelper { | |||
(ExpressionSet(filters) -- ExpressionSet(scanFilters)).toSeq | |||
|
|||
// Separate and compare the filter conditions in Scan and Filter. | |||
// Push down the remaining conditions in Filter into Scan. | |||
// Try push down the remaining conditions in Filter into Scan. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Try to push down.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's fix in the follow up PR
LGTM. Thanks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
What changes were proposed in this pull request?
Vanilla spark just push down part of filter condition into scan, however gluten can push down all filters in some cases, it was done in #4132. But, Clickhouse backend can not do this in parquet file format. we can add this later
This PR add
postProcessPushDownFilter
inSparkPlanExecApi
,CHSparkPlanExecApi
overwrite this funtion to add its own logic.Note for CH
Consider TPCH 22
c_acctbal > (select avg(c_acctbal) from customer where ...)
. it can be push down before this PR. I disable it for ch backend, since I want to make push down functionality same as vanilla spark first.Update for
ScanTransformerFactory
Before this PR,
createxxxTransformer
assume pass the extra ilters, after this PR, we pass all push down filters, this is because we may remove unsupported filtersHow was this patch tested?
Using existed UT