bugfix: remove cnf_rewrite in push_down_filter #4825
Conversation
| @@ -1561,7 +1561,7 @@ async fn reduce_left_join_2() -> Result<()> { | |||
| let expected = vec![ | |||
| "Explain [plan_type:Utf8, plan:Utf8]", | |||
| " Projection: t1.t1_id, t1.t1_name, t1.t1_int, t2.t2_id, t2.t2_name, t2.t2_int [t1_id:UInt32;N, t1_name:Utf8;N, t1_int:UInt32;N, t2_id:UInt32;N, t2_name:Utf8;N, t2_int:UInt32;N]", | |||
| " Filter: t2.t2_int < UInt32(10) OR t1.t1_int > UInt32(2) [t1_id:UInt32;N, t1_name:Utf8;N, t1_int:UInt32;N, t2_id:UInt32;N, t2_name:Utf8;N, t2_int:UInt32;N]", | |||
| " Filter: t2.t2_int < UInt32(10) OR t1.t1_int > UInt32(2) AND t2.t2_name != Utf8(\"w\") [t1_id:UInt32;N, t1_name:Utf8;N, t1_int:UInt32;N, t2_id:UInt32;N, t2_name:Utf8;N, t2_int:UInt32;N]", | |||
There was a problem hiding this comment.
we can find AND t2.t2_name != Utf8(\"w\") is lost due to original cnf_rewrite()
|
PTAL @alamb @Ted-Jiang |
There was a problem hiding this comment.
Thanks @jackwener -- I think this rewrite was added by @Ted-Jiang in #3903 so perhaps he can weigh in on this proposal
Given it fixes a bug I am inclined to merge it in but will wait for @Ted-Jiang for a few days before doing so
I found https://github.com/apache/arrow-datafusion/pull/4825/files?w=1 shows the diffs much easier
| push_down_all_join(predicates, &filter.input, left, right, vec![])? | ||
| } | ||
| LogicalPlan::TableScan(scan) => { | ||
| let mut new_scan_filters = scan.filters.clone(); | ||
| let mut new_predicate = vec![]; | ||
|
|
||
| let filter_predicates = utils::split_conjunction_owned( | ||
| utils::cnf_rewrite(filter.predicate.clone()), |
There was a problem hiding this comment.
I believe this is the only place that calls cnf_rewrite
https://github.com/search?q=repo%3Aapache%2Farrow-datafusion%20cnf_rewrite&type=code
so if we are going to stop calling it, I think we should also remove cnf_rewrite from the code
There was a problem hiding this comment.
I'll plan to merge this over the weekend unless we hear something from @Ted-Jiang
|
Thanks again @jackwener |
|
Benchmark runs are scheduled for baseline = a6d067c and contender = 2db3d2e. 2db3d2e is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
Which issue does this PR close?
Closes #4822.
Rationale for this change
I found this error when I finish #4465
What changes are included in this PR?
RewriteDisjunctivePredicate)Are these changes tested?
Are there any user-facing changes?