-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
fix bug the optimizer rule filter push down #2039
Conversation
@liukun4515 @Dandandan @alamb @houqp @yjshen PTAL, thanks!😃❤ |
It's found during I add the I found that The filter split is caused by this bug. |
Now it's fixed. explain verbose select c1, c2 from test where c3 = true and c2 = 0.000001;
+-------------------------------------------------------+---------------------------------------------------------------------------------------------------------------------------------+
| plan_type | plan |
+-------------------------------------------------------+---------------------------------------------------------------------------------------------------------------------------------+
| initial_logical_plan | Projection: #test.c1, #test.c2 |
| | Filter: #test.c3 = Boolean(true) AND #test.c2 = Float64(0.000001) |
| | TableScan: test projection=None |
| logical_plan after simplify_expressions | Projection: #test.c1, #test.c2 |
| | Filter: #test.c3 AND #test.c2 = Float64(0.000001) AS test.c3 = Boolean(true) AND test.c2 = Float64(0.000001) |
| | TableScan: test projection=None |
| logical_plan after eliminate_filter | SAME TEXT AS ABOVE |
| logical_plan after common_sub_expression_eliminate | SAME TEXT AS ABOVE |
| logical_plan after eliminate_limit | SAME TEXT AS ABOVE |
| logical_plan after projection_push_down | Projection: #test.c1, #test.c2 |
| | Filter: #test.c3 AND #test.c2 = Float64(0.000001) AS test.c3 = Boolean(true) AND test.c2 = Float64(0.000001) |
| | TableScan: test projection=Some([0, 1, 2]) |
| logical_plan after filter_push_down | Projection: #test.c1, #test.c2 |
| | Filter: #test.c3 AND #test.c2 = Float64(0.000001) |
| | TableScan: test projection=Some([0, 1, 2]), filters=[#test.c3, #test.c2 = Float64(0.000001)] |
| logical_plan after limit_push_down | SAME TEXT AS ABOVE |
| logical_plan after SingleDistinctAggregationToGroupBy | SAME TEXT AS ABOVE |
| logical_plan after ToApproxPerc | SAME TEXT AS ABOVE |
| logical_plan after simplify_expressions | SAME TEXT AS ABOVE |
| logical_plan after eliminate_filter | SAME TEXT AS ABOVE |
| logical_plan after common_sub_expression_eliminate | SAME TEXT AS ABOVE |
| logical_plan after eliminate_limit | SAME TEXT AS ABOVE |
| logical_plan after projection_push_down | SAME TEXT AS ABOVE |
| logical_plan after filter_push_down | SAME TEXT AS ABOVE |
| logical_plan after limit_push_down | SAME TEXT AS ABOVE |
| logical_plan after SingleDistinctAggregationToGroupBy | SAME TEXT AS ABOVE |
| logical_plan after ToApproxPerc | SAME TEXT AS ABOVE |
| logical_plan | Projection: #test.c1, #test.c2 |
| | Filter: #test.c3 AND #test.c2 = Float64(0.000001) |
| | TableScan: test projection=Some([0, 1, 2]), filters=[#test.c3, #test.c2 = Float64(0.000001)] |
I think the change looks good! Can we add a test to make sure we get the correct plan now without splitting the original expressions into multiple filters? |
LGTM +1 on adding a unit test |
Yes, add unit test is necessary. |
The result of unit test: Before
After
|
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.
Makes sense to me -- thank you for persevering @jackwener and finding a good solution. This change seems much more preferable than #2026 to me.
I also double checked by running this test locally without the change and with the change
Which issue does this PR close?
Closes #2038.
What changes are included in this PR?
Fix the bug due to break loop cause
used_columns
is wrong.It will cause the
issue_filters
get wrongused_columns
paramAre there any user-facing changes?
None