Consolidate and better tests for expression re-rewriting / aliasing #3727

alamb · 2022-10-05T17:46:53Z

~~Draft as it builds on #3710~~

Which issue does this PR close?

Rationale for this change

There are at least three places that we have rediscovered the issue of names changing when rewriting expressions such as #3704, #3555 and https://github.com/apache/arrow-datafusion/blob/master/datafusion/optimizer/src/simplify_expressions.rs#L316

I hope we can avoid other tricky bugs in the future

What changes are included in this PR?

introduce a function rewrite_preserving_name that handles aliasing during rewrites
Change code to use rewrite_preserving_name
Add tests

Are there any user-facing changes?

no

alamb · 2022-10-05T18:31:47Z

datafusion/optimizer/src/type_coercion.rs

-            }
-
-            Ok(expr)
+            rewrite_preserving_name(expr, &mut expr_rewrite)


fyi @thinkharderdev

alamb · 2022-10-05T18:32:05Z

datafusion/optimizer/src/type_coercion.rs

@@ -637,7 +621,8 @@ mod test {
        let mut config = OptimizerConfig::default();
        let plan = rule.optimize(&plan, &mut config)?;
        assert_eq!(
-            "Projection: a IN ([CAST(Int32(1) AS Int64), CAST(Int8(4) AS Int64), Int64(8)])\n  EmptyRelation",
+            "Projection: a IN ([CAST(Int32(1) AS Int64), CAST(Int8(4) AS Int64), Int64(8)]) AS a IN (Map { iter: Iter([Int32(1), Int8(4), Int64(8)]) })\


cc @liukun4515 -- I think this is an improvement (and maybe a bug fix 🤔 )

alamb · 2022-10-05T18:33:03Z

datafusion/optimizer/src/unwrap_cast_in_comparison.rs

-            let expr = expr.rewrite(&mut expr_rewriter)?;
-            add_alias_if_changed(&original_name, expr)
-        })
+        .map(|expr| rewrite_preserving_name(expr, &mut expr_rewriter))


this PR basically refactors the code into rewrite_preserving_name, adds tests, and calls it in a few places

liukun4515 · 2022-10-06T03:50:54Z

datafusion/optimizer/src/utils.rs

+/// `Expr::Sort` as appropriate
+fn name_for_alias(expr: &Expr) -> Result<String> {
+    match expr {
+        Expr::Sort { expr, .. } => name_for_alias(expr),


I missed this issue #3710

But I want to know why we need to do the special branch for Sort Expr?

Basically because calling Expr::name() on a Expr::Sort will throw an error:

https://github.com/apache/arrow-datafusion/blob/7c5c2e5/datafusion/expr/src/expr.rs#L1133-L1135

I am not super thrilled in general about how this works -- I wonder if I should support calling Expr::name() on Expr::Sort 🤔

liukun4515 · 2022-10-06T03:56:56Z

I think there are may place which use the ExprRewriter to rewrite the expr in the optimizer like ConstEvaluator and other.

Do we need to fix all of them?

alamb · 2022-10-06T10:45:39Z

I think there are may place which use the ExprRewriter to rewrite the expr in the optimizer like ConstEvaluator and other.
Do we need to fix all of them?

That is an excellent point @liukun4515 -- I think we probably do. I will file a follow on ticket to track and do so

alamb · 2022-10-06T11:02:26Z

Github actions is having issues I think -- the CI failures are not related to changes in this PR

liukun4515 · 2022-10-06T11:39:55Z

I think there are may place which use the ExprRewriter to rewrite the expr in the optimizer like ConstEvaluator and other.
Do we need to fix all of them?

That is an excellent point @liukun4515 -- I think we probably do. I will file a follow on ticket to track and do so

I have less knowledge about the alias for the expr, can you explain when and where we should do the alias for the expr which will be rewrited.

alamb · 2022-10-06T13:25:15Z

I have less knowledge about the alias for the expr, can you explain when and where we should do the alias for the expr which will be rewrited.

I will file a ticket that explains the issue in more detail

alamb · 2022-10-11T11:02:21Z

I have less knowledge about the alias for the expr, can you explain when and where we should do the alias for the expr which will be rewrited.

@liukun4515 I have filed #3794 which describes the issue more generally and explains why and when aliases are needed

liukun4515 · 2022-10-12T14:23:01Z

I have less knowledge about the alias for the expr, can you explain when and where we should do the alias for the expr which will be rewrited.

@liukun4515 I have filed #3794 which describes the issue more generally and explains why and when aliases are needed

I will take a look your issue with this pr #3788, but it will take more time for this.

alamb · 2022-10-12T14:54:51Z

Unless there are objections, I plan to merge this PR as it doesn't change any behavior and adds additional test coverage. We can continue to improve the code in other places it may be renaming incorrectly as part of #3794

xudong963 · 2022-10-12T15:18:13Z

Thanks @alamb , LGTM. Let's merge!

ursabot · 2022-10-12T15:22:31Z

Benchmark runs are scheduled for baseline = 61c38b7 and contender = e27e86b. e27e86b is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ec2-t3-xlarge-us-east-2] ec2-t3-xlarge-us-east-2
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on test-mac-arm] test-mac-arm
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ursa-i9-9960x] ursa-i9-9960x
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ursa-thinkcentre-m75q] ursa-thinkcentre-m75q
Buildkite builds:
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

