Remove reundant filters (e.g. c> 5 AND c>5 --> c>5) by jgoday · Pull Request #436 · apache/datafusion

jgoday · 2021-05-28T18:46:38Z

Which issue does this PR close?

Closes#410.

Rationale for this change

Uses boolean algebra laws to simplify binary boolean expressions.

What changes are included in this PR?

New RemoveDuplicateFilters optimizer.

Are there any user-facing changes?

jgoday · 2021-05-28T20:41:48Z

(c > 5) OR ((d < 6) AND (c > 5) -- can't remove

@alamb
I Have updated the unit tests, but
(c > 5) OR ((d < 6) AND (c > 5)) should be simplified to (c > 5), isn't it?

Maybe you mean a different expression ?

Dandandan · 2021-05-29T07:48:07Z

+
+fn simplify<'a>(expr: &'a Expr) -> Expr {
+ match expr {
+ Expr::BinaryExpr { left, op: _, right } if left == right => simplify(left),


This seems not correct to me, it seems to "simplify" a + a to a too?

I am seeing one other example from the test 1=1 is also a good example, it now is rewritten to 1

You're right, I was wrongly assuming that all expressions are boolean so ... my mistake.
I have updated it. I hope I can check your other comments soon. Thanks !

Dandandan · 2021-05-29T08:12:36Z

Looks like a good start!

I think it's good to think of these rules as simple rewrite rules we can apply to the expression tree.

p OR p => p
p AND p => p

Some weeks ago I implemented some rules in this custom DataFusion optimizer (based on graph rewriting), you can see them here:

https://github.com/Dandandan/datafusion-tokomak/blob/main/src/lib.rs#L44
There are a lot of other patterns we can add if we do term rewriting in this way.

Also, I think we should make this optimizer run on anyExpr, not just those in Filter as this would benefit any Expr (in projections, aggregations, joins, etc)

alamb · 2021-05-29T10:52:28Z

(c > 5) OR ((d < 6) AND (c > 5)) should be simplified to (c > 5), isn't it?

Yes you are correct -- I guess I was quickly trying to cook up examples similar to @Dandandan that showed searching for an arbitrary instance of the same expression (even if it appeared in some nested structure) was going to incorrectly remove the nested instance in some cases

jgoday · 2021-05-31T20:04:20Z

@Dandandan I've just added some more simplification rules (from https://github.com/Dandandan/datafusion-tokomak/blob/main/src/lib.rs#L44, as you mentioned before), What do you think ?

Still have to test and add more LogicalPlan cases (Projection, Window, Aggregation, ...) to optimize function.

Dandandan · 2021-05-31T20:27:50Z

+/// # Introduction
+/// It uses boolean algebra laws to simplify or reduce the number of terms in expressions.
+///
+/// Filter: #b Gt Int32(2) And #b Gt Int32(2)


I think we can best use b > 2 AND b > 2 here in the docs.
Hopefully we can implement a better way of displaying expressions soon!

Tracked in #347 for anyone following along :)

Changed, but I'm afraid this file docs will have to be fixed/completed later ...

Dandandan · 2021-05-31T20:29:14Z

+/// Filter: #b Gt Int32(2)
+pub struct RemoveDuplicateFilters {}
+
+fn expr_contains<'a>(expr: &'a Expr, needle: &'a Expr) -> bool {


any reason to use lifetime specifiers everywhere - we can't use just use &Expr in here?

My mistake, should be fixed now.

Dandandan · 2021-05-31T20:29:53Z

+}
+
+fn is_one<'a>(s: &'a Expr) -> bool {
+ match s {


These are really good use cases for the matches! macro https://doc.rust-lang.org/std/macro.matches.html

Ok, nice macro (I did not know it).
Sorry if i'm being too clumsy, but how can I use it if I have to guard two different cases (each one with a different variable/type) ?
matches!(s, Scalar::Int8(1) | Scalar::Int16(1) | Scalar::Float32(Some(v)) if *v == 1. | Scalar::Float64(Some(v)) if *v == 1. )
Seems that maches! only allows one last guard, and the existing binding must have the same type in all alternatives.
Is there any other way ?

A matches! returns a boolean so you can also use || / boolean or to cover the float cases and keep the rest in a single matches!

Or it might be better to just use | patterns (without matches!) in the code.

I like that last option :)

Dandandan · 2021-05-31T20:30:33Z

+ }
+}
+
+fn operator_is_boolean(op: &Operator) -> bool {


Operator is probably Copy so no need to add &

You're right! should be fixed in latest commit

Dandandan · 2021-05-31T20:33:20Z

+ .map(|input| optimize(input))
+ .collect::<Result<Vec<_>>>()?;
+
+ let expr = plan.expressions();


This could use something like plan.expressions().into_iter().map(|x| simplify(x)).collect::<Vec<_>>() ? and the other patterns could be removed here? (So it also applies for any node containing expressions.

Should this be valid ?
fn optimize(plan: &LogicalPlan) -> Result<LogicalPlan> { let new_inputs = plan .inputs() .iter() .map(|input| optimize(input)) .collect::<Result<Vec<_>>>()?; let expr = plan .expressions() .into_iter() .map(|x| simplify(&x)) .collect::<Vec<_>>(); utils::from_plan(&plan, &expr, &new_inputs) }

That looks right to me!

Dandandan · 2021-05-31T20:34:44Z

 pub mod limit_push_down;
 pub mod optimizer;
 pub mod projection_push_down;
+pub mod remove_duplicate_filters;


Probably we should rename it now to something like "simplify expressions".

Should we rename it to 'optimizer::simplify_expressions' ?

I think that would be a more accurate name by now 👍

Ok, changed to simplify_expressions.

Dandandan · 2021-05-31T21:03:29Z

+ left,
+ op: Operator::Minus,
+ right
+ } if left == right => lit(0),


I removed those by now from this PR #441 as I think they might be wrong in precense of nulls (null - null is not 0 ?) null * 0 is not 0

I agree, already removed.
maybe there should be some specific optimizations for arithmetic operators ...

It might be possible to do the optimization if we know the input is non null based on the schema. But probably something to find out later 👍

codecov-commenter · 2021-06-01T20:47:48Z

Codecov Report

Merging #436 (af2c6ac) into master (db4f098) will increase coverage by 0.98%.
The diff coverage is 87.45%.

@@ Coverage Diff @@## master #436 +/- ##
==========================================
+ Coverage 74.94% 75.92% +0.98% 
==========================================
Files 146 154 +8 Lines 24314 26195 +1881 ==========================================
+ Hits 18221 19889 +1668 - Misses 6093 6306 +213

Impacted Files	Coverage Δ
datafusion/src/execution/context.rs	`92.08% <ø> (+0.03%)`	⬆️
datafusion/src/optimizer/simplify_expressions.rs	`87.45% <87.45%> (ø)`
datafusion-cli/src/print_format.rs	`81.25% <0.00%> (-9.17%)`	⬇️
datafusion/src/physical_plan/mod.rs	`78.70% <0.00%> (-4.06%)`	⬇️
datafusion/src/physical_plan/window_functions.rs	`85.71% <0.00%> (-3.01%)`	⬇️
...tafusion/src/physical_plan/datetime_expressions.rs	`67.29% <0.00%> (-2.52%)`	⬇️
datafusion/src/physical_plan/common.rs	`84.21% <0.00%> (-2.00%)`	⬇️
datafusion/src/optimizer/hash_build_probe_order.rs	`58.53% <0.00%> (-1.97%)`	⬇️
datafusion/src/physical_plan/repartition.rs	`82.45% <0.00%> (-1.89%)`	⬇️
ballista/rust/scheduler/src/planner.rs	`66.91% <0.00%> (-0.74%)`	⬇️
... and 57 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update db4f098...af2c6ac. Read the comment docs.

Dandandan · 2021-06-02T06:32:18Z

+ left,
+ op: Operator::Or,
+ right,
+ } if expr_contains(left, right) => as_binary_expr(left)


