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-unsubscr...@arrow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
