You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "eejbyfeldt (via GitHub)" <gi...@apache.org> on 2024/10/21 12:14:06 UTC
[PR] refactor: Incorporate RewriteDisjunctivePredicate rule into SimplifyExpressions [datafusion]
eejbyfeldt opened a new pull request, #13032:
URL: https://github.com/apache/datafusion/pull/13032
## Which issue does this PR close?
Closes #13031
## Rationale for this change
The rational for moving the logic in to the SimplifyExpression is that is allows the rule to better interact with other simplifications and also be applied to all logical nodes, not just filters as the current rule is.
The reason for changing the logic is to only only rewrite the Expr when the rule acutally applies. The current code will always change the current structure of the expression which makes our optimizer harded to predict and more sensitive to changes in the order the rules are applied.
The new code is also significantly shorter and at least to me easier to follow.
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
## What changes are included in this PR?
Adding a rule in SimplifyExpressions that will extract common conjuntive predicates in disjuntive predicates and removal of the existing rule RewriteDisjuntivePredicate.
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
## Are these changes tested?
Yes, existing and new tests.
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
2. Serve as another way to document the expected behavior of the code
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
-->
## Are there any user-facing changes?
Yes, the rule RewriteDisjuntivePredicate no longer exists.
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!--
If there are any breaking changes to public APIs, please add the `api change` label.
-->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] refactor: Incorporate RewriteDisjunctivePredicate rule into SimplifyExpressions [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #13032:
URL: https://github.com/apache/datafusion/pull/13032#discussion_r1809521444
##########
datafusion/sqllogictest/test_files/cse.slt:
##########
@@ -199,18 +199,18 @@ physical_plan
# Surely only once but also conditionally evaluated subexpressions
query TT
EXPLAIN SELECT
- (a = 1 OR random() = 0) AND (a = 1 OR random() = 1) AS c1,
- (a = 2 AND random() = 0) OR (a = 2 AND random() = 1) AS c2,
+ (a = 1 OR random() = 0) AND (a = 2 OR random() = 1) AS c1,
Review Comment:
Why this change?
I ran the test locally for the original and here it is
```sql
query TT
EXPLAIN SELECT
(a = 1 OR random() = 0) AND (a = 1 OR random() = 1) AS c1,
(a = 2 AND random() = 0) OR (a = 2 AND random() = 1) AS c2,
CASE WHEN a + 3 = 0 THEN a + 3 + random() ELSE 0 END AS c3,
CASE WHEN a + 4 = 0 THEN 0 ELSE a + 4 + random() END AS c4
FROM t1
----
logical_plan
01)Projection: (__common_expr_1 OR random() = Float64(0)) AND (__common_expr_1 OR random() = Float64(1)) AS c1, t1.a = Float64(2) AND (random() = Float64(0) OR random() = Float64(1)) AS c2, CASE WHEN __common_expr_2 = Float64(0) THEN __common_expr_2 + random() ELSE Float64(0) END AS c3, CASE WHEN __common_expr_3 = Float64(0) THEN Float64(0) ELSE __common_expr_3 + random() END AS c4
02)--Projection: t1.a = Float64(1) AS __common_expr_1, t1.a + Float64(3) AS __common_expr_2, t1.a + Float64(4) AS __common_expr_3, t1.a
03)----TableScan: t1 projection=[a]
physical_plan
01)ProjectionExec: expr=[(__common_expr_1@0 OR random() = 0) AND (__common_expr_1@0 OR random() = 1) as c1, a@3 = 2 AND (random() = 0 OR random() = 1) as c2, CASE WHEN __common_expr_2@1 = 0 THEN __common_expr_2@1 + random() ELSE 0 END as c3, CASE WHEN __common_expr_3@2 = 0 THEN 0 ELSE __common_expr_3@2 + random() END as c4]
02)--ProjectionExec: expr=[a@0 = 1 as __common_expr_1, a@0 + 3 as __common_expr_2, a@0 + 4 as __common_expr_3, a@0 as a]
03)----MemoryExec: partitions=1, partition_sizes=[0]
```
@peter-toth does this look right to you?
##########
datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs:
##########
@@ -850,6 +854,27 @@ impl<'a, S: SimplifyInfo> TreeNodeRewriter for Simplifier<'a, S> {
op: Or,
right,
}) if is_op_with(And, &left, &right) => Transformed::yes(*right),
+ // Elliminate common factors in conjunctions e.g
Review Comment:
```suggestion
// Eliminate common factors in conjunctions e.g
```
##########
datafusion/optimizer/src/optimizer.rs:
##########
@@ -255,7 +254,6 @@ impl Optimizer {
// run it again after running the optimizations that potentially converted
// subqueries to joins
Arc::new(SimplifyExpressions::new()),
- Arc::new(RewriteDisjunctivePredicate::new()),
Review Comment:
🥳 -- thank you @eejbyfeldt
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] refactor: Incorporate RewriteDisjunctivePredicate rule into SimplifyExpressions [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #13032:
URL: https://github.com/apache/datafusion/pull/13032#discussion_r1809499606
##########
datafusion/expr/src/utils.rs:
##########
@@ -1101,6 +1101,54 @@ fn split_conjunction_impl<'a>(expr: &'a Expr, mut exprs: Vec<&'a Expr>) -> Vec<&
}
}
+/// Iteratate parts in a conjunctive [`Expr`] such as `A AND B AND C` => `[A, B, C]`
+///
+/// See [`split_conjunction_owned`] for more details and an example.
+pub fn iter_conjunction(expr: &Expr) -> impl Iterator<Item = &Expr> {
+ let mut stack = vec![expr];
+ std::iter::from_fn(move || {
+ while let Some(expr) = stack.pop() {
+ match expr {
+ Expr::BinaryExpr(BinaryExpr {
+ right,
+ op: Operator::And,
+ left,
+ }) => {
+ stack.push(right);
+ stack.push(left);
+ }
+ Expr::Alias(Alias { expr, .. }) => stack.push(expr),
+ other => return Some(other),
+ }
+ }
+ None
+ })
+}
+
+/// Iteratate parts in a conjunctive [`Expr`] such as `A AND B AND C` => `[A, B, C]`
+///
+/// See [`split_conjunction_owned`] for more details and an example.
+pub fn iter_conjunction_owned(expr: Expr) -> impl Iterator<Item = Expr> {
+ let mut stack = vec![expr];
+ std::iter::from_fn(move || {
+ while let Some(expr) = stack.pop() {
Review Comment:
this is a clever formulation of an iterator
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] refactor: Incorporate RewriteDisjunctivePredicate rule into SimplifyExpressions [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #13032:
URL: https://github.com/apache/datafusion/pull/13032#discussion_r1809499606
##########
datafusion/expr/src/utils.rs:
##########
@@ -1101,6 +1101,54 @@ fn split_conjunction_impl<'a>(expr: &'a Expr, mut exprs: Vec<&'a Expr>) -> Vec<&
}
}
+/// Iteratate parts in a conjunctive [`Expr`] such as `A AND B AND C` => `[A, B, C]`
+///
+/// See [`split_conjunction_owned`] for more details and an example.
+pub fn iter_conjunction(expr: &Expr) -> impl Iterator<Item = &Expr> {
+ let mut stack = vec![expr];
+ std::iter::from_fn(move || {
+ while let Some(expr) = stack.pop() {
+ match expr {
+ Expr::BinaryExpr(BinaryExpr {
+ right,
+ op: Operator::And,
+ left,
+ }) => {
+ stack.push(right);
+ stack.push(left);
+ }
+ Expr::Alias(Alias { expr, .. }) => stack.push(expr),
+ other => return Some(other),
+ }
+ }
+ None
+ })
+}
+
+/// Iteratate parts in a conjunctive [`Expr`] such as `A AND B AND C` => `[A, B, C]`
+///
+/// See [`split_conjunction_owned`] for more details and an example.
+pub fn iter_conjunction_owned(expr: Expr) -> impl Iterator<Item = Expr> {
+ let mut stack = vec![expr];
+ std::iter::from_fn(move || {
+ while let Some(expr) = stack.pop() {
Review Comment:
this is a clever formulation of an iterator that does not require buffering all the conjuncts in a `Vec`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] refactor: Incorporate RewriteDisjunctivePredicate rule into SimplifyExpressions [datafusion]
Posted by "Dandandan (via GitHub)" <gi...@apache.org>.
Dandandan commented on code in PR #13032:
URL: https://github.com/apache/datafusion/pull/13032#discussion_r1808853767
##########
datafusion/sqllogictest/test_files/cse.slt:
##########
@@ -199,19 +199,19 @@ physical_plan
# Surely only once but also conditionally evaluated subexpressions
query TT
EXPLAIN SELECT
- (a = 1 OR random() = 0) AND (a = 1 OR random() = 1) AS c1,
- (a = 2 AND random() = 0) OR (a = 2 AND random() = 1) AS c2,
+ (a = 1 OR random() = 0) AND (a = 2 OR random() = 1) AS c1,
+ (a = 1 AND random() = 0) OR (a = 2 AND random() = 1) AS c2,
CASE WHEN a + 3 = 0 THEN a + 3 + random() ELSE 0 END AS c3,
CASE WHEN a + 4 = 0 THEN 0 ELSE a + 4 + random() END AS c4
FROM t1
----
logical_plan
-01)Projection: (__common_expr_1 OR random() = Float64(0)) AND (__common_expr_1 OR random() = Float64(1)) AS c1, __common_expr_2 AND random() = Float64(0) OR __common_expr_2 AND random() = Float64(1) AS c2, CASE WHEN __common_expr_3 = Float64(0) THEN __common_expr_3 + random() ELSE Float64(0) END AS c3, CASE WHEN __common_expr_4 = Float64(0) THEN Float64(0) ELSE __common_expr_4 + random() END AS c4
-02)--Projection: t1.a = Float64(1) AS __common_expr_1, t1.a = Float64(2) AS __common_expr_2, t1.a + Float64(3) AS __common_expr_3, t1.a + Float64(4) AS __common_expr_4
+01)Projection: (__common_expr_1 OR random() = Float64(0)) AND (t1.a = Float64(2) OR random() = Float64(1)) AS c1, __common_expr_1 AND random() = Float64(0) OR t1.a = Float64(2) AND random() = Float64(1) AS c2, CASE WHEN __common_expr_2 = Float64(0) THEN __common_expr_2 + random() ELSE Float64(0) END AS c3, CASE WHEN __common_expr_3 = Float64(0) THEN Float64(0) ELSE __common_expr_3 + random() END AS c4
+02)--Projection: t1.a = Float64(1) AS __common_expr_1, t1.a + Float64(3) AS __common_expr_2, t1.a + Float64(4) AS __common_expr_3, t1.a
03)----TableScan: t1 projection=[a]
physical_plan
-01)ProjectionExec: expr=[(__common_expr_1@0 OR random() = 0) AND (__common_expr_1@0 OR random() = 1) as c1, __common_expr_2@1 AND random() = 0 OR __common_expr_2@1 AND random() = 1 as c2, CASE WHEN __common_expr_3@2 = 0 THEN __common_expr_3@2 + random() ELSE 0 END as c3, CASE WHEN __common_expr_4@3 = 0 THEN 0 ELSE __common_expr_4@3 + random() END as c4]
-02)--ProjectionExec: expr=[a@0 = 1 as __common_expr_1, a@0 = 2 as __common_expr_2, a@0 + 3 as __common_expr_3, a@0 + 4 as __common_expr_4]
+01)ProjectionExec: expr=[(__common_expr_1@0 OR random() = 0) AND (a@3 = 2 OR random() = 1) as c1, __common_expr_1@0 AND random() = 0 OR a@3 = 2 AND random() = 1 as c2, CASE WHEN __common_expr_2@1 = 0 THEN __common_expr_2@1 + random() ELSE 0 END as c3, CASE WHEN __common_expr_3@2 = 0 THEN 0 ELSE __common_expr_3@2 + random() END as c4]
+02)--ProjectionExec: expr=[a@0 = 1 as __common_expr_1, a@0 + 3 as __common_expr_2, a@0 + 4 as __common_expr_3, a@0 as a]
Review Comment:
(Same as above, but in physical plan).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] refactor: Incorporate RewriteDisjunctivePredicate rule into SimplifyExpressions [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #13032:
URL: https://github.com/apache/datafusion/pull/13032#issuecomment-2427701171
(thank you @eejbyfeldt and @Dandandan )
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] refactor: Incorporate RewriteDisjunctivePredicate rule into SimplifyExpressions [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #13032:
URL: https://github.com/apache/datafusion/pull/13032
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] refactor: Incorporate RewriteDisjunctivePredicate rule into SimplifyExpressions [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #13032:
URL: https://github.com/apache/datafusion/pull/13032#discussion_r1809499606
##########
datafusion/expr/src/utils.rs:
##########
@@ -1101,6 +1101,54 @@ fn split_conjunction_impl<'a>(expr: &'a Expr, mut exprs: Vec<&'a Expr>) -> Vec<&
}
}
+/// Iteratate parts in a conjunctive [`Expr`] such as `A AND B AND C` => `[A, B, C]`
+///
+/// See [`split_conjunction_owned`] for more details and an example.
+pub fn iter_conjunction(expr: &Expr) -> impl Iterator<Item = &Expr> {
+ let mut stack = vec![expr];
+ std::iter::from_fn(move || {
+ while let Some(expr) = stack.pop() {
+ match expr {
+ Expr::BinaryExpr(BinaryExpr {
+ right,
+ op: Operator::And,
+ left,
+ }) => {
+ stack.push(right);
+ stack.push(left);
+ }
+ Expr::Alias(Alias { expr, .. }) => stack.push(expr),
+ other => return Some(other),
+ }
+ }
+ None
+ })
+}
+
+/// Iteratate parts in a conjunctive [`Expr`] such as `A AND B AND C` => `[A, B, C]`
+///
+/// See [`split_conjunction_owned`] for more details and an example.
+pub fn iter_conjunction_owned(expr: Expr) -> impl Iterator<Item = Expr> {
+ let mut stack = vec![expr];
+ std::iter::from_fn(move || {
+ while let Some(expr) = stack.pop() {
Review Comment:
this is a clever formulation of an iterator that does not require buffering a large number of them
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] refactor: Incorporate RewriteDisjunctivePredicate rule into SimplifyExpressions [datafusion]
Posted by "eejbyfeldt (via GitHub)" <gi...@apache.org>.
eejbyfeldt commented on code in PR #13032:
URL: https://github.com/apache/datafusion/pull/13032#discussion_r1810640566
##########
datafusion/sqllogictest/test_files/cse.slt:
##########
@@ -199,18 +199,18 @@ physical_plan
# Surely only once but also conditionally evaluated subexpressions
query TT
EXPLAIN SELECT
- (a = 1 OR random() = 0) AND (a = 1 OR random() = 1) AS c1,
- (a = 2 AND random() = 0) OR (a = 2 AND random() = 1) AS c2,
+ (a = 1 OR random() = 0) AND (a = 2 OR random() = 1) AS c1,
Review Comment:
> (Actually the same trick is applied in the previous test called # Surely only once but also conditionally evaluated expressions a bit above.)
That was also changed by me in https://github.com/apache/datafusion/pull/12746 for basically the same reason for another rule.
> Out of curiosity, volatile common expressions don't get simplified, do they?
> E.g. `(a = 1 AND random() = 0) OR (a = 2 AND random() = 0)` is not rewritten to `random() = 0 AND (a = 1 OR a = 2)`?
It would end up being rewritten. I agree that does not sound correct. It should probably be addressed separatly as the issue applies to many of the other simplification rules. Created https://github.com/apache/datafusion/issues/13060
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] refactor: Incorporate RewriteDisjunctivePredicate rule into SimplifyExpressions [datafusion]
Posted by "peter-toth (via GitHub)" <gi...@apache.org>.
peter-toth commented on code in PR #13032:
URL: https://github.com/apache/datafusion/pull/13032#discussion_r1810351750
##########
datafusion/sqllogictest/test_files/cse.slt:
##########
@@ -199,18 +199,18 @@ physical_plan
# Surely only once but also conditionally evaluated subexpressions
query TT
EXPLAIN SELECT
- (a = 1 OR random() = 0) AND (a = 1 OR random() = 1) AS c1,
- (a = 2 AND random() = 0) OR (a = 2 AND random() = 1) AS c2,
+ (a = 1 OR random() = 0) AND (a = 2 OR random() = 1) AS c1,
Review Comment:
Yeah, this change looks good to me to avoid simplification in this CSE test.
(Actually the same trick is applied in the previous test called `# Surely only once but also conditionally evaluated expressions` a bit above.)
Out of curiosity, volatile common expressions don't get eliminated, do they?
E.g. `(a = 1 AND random() = 0) OR (a = 2 AND random() = 0)` is not rewritten to `random() = 0 AND (a = 1 OR a = 2)`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] refactor: Incorporate RewriteDisjunctivePredicate rule into SimplifyExpressions [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #13032:
URL: https://github.com/apache/datafusion/pull/13032#issuecomment-2429598135
🚀 -- thank you @eejbyfeldt @Dandandan and @peter-toth
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] refactor: Incorporate RewriteDisjunctivePredicate rule into SimplifyExpressions [datafusion]
Posted by "peter-toth (via GitHub)" <gi...@apache.org>.
peter-toth commented on code in PR #13032:
URL: https://github.com/apache/datafusion/pull/13032#discussion_r1810351750
##########
datafusion/sqllogictest/test_files/cse.slt:
##########
@@ -199,18 +199,18 @@ physical_plan
# Surely only once but also conditionally evaluated subexpressions
query TT
EXPLAIN SELECT
- (a = 1 OR random() = 0) AND (a = 1 OR random() = 1) AS c1,
- (a = 2 AND random() = 0) OR (a = 2 AND random() = 1) AS c2,
+ (a = 1 OR random() = 0) AND (a = 2 OR random() = 1) AS c1,
Review Comment:
Yeah, this change looks good to me to avoid simplification in this CSE test.
(Actually the same trick is applied in the previous test called `# Surely only once but also conditionally evaluated expressions` a bit above.)
Out of curiosity, volatile common expressions don't get simplified, do they?
E.g. `(a = 1 AND random() = 0) OR (a = 2 AND random() = 0)` is not rewritten to `random() = 0 AND (a = 1 OR a = 2)`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] refactor: Incorporate RewriteDisjunctivePredicate rule into SimplifyExpressions [datafusion]
Posted by "Dandandan (via GitHub)" <gi...@apache.org>.
Dandandan commented on code in PR #13032:
URL: https://github.com/apache/datafusion/pull/13032#discussion_r1808852820
##########
datafusion/sqllogictest/test_files/cse.slt:
##########
@@ -199,19 +199,19 @@ physical_plan
# Surely only once but also conditionally evaluated subexpressions
query TT
EXPLAIN SELECT
- (a = 1 OR random() = 0) AND (a = 1 OR random() = 1) AS c1,
- (a = 2 AND random() = 0) OR (a = 2 AND random() = 1) AS c2,
+ (a = 1 OR random() = 0) AND (a = 2 OR random() = 1) AS c1,
+ (a = 1 AND random() = 0) OR (a = 2 AND random() = 1) AS c2,
CASE WHEN a + 3 = 0 THEN a + 3 + random() ELSE 0 END AS c3,
CASE WHEN a + 4 = 0 THEN 0 ELSE a + 4 + random() END AS c4
FROM t1
----
logical_plan
-01)Projection: (__common_expr_1 OR random() = Float64(0)) AND (__common_expr_1 OR random() = Float64(1)) AS c1, __common_expr_2 AND random() = Float64(0) OR __common_expr_2 AND random() = Float64(1) AS c2, CASE WHEN __common_expr_3 = Float64(0) THEN __common_expr_3 + random() ELSE Float64(0) END AS c3, CASE WHEN __common_expr_4 = Float64(0) THEN Float64(0) ELSE __common_expr_4 + random() END AS c4
-02)--Projection: t1.a = Float64(1) AS __common_expr_1, t1.a = Float64(2) AS __common_expr_2, t1.a + Float64(3) AS __common_expr_3, t1.a + Float64(4) AS __common_expr_4
+01)Projection: (__common_expr_1 OR random() = Float64(0)) AND (t1.a = Float64(2) OR random() = Float64(1)) AS c1, __common_expr_1 AND random() = Float64(0) OR t1.a = Float64(2) AND random() = Float64(1) AS c2, CASE WHEN __common_expr_2 = Float64(0) THEN __common_expr_2 + random() ELSE Float64(0) END AS c3, CASE WHEN __common_expr_3 = Float64(0) THEN Float64(0) ELSE __common_expr_3 + random() END AS c4
+02)--Projection: t1.a = Float64(1) AS __common_expr_1, t1.a + Float64(3) AS __common_expr_2, t1.a + Float64(4) AS __common_expr_3, t1.a
03)----TableScan: t1 projection=[a]
physical_plan
-01)ProjectionExec: expr=[(__common_expr_1@0 OR random() = 0) AND (__common_expr_1@0 OR random() = 1) as c1, __common_expr_2@1 AND random() = 0 OR __common_expr_2@1 AND random() = 1 as c2, CASE WHEN __common_expr_3@2 = 0 THEN __common_expr_3@2 + random() ELSE 0 END as c3, CASE WHEN __common_expr_4@3 = 0 THEN 0 ELSE __common_expr_4@3 + random() END as c4]
-02)--ProjectionExec: expr=[a@0 = 1 as __common_expr_1, a@0 = 2 as __common_expr_2, a@0 + 3 as __common_expr_3, a@0 + 4 as __common_expr_4]
+01)ProjectionExec: expr=[(__common_expr_1@0 OR random() = 0) AND (a@3 = 2 OR random() = 1) as c1, __common_expr_1@0 AND random() = 0 OR a@3 = 2 AND random() = 1 as c2, CASE WHEN __common_expr_2@1 = 0 THEN __common_expr_2@1 + random() ELSE 0 END as c3, CASE WHEN __common_expr_3@2 = 0 THEN 0 ELSE __common_expr_3@2 + random() END as c4]
+02)--ProjectionExec: expr=[a@0 = 1 as __common_expr_1, a@0 + 3 as __common_expr_2, a@0 + 4 as __common_expr_3, a@0 as a]
Review Comment:
Here it loses the `a@0= 2` CSE.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] refactor: Incorporate RewriteDisjunctivePredicate rule into SimplifyExpressions [datafusion]
Posted by "Dandandan (via GitHub)" <gi...@apache.org>.
Dandandan commented on code in PR #13032:
URL: https://github.com/apache/datafusion/pull/13032#discussion_r1808850281
##########
datafusion/sqllogictest/test_files/cse.slt:
##########
@@ -199,19 +199,19 @@ physical_plan
# Surely only once but also conditionally evaluated subexpressions
query TT
EXPLAIN SELECT
- (a = 1 OR random() = 0) AND (a = 1 OR random() = 1) AS c1,
- (a = 2 AND random() = 0) OR (a = 2 AND random() = 1) AS c2,
+ (a = 1 OR random() = 0) AND (a = 2 OR random() = 1) AS c1,
+ (a = 1 AND random() = 0) OR (a = 2 AND random() = 1) AS c2,
CASE WHEN a + 3 = 0 THEN a + 3 + random() ELSE 0 END AS c3,
CASE WHEN a + 4 = 0 THEN 0 ELSE a + 4 + random() END AS c4
FROM t1
----
logical_plan
-01)Projection: (__common_expr_1 OR random() = Float64(0)) AND (__common_expr_1 OR random() = Float64(1)) AS c1, __common_expr_2 AND random() = Float64(0) OR __common_expr_2 AND random() = Float64(1) AS c2, CASE WHEN __common_expr_3 = Float64(0) THEN __common_expr_3 + random() ELSE Float64(0) END AS c3, CASE WHEN __common_expr_4 = Float64(0) THEN Float64(0) ELSE __common_expr_4 + random() END AS c4
-02)--Projection: t1.a = Float64(1) AS __common_expr_1, t1.a = Float64(2) AS __common_expr_2, t1.a + Float64(3) AS __common_expr_3, t1.a + Float64(4) AS __common_expr_4
+01)Projection: (__common_expr_1 OR random() = Float64(0)) AND (t1.a = Float64(2) OR random() = Float64(1)) AS c1, __common_expr_1 AND random() = Float64(0) OR t1.a = Float64(2) AND random() = Float64(1) AS c2, CASE WHEN __common_expr_2 = Float64(0) THEN __common_expr_2 + random() ELSE Float64(0) END AS c3, CASE WHEN __common_expr_3 = Float64(0) THEN Float64(0) ELSE __common_expr_3 + random() END AS c4
+02)--Projection: t1.a = Float64(1) AS __common_expr_1, t1.a + Float64(3) AS __common_expr_2, t1.a + Float64(4) AS __common_expr_3, t1.a
Review Comment:
This seems to have lost the `t1.a = Float64(2)` common subexpression?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] refactor: Incorporate RewriteDisjunctivePredicate rule into SimplifyExpressions [datafusion]
Posted by "Dandandan (via GitHub)" <gi...@apache.org>.
Dandandan commented on PR #13032:
URL: https://github.com/apache/datafusion/pull/13032#issuecomment-2426729110
Looks like a nice refactor. The changed plan looks slightly less optimal (one expression repeated where it was executed only a single time before).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] refactor: Incorporate RewriteDisjunctivePredicate rule into SimplifyExpressions [datafusion]
Posted by "eejbyfeldt (via GitHub)" <gi...@apache.org>.
eejbyfeldt commented on code in PR #13032:
URL: https://github.com/apache/datafusion/pull/13032#discussion_r1809298618
##########
datafusion/sqllogictest/test_files/cse.slt:
##########
@@ -199,19 +199,19 @@ physical_plan
# Surely only once but also conditionally evaluated subexpressions
query TT
EXPLAIN SELECT
- (a = 1 OR random() = 0) AND (a = 1 OR random() = 1) AS c1,
- (a = 2 AND random() = 0) OR (a = 2 AND random() = 1) AS c2,
+ (a = 1 OR random() = 0) AND (a = 2 OR random() = 1) AS c1,
+ (a = 1 AND random() = 0) OR (a = 2 AND random() = 1) AS c2,
CASE WHEN a + 3 = 0 THEN a + 3 + random() ELSE 0 END AS c3,
CASE WHEN a + 4 = 0 THEN 0 ELSE a + 4 + random() END AS c4
FROM t1
----
logical_plan
-01)Projection: (__common_expr_1 OR random() = Float64(0)) AND (__common_expr_1 OR random() = Float64(1)) AS c1, __common_expr_2 AND random() = Float64(0) OR __common_expr_2 AND random() = Float64(1) AS c2, CASE WHEN __common_expr_3 = Float64(0) THEN __common_expr_3 + random() ELSE Float64(0) END AS c3, CASE WHEN __common_expr_4 = Float64(0) THEN Float64(0) ELSE __common_expr_4 + random() END AS c4
-02)--Projection: t1.a = Float64(1) AS __common_expr_1, t1.a = Float64(2) AS __common_expr_2, t1.a + Float64(3) AS __common_expr_3, t1.a + Float64(4) AS __common_expr_4
+01)Projection: (__common_expr_1 OR random() = Float64(0)) AND (t1.a = Float64(2) OR random() = Float64(1)) AS c1, __common_expr_1 AND random() = Float64(0) OR t1.a = Float64(2) AND random() = Float64(1) AS c2, CASE WHEN __common_expr_2 = Float64(0) THEN __common_expr_2 + random() ELSE Float64(0) END AS c3, CASE WHEN __common_expr_3 = Float64(0) THEN Float64(0) ELSE __common_expr_3 + random() END AS c4
+02)--Projection: t1.a = Float64(1) AS __common_expr_1, t1.a + Float64(3) AS __common_expr_2, t1.a + Float64(4) AS __common_expr_3, t1.a
Review Comment:
This was due to me modifying this query to avoid it being rewritten by this patch. But when I did it I was unaware about the logic around conditionals in CSE. I changed the modification slightly and we are no longer missing any cse.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org