It seems if this matches any oppurtunity to go deeper is lost? Is this pattern still needed?

I think that is needed for expressions like '((c > 5) AND (d < 6)) AND (c > 5)'.
Nested simplification should be fixed by latest commit.

Ah it makes sense, I missed that expr_contains pattern matches on or/and 👍

Dandandan · 2021-06-02T06:33:42Z

+ left,
+ op: Operator::And,
+ right,
+ } if is_false(left) || is_false(right) => lit(false),


This is also not correct I think if a null is present

nvm this is actually correct 👍

Dandandan · 2021-06-02T06:37:22Z

+ left,
+ op: Operator::And,
+ right,
+ } if expr_contains(left, right) => as_binary_expr(left)


Also here - is this still needed and doesn't this remove certain opportunities as the recursive call isn't done?

Dandandan · 2021-06-02T06:39:20Z

+ left,
+ op: Operator::Divide,
+ right,
+ } if left == right => lit(1),


This isn't correct if both sides can be null.

You're right, should be fixed now.

I merged it, but see this part might still be not 100% correct.
if left and right are a column, the value can still be null (for some evaluations).
Also the division by 0 (e.g. 0/0 or x/x where x is 0) is tricky.
I think there is no easy way to do it safely.

Dandandan

LGTM

Dandandan · 2021-06-02T11:09:24Z