alamb · 2022-10-12T16:07:40Z

🥳

github-actions bot added logical-expr Logical plan and expressions optimizer Optimizer rules labels Oct 5, 2022

alamb mentioned this pull request Oct 5, 2022

Fix aggregate type coercion bug #3710

Merged

Consolidate and better tests for expression re-rewriting / aliasing

a82ad9e

alamb force-pushed the alamb/cleanup_rename branch from cec55d1 to a82ad9e Compare October 5, 2022 18:31

alamb commented Oct 5, 2022

View reviewed changes

alamb requested a review from liukun4515 October 5, 2022 18:33

alamb marked this pull request as ready for review October 5, 2022 18:33

liukun4515 reviewed Oct 6, 2022

View reviewed changes

Merge remote-tracking branch 'apache/master' into alamb/cleanup_rename

3f8f94f

Merge remote-tracking branch 'apache/master' into alamb/cleanup_rename

74934dc

alamb mentioned this pull request Oct 11, 2022

When DataFusion optimizer rewrites an expression, it should not change the expressions "name" / "identity" / "alias" #3794

Closed

xudong963 approved these changes Oct 12, 2022

View reviewed changes

xudong963 merged commit e27e86b into apache:master Oct 12, 2022

alamb deleted the alamb/cleanup_rename branch October 12, 2022 16:07

jayzhan211 mentioned this pull request Apr 29, 2024

Better Cast name for display #10276

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Consolidate and better tests for expression re-rewriting / aliasing #3727

Consolidate and better tests for expression re-rewriting / aliasing #3727

alamb commented Oct 5, 2022 •

edited

Loading

alamb Oct 5, 2022

alamb Oct 5, 2022

alamb Oct 5, 2022

liukun4515 Oct 6, 2022 •

edited

Loading

alamb Oct 6, 2022 •

edited

Loading

liukun4515 commented Oct 6, 2022 •

edited

Loading

alamb commented Oct 6, 2022

alamb commented Oct 6, 2022

liukun4515 commented Oct 6, 2022

alamb commented Oct 6, 2022

alamb commented Oct 11, 2022

liukun4515 commented Oct 12, 2022

alamb commented Oct 12, 2022

xudong963 commented Oct 12, 2022

ursabot commented Oct 12, 2022

alamb commented Oct 12, 2022

Consolidate and better tests for expression re-rewriting / aliasing #3727

Consolidate and better tests for expression re-rewriting / aliasing #3727

Conversation

alamb commented Oct 5, 2022 • edited Loading

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

alamb Oct 5, 2022

Choose a reason for hiding this comment

alamb Oct 5, 2022

Choose a reason for hiding this comment

alamb Oct 5, 2022

Choose a reason for hiding this comment

liukun4515 Oct 6, 2022 • edited Loading

Choose a reason for hiding this comment

alamb Oct 6, 2022 • edited Loading

Choose a reason for hiding this comment

liukun4515 commented Oct 6, 2022 • edited Loading

alamb commented Oct 6, 2022

alamb commented Oct 6, 2022

liukun4515 commented Oct 6, 2022

alamb commented Oct 6, 2022

alamb commented Oct 11, 2022

liukun4515 commented Oct 12, 2022

alamb commented Oct 12, 2022

xudong963 commented Oct 12, 2022

ursabot commented Oct 12, 2022

alamb commented Oct 12, 2022

alamb commented Oct 5, 2022 •

edited

Loading

liukun4515 Oct 6, 2022 •

edited

Loading

alamb Oct 6, 2022 •

edited

Loading

liukun4515 commented Oct 6, 2022 •

edited

Loading