Remove reundant filters (e.g. c> 5 AND c>5 --> c>5) #436

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

datafusion/src/optimizer/remove_duplicate_filters.rs

+
+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 any Expr, 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

datafusion/src/optimizer/remove_duplicate_filters.rs

+/// # 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

datafusion/src/optimizer/remove_duplicate_filters.rs

+/// 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

datafusion/src/optimizer/remove_duplicate_filters.rs

+}
+
+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

datafusion/src/optimizer/remove_duplicate_filters.rs

+    }
+}
+
+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

datafusion/src/optimizer/remove_duplicate_filters.rs

+                .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

datafusion/src/optimizer/mod.rs

@@ -25,4 +25,5 @@ pub mod hash_build_probe_order;
 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

datafusion/src/optimizer/remove_duplicate_filters.rs

+            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

datafusion/src/optimizer/simplify_expressions.rs

+            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

datafusion/src/optimizer/simplify_expressions.rs

+            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

datafusion/src/optimizer/simplify_expressions.rs

+            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

datafusion/src/optimizer/simplify_expressions.rs

+            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

datafusion/src/optimizer/simplify_expressions.rs

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

datafusion/src/optimizer/simplify_expressions.rs

+/// 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

datafusion/src/optimizer/simplify_expressions.rs

+
+    #[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

datafusion/src/optimizer/simplify_expressions.rs

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!

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

jgoday and 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:master Jun 3, 2021

houqp added datafusion Changes in the datafusion crate enhancement New feature or request labels Jul 30, 2021

Remove reundant filters (e.g. c> 5 AND c>5 --> c>5) #436

Remove reundant filters (e.g. c> 5 AND c>5 --> c>5) #436

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?

jgoday commented May 28, 2021

Choose a reason for hiding this comment

Dandandan May 29, 2021 • edited Loading

Choose a reason for hiding this comment

jgoday May 29, 2021 • edited Loading

Choose a reason for hiding this comment

Dandandan commented May 29, 2021 • edited Loading

alamb commented May 29, 2021

jgoday commented May 31, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dandandan May 31, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Jun 1, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dandandan Jun 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dandandan left a comment

Choose a reason for hiding this comment

Dandandan commented Jun 2, 2021

jgoday commented Jun 2, 2021

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dandandan commented Jun 3, 2021

alamb commented Jun 3, 2021

jgoday commented Jun 3, 2021

Dandandan May 29, 2021 •

edited

Loading

jgoday May 29, 2021 •

edited

Loading

Dandandan commented May 29, 2021 •

edited

Loading

Dandandan May 31, 2021 •

edited

Loading

codecov-commenter commented Jun 1, 2021 •

edited

Loading

Dandandan Jun 2, 2021 •

edited

Loading