It looks good to me @jgoday
Could you fix the clippy and formatting errors?

jgoday · 2021-06-02T11:33:20Z

It looks good to me @jgoday
Could you fix the clippy and formatting errors?

Should be fixed now 👍

alamb

Thank you @jgoday -- this looks like a good step forward. 👍

As I mentioned I think this logic could be put into the constant folding pass but we can always do that as a follow on

alamb · 2021-06-02T22:26:29Z

+/// Filter: b > 2 AND b > 2
+/// is optimized to
+/// Filter: b > 2
+pub struct SimplifyExpressions {}


I can't help but notice the similarity between this pass and ConstantFolding: https://github.com/apache/arrow-datafusion/blob/master/datafusion/src/optimizer/constant_folding.rs#L130
Perhaps as a follow on PR, we could combine the code into a single pass

alamb · 2021-06-02T22:29:34Z

+
+ #[test]
+ fn test_simplify_divide_by_same() -> Result<()> {
+ let expr = binary_expr(col("c"), Operator::Divide, col("c"));


I wonder if we care that in some cases (where c has a 0) this simplification will avoid a runtime error when it would have generated one without the optimization pass
I personally think it is ok, but wanted to mention it

Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>

Dandandan · 2021-06-03T15:54:08Z

Awesome thanks @jgoday 💯

alamb · 2021-06-03T16:18:52Z

Thanks again @jgoday 👍

jgoday · 2021-06-03T17:48:17Z

Thank you @Dandandan and @alamb for all your help!

* test: Fix explain with exteded info comet test * address review comments * address review comments

apache#410: Remove reundant filters (e.g. c> 5 AND c>5 --> c>5)
37423dc

jgoday mentioned this pull request May 28, 2021
Remove reundant filters (e.g. c> 5 AND c>5 --> c>5) #410
Closed

jgoday added 2 commits May 28, 2021 22:33

RemoveDuplicateFilters: fix unit tests
5cb888a

fix lint
3c37a55

Dandandan reviewed May 29, 2021
View reviewed changes

jgoday added 2 commits May 29, 2021 15:54

fix erroneous simplifications of arithmetic expressions
e64c69f

RemoveDuplicateFilters: add more simplification rules (@Dandandan)
9fbeb96

Dandandan reviewed May 31, 2021
View reviewed changes

jgoday added 3 commits June 1, 2021 20:57

Remove unnecessary lifetime specifiers and other fixes ...
4ade2ba

RemoveDuplicateFilter: fix is_one matches
55fb021

Change remove_duplicate_filter to simplify_expressions
25adf53

Dandandan reviewed Jun 2, 2021
View reviewed changes

fix simplify expressions
2e777a8

Dandandan approved these changes Jun 2, 2021
View reviewed changes

fix lint and clippy
c79899b

alamb approved these changes Jun 2, 2021
View reviewed changes

jgodayand others added 5 commits June 3, 2021 07:34

Update datafusion/src/optimizer/simplify_expressions.rs
f161d6f
Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>

Update datafusion/src/optimizer/simplify_expressions.rs
71b95a2
Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>

Update datafusion/src/optimizer/simplify_expressions.rs
618f34c
Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>

Update datafusion/src/optimizer/simplify_expressions.rs
538ccdb
Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>

simplify test expressions
af2c6ac

Dandandan merged commit 4392eea into apache:masterJun 3, 2021

houqp added datafusion enhancement New feature or request labels Jul 30, 2021

unkloud pushed a commit to unkloud/datafusion that referenced this pull request Mar 23, 2025
test: Fix explain with exteded info comet test (apache#436)
7fe20b1
* test: Fix explain with exteded info comet test * address review comments * address review comments

Uh oh!

Conversation

jgoday commented May 28, 2021

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

Uh oh!

jgoday commented May 28, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DandandanMay 29, 2021• 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!

jgodayMay 29, 2021• 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!

Dandandan commented May 29, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alamb commented May 29, 2021

Uh oh!

jgoday commented May 31, 2021

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!

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!

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!

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!

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!

DandandanMay 31, 2021• 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!

codecov-commenter commented Jun 1, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

DandandanMay 29, 2021•
edited
Loading

jgodayMay 29, 2021•
edited
Loading

Dandandan commented May 29, 2021•
edited
Loading

DandandanMay 31, 2021•
edited
Loading

codecov-commenter commented Jun 1, 2021•
edited
Loading

DandandanJun 2, 2021•
edited
Loading