You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2021/11/16 22:58:13 UTC
[GitHub] [arrow-datafusion] viirya opened a new pull request #1319: Avoid changing output column name before pushdown projection
viirya opened a new pull request #1319:
URL: https://github.com/apache/arrow-datafusion/pull/1319
# Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
Closes #1316.
# Rationale for this change
<!--
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.
-->
This patch fixes a bug happened when users construct an aggregate function which has constants that can be folded.
As in projection pushdown rule, we check if an expression output (e.g., an aggregate function) is required by above plan (e.g. a projection) by comparing expression's name. But after constant folding rule, it is likely that the name of an aggregate function is changed (e.g., from `COUNT(1 + 1)` to `COUNT(2)`). Changed aggregate function is removed as we wrongly think it as unnecessary expression as the top projection requires `#COUNT(1 + 1)`, not `#COUNT(2)`.
# What changes are included in this PR?
<!--
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.
-->
Moving `ConstantFolding` optimizer rules to the end.
# Are there any user-facing changes?
<!--
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.
-->
Should be not.
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] viirya commented on a change in pull request #1319: Avoid changing expression names during constant folding
Posted by GitBox <gi...@apache.org>.
viirya commented on a change in pull request #1319:
URL: https://github.com/apache/arrow-datafusion/pull/1319#discussion_r753610863
##########
File path: datafusion/src/optimizer/constant_folding.rs
##########
@@ -101,7 +105,30 @@ impl OptimizerRule for ConstantFolding {
// fold constants and then simplify
.rewrite(&mut const_evaluator)?
.rewrite(&mut simplifier)?;
- Ok(new_e)
+
+ let new_name = &new_e.name(plan.schema());
+
+ // Some plans will be candidates in projection pushdown rule to
+ // trim expressions based on expression names. We need to keep
+ // expression name for them.
+ let is_plan_for_projection_pushdown = matches!(
+ plan,
+ LogicalPlan::Window { .. }
Review comment:
Oh, I'm simply unsure if such changes are okay here as it looks like most queries will be affected (not about its results but the cosmetic one).
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] viirya commented on pull request #1319: Avoid changing expression names during constant folding
Posted by GitBox <gi...@apache.org>.
viirya commented on pull request #1319:
URL: https://github.com/apache/arrow-datafusion/pull/1319#issuecomment-974502648
Note that for the current approach that compares expression names, it works. The only issue is if we apply to all nodes there are many tests needed to be updated because many aliases are to be added there. E.g.,
1. optimizer::constant_folding::tests::optimize_plan_support_projection
```
left: `"Projection: #test.a, #test.d, NOT #test.b AS test.b = Boolean(false)\n TableScan: test projection=None"`,
right: `"Projection: #test.a, #test.d, NOT #test.b\n TableScan: test projection=None"`
```
2. optimizer::constant_folding::tests::optimize_plan_and_expr
```
left: `"Projection: #test.a\n Filter: NOT #test.b AND #test.c AS test.b != Boolean(true) AND test.c = Boolean(true)\n TableScan: test projection=None"`,
right: `"Projection: #test.a\n Filter: NOT #test.b AND #test.c\n TableScan: test projection=None"`
```
3. optimizer::constant_folding::tests::optimize_plan_not_expr
```
left: `"Projection: #test.a\n Filter: #test.b AS NOT test.b = Boolean(false)\n TableScan: test projection=None"`,
right: `"Projection: #test.a\n Filter: #test.b\n TableScan: test projection=None"`'
```
That's why I limit to certain nodes that I think mostly we like to add aliases to deal with projection push down rule.
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on pull request #1319: Avoid changing output column name before pushdown projection
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #1319:
URL: https://github.com/apache/arrow-datafusion/pull/1319#issuecomment-971663467
> Hmm, there is a test ctx_sql_should_optimize_plan that assumes the logical plan after sql is optimized one. I thought it doesn't make sense as the logical plan is not a final one, e.g. you could do select etc. operations to produce new DataFrame (i.e., new logical plan). We only need to optimize it before creating physical plan (create_physical_plan).
I wonder if another possible bug fix might be to change the `ConstantEvaluation` code to add an alias for aggregates? For example, rewrite aggregates so they explicitly keep the same (original) display name
Like for example, rewrite `COUNT(1+1)` to `COUNT(2) as "COUNT(1+1)"`
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on a change in pull request #1319: Avoid changing expression names during constant folding
Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #1319:
URL: https://github.com/apache/arrow-datafusion/pull/1319#discussion_r752660357
##########
File path: datafusion/src/optimizer/constant_folding.rs
##########
@@ -101,7 +105,30 @@ impl OptimizerRule for ConstantFolding {
// fold constants and then simplify
.rewrite(&mut const_evaluator)?
.rewrite(&mut simplifier)?;
- Ok(new_e)
+
+ let new_name = &new_e.name(plan.schema());
+
+ // Some plans will be candidates in projection pushdown rule to
+ // trim expressions based on expression names. We need to keep
+ // expression name for them.
+ let is_plan_for_projection_pushdown = matches!(
+ plan,
+ LogicalPlan::Window { .. }
Review comment:
Have you tried following the model in https://github.com/apache/arrow-datafusion/pull/1315/files#diff-1d33be1a7e8231e53102eab8112e30aa89d8f5cb8c21cd25bcfbce3050cdb433R110 ? I think that calls `columnize_expr` among perhaps some other differences.
Basically I think the code needs to do something like walk over the field names in the output `schema` and if they names of the rewritten exprs weren't the same add an alias;
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] viirya commented on a change in pull request #1319: Avoid changing expression names during constant folding
Posted by GitBox <gi...@apache.org>.
viirya commented on a change in pull request #1319:
URL: https://github.com/apache/arrow-datafusion/pull/1319#discussion_r752840803
##########
File path: datafusion/src/optimizer/constant_folding.rs
##########
@@ -101,7 +105,30 @@ impl OptimizerRule for ConstantFolding {
// fold constants and then simplify
.rewrite(&mut const_evaluator)?
.rewrite(&mut simplifier)?;
- Ok(new_e)
+
+ let new_name = &new_e.name(plan.schema());
+
+ // Some plans will be candidates in projection pushdown rule to
+ // trim expressions based on expression names. We need to keep
+ // expression name for them.
+ let is_plan_for_projection_pushdown = matches!(
+ plan,
+ LogicalPlan::Window { .. }
Review comment:
Not these approaches can work except the previous commit. Field name isn't equal to expression name for all cases. The cause of the problem is highly coupled with how projection push down rule works. Currently I don't find a solution to deal with all nodes other than current working version.
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] viirya commented on a change in pull request #1319: Avoid changing expression names during constant folding
Posted by GitBox <gi...@apache.org>.
viirya commented on a change in pull request #1319:
URL: https://github.com/apache/arrow-datafusion/pull/1319#discussion_r753499923
##########
File path: datafusion/src/optimizer/constant_folding.rs
##########
@@ -101,7 +105,30 @@ impl OptimizerRule for ConstantFolding {
// fold constants and then simplify
.rewrite(&mut const_evaluator)?
.rewrite(&mut simplifier)?;
- Ok(new_e)
+
+ let new_name = &new_e.name(plan.schema());
+
+ // Some plans will be candidates in projection pushdown rule to
+ // trim expressions based on expression names. We need to keep
+ // expression name for them.
+ let is_plan_for_projection_pushdown = matches!(
+ plan,
+ LogicalPlan::Window { .. }
Review comment:
Thanks @alamb . I'll keep trying on this too.
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] Dandandan commented on pull request #1319: Avoid changing expression names during constant folding
Posted by GitBox <gi...@apache.org>.
Dandandan commented on pull request #1319:
URL: https://github.com/apache/arrow-datafusion/pull/1319#issuecomment-980178446
I think this PR had an unintended side-effect on some other optimizers, like filter push down.
See https://github.com/apache/arrow-datafusion/issues/1367
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on a change in pull request #1319: Avoid changing expression names during constant folding
Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #1319:
URL: https://github.com/apache/arrow-datafusion/pull/1319#discussion_r754659853
##########
File path: datafusion/src/logical_plan/expr.rs
##########
@@ -1349,6 +1349,15 @@ pub fn unnormalize_cols(exprs: impl IntoIterator<Item = Expr>) -> Vec<Expr> {
exprs.into_iter().map(unnormalize_col).collect()
}
+/// Recursively un-alias an expressions
Review comment:
The "recursively" part may be misleading, this function unwraps all current aliases
So an expr like `(a as "foo") + (b as "bar")` will not be unaliased, but an expr like `(a as "foo") as "bar"` will be unaliased to "foo"
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] capkurmagati commented on pull request #1319: Avoid changing output column name before pushdown projection
Posted by GitBox <gi...@apache.org>.
capkurmagati commented on pull request #1319:
URL: https://github.com/apache/arrow-datafusion/pull/1319#issuecomment-971211300
Thanks @viirya.
I also thought it was the `ConstantFolding` optimizer leads to the bug when I filed the issue. But I couldn't digger further.
I tested the code locally and the test is passing but the cli still crashes for `SELECT count(1 + 1)`.
I wonder if it works in your environment?
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] Dandandan commented on pull request #1319: Avoid changing output column name before pushdown projection
Posted by GitBox <gi...@apache.org>.
Dandandan commented on pull request #1319:
URL: https://github.com/apache/arrow-datafusion/pull/1319#issuecomment-971819624
Great find!
I agree the general fix suggested by @alamb (to add / keep the original alias) is what we should do in this case. In general, I think the order of the optimizers shouldn't have any influence on the outcome of the query - only on the performance.
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on a change in pull request #1319: Avoid changing expression names during constant folding
Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #1319:
URL: https://github.com/apache/arrow-datafusion/pull/1319#discussion_r752660770
##########
File path: datafusion/src/optimizer/constant_folding.rs
##########
@@ -101,7 +105,30 @@ impl OptimizerRule for ConstantFolding {
// fold constants and then simplify
.rewrite(&mut const_evaluator)?
.rewrite(&mut simplifier)?;
- Ok(new_e)
+
+ let new_name = &new_e.name(plan.schema());
+
+ // Some plans will be candidates in projection pushdown rule to
+ // trim expressions based on expression names. We need to keep
+ // expression name for them.
+ let is_plan_for_projection_pushdown = matches!(
+ plan,
+ LogicalPlan::Window { .. }
Review comment:
(I agree with @Dandandan that this should apply to all nodes, not just a few special cased ones)
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] viirya commented on a change in pull request #1319: Avoid changing expression names during constant folding
Posted by GitBox <gi...@apache.org>.
viirya commented on a change in pull request #1319:
URL: https://github.com/apache/arrow-datafusion/pull/1319#discussion_r752636483
##########
File path: datafusion/src/optimizer/constant_folding.rs
##########
@@ -101,7 +105,30 @@ impl OptimizerRule for ConstantFolding {
// fold constants and then simplify
.rewrite(&mut const_evaluator)?
.rewrite(&mut simplifier)?;
- Ok(new_e)
+
+ let new_name = &new_e.name(plan.schema());
+
+ // Some plans will be candidates in projection pushdown rule to
+ // trim expressions based on expression names. We need to keep
+ // expression name for them.
+ let is_plan_for_projection_pushdown = matches!(
+ plan,
+ LogicalPlan::Window { .. }
Review comment:
For example, for Project, it will create many (looks redundant) aliases. Some looks okay but some looks really weird, e.g. some failed tests:
```
Projection: #test.a, #test.d, NOT #test.b AS test.b = Boolean(false)
...
```
```
Projection: Int32(0) AS CAST(Utf8(\"0\") AS Int32)
...
```
We have a lot tests that would be failed due to that.
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] viirya commented on pull request #1319: Avoid changing output column name before pushdown projection
Posted by GitBox <gi...@apache.org>.
viirya commented on pull request #1319:
URL: https://github.com/apache/arrow-datafusion/pull/1319#issuecomment-971375367
Hmm, there is a test `ctx_sql_should_optimize_plan` that assumes the logical plan after `sql` is optimized one. I thought it doesn't make sense as the logical plan is not a final one, e.g. you could do `select` etc. operations to produce new DataFrame (i.e., new logical plan). We only need to optimize it before creating physical plan (`create_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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on a change in pull request #1319: Avoid changing expression names during constant folding
Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #1319:
URL: https://github.com/apache/arrow-datafusion/pull/1319#discussion_r753478006
##########
File path: datafusion/src/optimizer/constant_folding.rs
##########
@@ -101,7 +105,30 @@ impl OptimizerRule for ConstantFolding {
// fold constants and then simplify
.rewrite(&mut const_evaluator)?
.rewrite(&mut simplifier)?;
- Ok(new_e)
+
+ let new_name = &new_e.name(plan.schema());
+
+ // Some plans will be candidates in projection pushdown rule to
+ // trim expressions based on expression names. We need to keep
+ // expression name for them.
+ let is_plan_for_projection_pushdown = matches!(
+ plan,
+ LogicalPlan::Window { .. }
Review comment:
Thanks for trying @viirya -- I'll see if I can find some time this weekend to mess around with it
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] houqp commented on a change in pull request #1319: Avoid changing expression names during constant folding
Posted by GitBox <gi...@apache.org>.
houqp commented on a change in pull request #1319:
URL: https://github.com/apache/arrow-datafusion/pull/1319#discussion_r753575943
##########
File path: datafusion/src/optimizer/constant_folding.rs
##########
@@ -101,7 +105,30 @@ impl OptimizerRule for ConstantFolding {
// fold constants and then simplify
.rewrite(&mut const_evaluator)?
.rewrite(&mut simplifier)?;
- Ok(new_e)
+
+ let new_name = &new_e.name(plan.schema());
+
+ // Some plans will be candidates in projection pushdown rule to
+ // trim expressions based on expression names. We need to keep
+ // expression name for them.
+ let is_plan_for_projection_pushdown = matches!(
+ plan,
+ LogicalPlan::Window { .. }
Review comment:
> For example, for Project, it will create many (looks redundant) aliases. Some looks okay but some looks really weird, e.g. some failed tests:
@viirya the example you gave here looks like correct behavior to me, are you concerned with lots of updates on the tests? or are there other unwanted side effect of this approach?
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] viirya commented on pull request #1319: Avoid changing expression names during constant folding
Posted by GitBox <gi...@apache.org>.
viirya commented on pull request #1319:
URL: https://github.com/apache/arrow-datafusion/pull/1319#issuecomment-974509898
If we inevitably need to update the tests with additional aliases, I'm not sure which one you prefer?
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] viirya commented on pull request #1319: Avoid changing expression names during constant folding
Posted by GitBox <gi...@apache.org>.
viirya commented on pull request #1319:
URL: https://github.com/apache/arrow-datafusion/pull/1319#issuecomment-974508956
For the approach of looking field name, it is similar. Some tests are needed to update, e.g.
1. optimizer::constant_folding::tests::optimize_plan_support_projectionorg_expr
```
left: `"Projection: #test.a, #test.d, NOT #test.b AS test.b = Boolean(false)\n TableScan: test projection=None"`,
right: `"Projection: #test.a, #test.d, NOT #test.b\n TableScan: test projection=None"`
```
2. optimizer::constant_folding::tests::optimize_plan_or_expr
```
left: `"Projection: #test.a\n Filter: NOT #test.b OR NOT #test.c AS a\n TableScan: test projection=None"`,
right: `"Projection: #test.a\n Filter: NOT #test.b OR NOT #test.c\n TableScan: test projection=None"`
```
3. optimizer::constant_folding::tests::optimize_plan_not_exp
```
left: `"Projection: #test.a\n Filter: #test.b AS a\n TableScan: test projection=None"`,
right: `"Projection: #test.a\n Filter: #test.b\n TableScan: test projection=None"`
```
4. optimizer::constant_folding::tests::to_timestamp_expr_folded
```
left: `"Projection: TimestampNanosecond(1599566400000000000)\n TableScan: test projection=None"`,
right: `"Projection: TimestampNanosecond(1599566400000000000) AS totimestamp(Utf8(\"2020-09-08T12:00:00+00:00\"))\n TableScan: test projection=None"`
```
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] viirya commented on pull request #1319: Avoid changing expression names during constant folding
Posted by GitBox <gi...@apache.org>.
viirya commented on pull request #1319:
URL: https://github.com/apache/arrow-datafusion/pull/1319#issuecomment-974691991
Thank you @houqp
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] viirya commented on pull request #1319: Avoid changing expression names during constant folding
Posted by GitBox <gi...@apache.org>.
viirya commented on pull request #1319:
URL: https://github.com/apache/arrow-datafusion/pull/1319#issuecomment-974622684
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb merged pull request #1319: Avoid changing expression names during constant folding
Posted by GitBox <gi...@apache.org>.
alamb merged pull request #1319:
URL: https://github.com/apache/arrow-datafusion/pull/1319
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] viirya commented on pull request #1319: Avoid changing output column name before pushdown projection
Posted by GitBox <gi...@apache.org>.
viirya commented on pull request #1319:
URL: https://github.com/apache/arrow-datafusion/pull/1319#issuecomment-971814900
@alamb Thanks for looking into this. I've thought about adding aliases too. Just tried with the current simple approach to see if it works.
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] viirya commented on pull request #1319: Avoid changing expression names during constant folding
Posted by GitBox <gi...@apache.org>.
viirya commented on pull request #1319:
URL: https://github.com/apache/arrow-datafusion/pull/1319#issuecomment-972476223
Also tested with the cli:
```
DataFusion CLI v5.1.0-SNAPSHOT
❯ SELECT count(1 + 1);
+----------------------------+
| COUNT(Int64(1) + Int64(1)) |
+----------------------------+
| 1 |
+----------------------------+
1 row in set. Query took 0.006 seconds.
❯
```
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] viirya commented on pull request #1319: Avoid changing expression names during constant folding
Posted by GitBox <gi...@apache.org>.
viirya commented on pull request #1319:
URL: https://github.com/apache/arrow-datafusion/pull/1319#issuecomment-974622684
I've updated all affected tests. Now the aliasing is applied on all nodes. Please let me know if you think this is okay. Thanks.
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] viirya commented on a change in pull request #1319: Avoid changing expression names during constant folding
Posted by GitBox <gi...@apache.org>.
viirya commented on a change in pull request #1319:
URL: https://github.com/apache/arrow-datafusion/pull/1319#discussion_r752665715
##########
File path: datafusion/src/optimizer/constant_folding.rs
##########
@@ -101,7 +105,30 @@ impl OptimizerRule for ConstantFolding {
// fold constants and then simplify
.rewrite(&mut const_evaluator)?
.rewrite(&mut simplifier)?;
- Ok(new_e)
+
+ let new_name = &new_e.name(plan.schema());
+
+ // Some plans will be candidates in projection pushdown rule to
+ // trim expressions based on expression names. We need to keep
+ // expression name for them.
+ let is_plan_for_projection_pushdown = matches!(
+ plan,
+ LogicalPlan::Window { .. }
Review comment:
> Basically I think the code needs to do something like walk over the field names in the output schema and if they names of the rewritten exprs weren't the same add an alias;
This sounds promising. No, I've not tried `columnize_expr`. Let me revise this and see if it works.
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] viirya commented on a change in pull request #1319: Avoid changing expression names during constant folding
Posted by GitBox <gi...@apache.org>.
viirya commented on a change in pull request #1319:
URL: https://github.com/apache/arrow-datafusion/pull/1319#discussion_r752777853
##########
File path: datafusion/src/optimizer/constant_folding.rs
##########
@@ -101,7 +105,30 @@ impl OptimizerRule for ConstantFolding {
// fold constants and then simplify
.rewrite(&mut const_evaluator)?
.rewrite(&mut simplifier)?;
- Ok(new_e)
+
+ let new_name = &new_e.name(plan.schema());
+
+ // Some plans will be candidates in projection pushdown rule to
+ // trim expressions based on expression names. We need to keep
+ // expression name for them.
+ let is_plan_for_projection_pushdown = matches!(
+ plan,
+ LogicalPlan::Window { .. }
Review comment:
I'm trying above approach. But must to note here is, it is still not for all nodes. For example, `Filter`'s schema is not related to its expressions. So we don't need (and shouldn't) add aliases for `Filter`.
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on pull request #1319: Avoid changing output column name before pushdown projection
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #1319:
URL: https://github.com/apache/arrow-datafusion/pull/1319#issuecomment-971782944
I may be mistaken, but I think @ic4y added aliases in https://github.com/apache/arrow-datafusion/pull/1315/files#diff-1d33be1a7e8231e53102eab8112e30aa89d8f5cb8c21cd25bcfbce3050cdb433R110
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] Dandandan commented on a change in pull request #1319: Avoid changing expression names during constant folding
Posted by GitBox <gi...@apache.org>.
Dandandan commented on a change in pull request #1319:
URL: https://github.com/apache/arrow-datafusion/pull/1319#discussion_r752632653
##########
File path: datafusion/src/optimizer/constant_folding.rs
##########
@@ -101,7 +105,30 @@ impl OptimizerRule for ConstantFolding {
// fold constants and then simplify
.rewrite(&mut const_evaluator)?
.rewrite(&mut simplifier)?;
- Ok(new_e)
+
+ let new_name = &new_e.name(plan.schema());
+
+ // Some plans will be candidates in projection pushdown rule to
+ // trim expressions based on expression names. We need to keep
+ // expression name for them.
+ let is_plan_for_projection_pushdown = matches!(
+ plan,
+ LogicalPlan::Window { .. }
Review comment:
Why only those?
What about `SELECT 1+1`.
Currently this outputs:
```
❯ SELECT 1+1
;
+----------+
| Int64(2) |
+----------+
| 2 |
+----------+
1 row in set. Query took 0.001 seconds.
```
I would assume it should keep the `Int64(1) + Int64(1)` here instead.
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] viirya edited a comment on pull request #1319: Avoid changing expression names during constant folding
Posted by GitBox <gi...@apache.org>.
viirya edited a comment on pull request #1319:
URL: https://github.com/apache/arrow-datafusion/pull/1319#issuecomment-974502648
Note that for the current approach that compares expression names, it works. The only issue is if we apply to all nodes there are many tests needed to be updated because many aliases are to be added there. E.g.,
1. optimizer::constant_folding::tests::optimize_plan_support_projection
```
left: `"Projection: #test.a, #test.d, NOT #test.b AS test.b = Boolean(false)\n TableScan: test projection=None"`,
right: `"Projection: #test.a, #test.d, NOT #test.b\n TableScan: test projection=None"`
```
2. optimizer::constant_folding::tests::optimize_plan_and_expr
```
left: `"Projection: #test.a\n Filter: NOT #test.b AND #test.c AS test.b != Boolean(true) AND test.c = Boolean(true)\n TableScan: test projection=None"`,
right: `"Projection: #test.a\n Filter: NOT #test.b AND #test.c\n TableScan: test projection=None"`
```
3. optimizer::constant_folding::tests::optimize_plan_not_expr
```
left: `"Projection: #test.a\n Filter: #test.b AS NOT test.b = Boolean(false)\n TableScan: test projection=None"`,
right: `"Projection: #test.a\n Filter: #test.b\n TableScan: test projection=None"`'
```
That's why I limit to certain nodes that I think mostly we like to add aliases to deal with projection push down rule.
If you think this is okay, I can make it apply to all nodes and update these tests.
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] viirya commented on a change in pull request #1319: Avoid changing expression names during constant folding
Posted by GitBox <gi...@apache.org>.
viirya commented on a change in pull request #1319:
URL: https://github.com/apache/arrow-datafusion/pull/1319#discussion_r753612970
##########
File path: datafusion/src/optimizer/constant_folding.rs
##########
@@ -101,7 +105,30 @@ impl OptimizerRule for ConstantFolding {
// fold constants and then simplify
.rewrite(&mut const_evaluator)?
.rewrite(&mut simplifier)?;
- Ok(new_e)
+
+ let new_name = &new_e.name(plan.schema());
+
+ // Some plans will be candidates in projection pushdown rule to
+ // trim expressions based on expression names. We need to keep
+ // expression name for them.
+ let is_plan_for_projection_pushdown = matches!(
+ plan,
+ LogicalPlan::Window { .. }
Review comment:
If it looks good for you, I will update all the tests.
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on a change in pull request #1319: Avoid changing expression names during constant folding
Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #1319:
URL: https://github.com/apache/arrow-datafusion/pull/1319#discussion_r754665089
##########
File path: datafusion/src/optimizer/constant_folding.rs
##########
@@ -626,8 +641,8 @@ mod tests {
let expected = "\
Projection: #test.a\
- \n Filter: NOT #test.c\
- \n Filter: #test.b\
+ \n Filter: NOT #test.c AS test.c = Boolean(false)\
Review comment:
👍
##########
File path: datafusion/src/optimizer/constant_folding.rs
##########
@@ -101,7 +105,18 @@ impl OptimizerRule for ConstantFolding {
// fold constants and then simplify
.rewrite(&mut const_evaluator)?
.rewrite(&mut simplifier)?;
- Ok(new_e)
+
+ let new_name = &new_e.name(plan.schema());
+
+ if let (Ok(expr_name), Ok(new_expr_name)) = (name, new_name) {
+ if expr_name != new_expr_name {
+ Ok(new_e.alias(expr_name))
+ } else {
+ Ok(new_e)
+ }
+ } else {
+ Ok(new_e)
Review comment:
I worry we may be silently ignoring some real issues in the future.
However, I tried checking `expr_name` and `new_expr_name` for errors and I got a bunch of errors like
```
---- execution::context::tests::window_partition_by stdout ----
Error: Internal("Create name does not support sort expression")
thread 'execution::context::tests::window_partition_by' panicked at 'assertion failed: `(left == right)`
left: `1`,
right: `0`: the test returned a termination value with a non-zero status code (1) which indicates a failure', /rustc/59eed8a2aac0230a8b53e89d4e99d55912ba6b35/library/test/src/lib.rs:194:5
stack backtrace:
0: rust_begin_unwind
at /rustc/59eed8a2aac0230a8b53e89d4e99d55912ba6b35/library/std/src/panicking.rs:517:5
1: core::panicking::panic_fmt
at /rustc/59eed8a2aac0230a8b53e89d4e99d55912ba6b35/library/core/src/panicking.rs:101:14
2: core::panicking::assert_failed_inner
at /rustc/59eed8a2aac0230a8b53e89d4e99d55912ba6b35/library/core/src/panicking.rs:177:23
3: core::panicking::assert_failed
at /rustc/59eed8a2aac0230a8b53e89d4e99d55912ba6b35/library/core/src/panicking.rs:140:5
4: test::assert_test_result
at /rustc/59eed8a2aac0230a8b53e89d4e99d55912ba6b35/library/test/src/lib.rs:194:5
5: datafusion::execution::context::tests::window_partition_by::{{closure}}
at ./src/execution/context.rs:1771:11
6: core::ops::function::FnOnce::call_once
at /rustc/59eed8a2aac0230a8b53e89d4e99d55912ba6b35/library/core/src/ops/function.rs:227:5
7: core::ops::function::FnOnce::call_once
at /rustc/59eed8a2aac0230a8b53e89d4e99d55912ba6b35/library/core/src/ops/function.rs:227:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
````
So I suppose this is as good as we are going to do for now
##########
File path: datafusion/src/optimizer/constant_folding.rs
##########
@@ -92,6 +92,10 @@ impl OptimizerRule for ConstantFolding {
.expressions()
.into_iter()
.map(|e| {
+ // We need to keep original expression name, if any.
+ // Constant folding should not change expression name.
Review comment:
👍
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] viirya commented on pull request #1319: Avoid changing expression names during constant folding
Posted by GitBox <gi...@apache.org>.
viirya commented on pull request #1319:
URL: https://github.com/apache/arrow-datafusion/pull/1319#issuecomment-975957882
Thank you @alamb @Dandandan @houqp @capkurmagati !
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] viirya commented on pull request #1319: Avoid changing output column name before pushdown projection
Posted by GitBox <gi...@apache.org>.
viirya commented on pull request #1319:
URL: https://github.com/apache/arrow-datafusion/pull/1319#issuecomment-971361265
@capkurmagati Oh, I've not tested the cli before you asked. I just tested it and found another issue. It is because we do twice optimization on the logical plan, first time is when parsing sql into DataFrame, second time is when creating physical plan. Just updated with new change.
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org