Simplify display format of `AggregateFunctionExpr`, add `Expr::sql_name` by irenjj · Pull Request #15253 · apache/datafusion

irenjj · 2025-03-15T16:25:09Z

Which issue does this PR close?

Closes [tree explain] Simplify display format of AggregateFunctionExpr #15252

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

irenjj · 2025-03-15T16:34:55Z

datafusion/expr/src/expr.rs

+struct SqlDisplay<'a>(&'a Expr);
+impl Display for SqlDisplay<'_> {
+    fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result {
+        match self.0 {
+            Expr::Column(c) => {
+                write!(f, "{}", c.name)
+            }
+            Expr::Literal(_) => {
+                write!(f, "aa")
+            }
+            Expr::ScalarVariable(..) => {
+                write!(f, "bb")
+            }
+            Expr::OuterReferenceColumn(..) => {
+                write!(f, "cc")
+            }
+            Expr::Placeholder(_) => {
+                write!(f, "dd")
+            }
+            Expr::Wildcard { .. } => {
+                write!(f, "ee")
+            }
+            Expr::AggregateFunction(AggregateFunction { func, params }) => {
+                match func.sql_name(params) {
+                    Ok(name) => {
+                        write!(f, "{name}")
+                    }
+                    Err(e) => {
+                        write!(f, "got error from schema_name {}", e)
+                    }
+                }
+            }
+            _ => write!(f, "{}", self.0.schema_name()),
+        }
+    }
+}
+


Just a demo here.
AggregateFunctionExpr's name is constructed at logical expr phase, we need a new member sql_name for tree explain, and a new constructor SqlDisplay to handle the inner Expr recursively. cc @alamb 👀

irenjj · 2025-03-15T16:35:37Z

datafusion/sqllogictest/test_files/explain_tree.slt

 13)-----------------------------│       AggregateExec       │
 14)-----------------------------│    --------------------   │
-15)-----------------------------│   aggr: count(Int64(1))   │
+15)-----------------------------│      aggr: count(aa)      │


We can see the final display changed.

…_expr

alamb

Thank you @irenjj -- this looks really nice

I think there are a few ways we should improve the comments, ~~but I'll directly push those to this branch.~~

Thanks again!

Update; I don't have time right now to make the comment changes. Let me know what you think @irenjj and if you are willing to make them in this PR. If not I'll make a follow on PR tomorrow.

Thanks again ❤️

alamb · 2025-03-17T16:15:34Z

datafusion/expr/src/expr.rs

 }

+struct SqlDisplay<'a>(&'a Expr);
+impl Display for SqlDisplay<'_> {


We already have code to convert from Expr to SQL, in https://docs.rs/datafusion/latest/datafusion/sql/unparser/struct.Unparser.html#method.expr_to_sql

However, datafusion-expr doesn't currently depend on the datafusion-sql crate which is probably a good thing

So I think we should add a note in Expr::sql_name that if the user's want syntactically correct SQL to feed to some other system, they should use the Unparser and leave a link to https://docs.rs/datafusion/latest/datafusion/sql/unparser/struct.Unparser.html

alamb · 2025-03-17T18:31:49Z

datafusion/expr/src/expr.rs

    schema_name_from_exprs_inner(exprs, ",")
 }

+/// Get `sql_name` for Vector of expressions.


Instead of adding so many new so specific functions, what do you think about adding something like the following. I think the API would be easier to understand and it would also let us save a String copy when formatting as sql

/// Formats a list of `&Expr` with a custom separator struct ExprListDisplay<'a> { exprs: &'a [Expr], sep: &'a str, } ... impl <'a> Display for ExprListDisplay<'a> { ... }

datafusion/physical-expr/src/aggregate.rs

datafusion/physical-plan/src/aggregates/mod.rs

alamb · 2025-03-17T18:40:58Z

datafusion/sqllogictest/test_files/explain_tree.slt

-51)│          files: 1         │
-52)│        format: csv        │
-53)└───────────────────────────┘
+07)│    group_by: string_col   │


alamb · 2025-03-17T18:41:06Z

datafusion/sqllogictest/test_files/explain_tree.slt

-70)│       format: memory      ││       format: memory      │
-71)│          rows: 1          ││          rows: 1          │
-72)└───────────────────────────┘└───────────────────────────┘
+10)│       aggr: count(1)      │


this is wonderful

irenjj · 2025-03-18T00:10:22Z

Thank you @irenjj -- this looks really nice

I think there are a few ways we should improve the comments, ~~but I'll directly push those to this branch.~~

Thanks again!

Update; I don't have time right now to make the comment changes. Let me know what you think @irenjj and if you are willing to make them in this PR. If not I'll make a follow on PR tomorrow.

Thanks again ❤️

Thanks @alamb for your review, I will revise it later according to your suggestions.

jayzhan211 · 2025-03-18T00:36:51Z

datafusion/expr/src/expr.rs

 }

+struct SqlDisplay<'a>(&'a Expr);
+impl Display for SqlDisplay<'_> {


What is the main difference between Display? I found alias is different but others looks similar

That's true, it's redundant for most cases.

Maybe we should take nested expr into consideration, like aggr(case aggr() when...)

jayzhan211 · 2025-03-18T00:38:45Z

datafusion/expr/src/expr.rs

+                    }
+                }
+            }
+            _ => write!(f, "{}", self.0.schema_name()),


I think by default should be Display, self.0.to_string()

jayzhan211 · 2025-03-18T00:40:16Z

datafusion/expr/src/expr.rs

 }

+/// Get `sql_name` for Vector of expressions.
+pub(crate) fn sql_name_from_exprs_comma_separated_without_space(


without_space is the old format and ideally we should standardize the format #10364 so better not to introduce another format without good reason

jayzhan211 · 2025-03-18T00:41:13Z

Is it possible to modify Display for Expr for explain statement?

xudong963 · 2025-03-18T11:28:46Z

datafusion/sqllogictest/test_files/explain_tree.slt

 04)│   aggr: sum(bigint_col)   │
-05)│                           │
-06)│         group_by:         │
-07)│ string_col@0 as string_col│


I think it'll be helpful to keep the index @0, what do you think?

It's true that column indices provide precise tracking and tracing capabilities for complex query analysis, but I think tree explain should be as simple as possible🤔, as doc says:

/// TreeRender, displayed in the `tree` explain type. /// /// This format is inspired by DuckDB's explain plans. The information /// presented should be "user friendly", and contain only the most relevant /// information for understanding a plan. It should NOT contain the same level /// of detail information as the [`Self::Default`] format.

I think the key difference Display and the now named human_format is that Display has more details -- I tried to add comments that explain this

irenjj · 2025-03-18T11:53:51Z

Is it possible to modify Display for Expr for explain statement?

I haven't tried it, not sure how it will affect other logic.

Co-authored-by: Andrew Lamb <[email protected]>

irenjj · 2025-03-18T13:36:23Z

Thanks @alamb, @jayzhan211 and @xudong963 for your review, here are two points that remain unclear:

For GROUP BY, is it necessary to preserve the row index -- for more information, additional functionality, or just better aesthetics?
Should the default Display implementation of Expr be modified to replace SqlDisplay?

Co-authored-by: Andrew Lamb <[email protected]>

alamb · 2025-03-18T19:17:37Z

Thanks @alamb, @jayzhan211 and @xudong963 for your review, here are two points that remain unclear:

For GROUP BY, is it necessary to preserve the row index -- for more information, additional functionality, or just better aesthetics?

I don't think we should keep the index for the tree explain plan

Is it possible to modify Display for Expr for explain statement?

Should the default Display implementation of Expr be modified to replace SqlDisplay?

I think this would be a more disruptive change as it would change the existing EXPLAIN ... output. Thus I think we should consider it as a follow on PR

alamb · 2025-03-18T19:24:03Z

I pushed a commit to add more documentation explaining the different options for printing Exprs

I also pushed a commit to rename sql_name to human_display as that seemed much more explanatory when I was writing the documentation

Let me know what you think

alamb · 2025-03-18T19:25:27Z

datafusion/expr/src/expr.rs

 ///
 /// [`ExprFunctionExt`]: crate::expr_fn::ExprFunctionExt
 ///
+/// # Printing Expressions


I added this section and the one below to try and explain the uses of the different explain variants.

alamb · 2025-03-19T19:29:11Z

I'll plan to merge this tomorrow unless others would like time to review

alamb · 2025-03-20T14:54:34Z

Thank you @irenjj @jayzhan211 and @xudong963 🙏

berkaysynnada · 2025-03-28T12:57:44Z

@irenjj AggregateFunctionExpr has with_new_expressions() API. As datafusion hasn't implemented it yet, you didn't have difficulty rewriting the human_display according to the new expressions. Could you suggest a way to update human_readable field for the implementers of with_new_expressions()?

irenjj · 2025-03-28T14:06:38Z

@irenjj AggregateFunctionExpr has with_new_expressions() API. As datafusion hasn't implemented it yet, you didn't have difficulty rewriting the human_display according to the new expressions. Could you suggest a way to update human_readable field for the implementers of with_new_expressions()?

I believe that the human_display in AggregateFunctionExpr is constructed during the logical plan phase based on the available information at that time (and the name in AggregateFunctionExpr is similar). On the other hand, with_new_expressions() replaces the args in AggregateFunctionExpr during the physical plan phase (perhaps related to the optimizer?). It's perfectly fine to keep human_display unchanged when implementing with_new_expressions() because the name used in the original indented explain is actually quite similar to human_display.

Perhaps what we should really focus on is ensuring that the explain output correctly displays the name of the expressions after with_new_expressions() has been applied.

github-actions bot added logical-expr Logical plan and expressions physical-expr Changes to the physical-expr crates core Core DataFusion crate sqllogictest SQL Logic Tests (.slt) labels Mar 15, 2025

irenjj commented Mar 15, 2025

View reviewed changes

irenjj added 2 commits March 17, 2025 20:42

Simplify display format of AggregateFunctionExpr

f8dd65a

Merge remote-tracking branch 'upstream/main' into irenjj/simplify_agg…

03e33a4

…_expr

irenjj force-pushed the irenjj/simplify_agg_expr branch from 30efcd5 to 03e33a4 Compare March 17, 2025 12:47

github-actions bot removed the sqllogictest SQL Logic Tests (.slt) label Mar 17, 2025

add more info for SqlDisplay

3610bf5

github-actions bot added the sqllogictest SQL Logic Tests (.slt) label Mar 17, 2025

irenjj added 3 commits March 17, 2025 22:54

simplify aggr expr

38d34e2

add doc

3e8f1d0

add table name

5929ef4

irenjj marked this pull request as ready for review March 17, 2025 15:28

alamb changed the title ~~Simplify display format of AggregateFunctionExpr~~ Simplify display format of AggregateFunctionExpr, add Expr::sql_name Mar 17, 2025

alamb approved these changes Mar 17, 2025

View reviewed changes

jayzhan211 reviewed Mar 18, 2025

View reviewed changes

xudong963 reviewed Mar 18, 2025

View reviewed changes

irenjj and others added 4 commits March 18, 2025 21:30

fix issues

f13a0ca

Update datafusion/physical-expr/src/aggregate.rs

2720261

Co-authored-by: Andrew Lamb <[email protected]>

Update datafusion/physical-expr/src/aggregate.rs

316ab9b

Co-authored-by: Andrew Lamb <[email protected]>

Update datafusion/physical-plan/src/aggregates/mod.rs

202a1d2

Co-authored-by: Andrew Lamb <[email protected]>

Update datafusion/physical-plan/src/aggregates/mod.rs

f0e0798

Co-authored-by: Andrew Lamb <[email protected]>

irenjj and others added 4 commits March 18, 2025 21:42

fmt

b7e985d

fix build

acc0dc5

improve docs

af13820

Add documentation about sql_format

e1bdb5f

alamb added 2 commits March 18, 2025 15:22

Rename to human_display

76eb7f9

Merge remote-tracking branch 'apache/main' into irenjj/simplify_agg_expr

4b9c89e

alamb reviewed Mar 18, 2025

View reviewed changes

jayzhan211 approved these changes Mar 20, 2025

View reviewed changes

xudong963 approved these changes Mar 20, 2025

View reviewed changes

alamb merged commit 11838be into apache:main Mar 20, 2025
27 checks passed

alamb mentioned this pull request Mar 25, 2025

Scalars are too verbose in column name output #15395

Open

irenjj deleted the irenjj/simplify_agg_expr branch June 23, 2025 00:03

Comments

Conversation

irenjj commented Mar 15, 2025

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

irenjj commented Mar 18, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jayzhan211 commented Mar 18, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

irenjj commented Mar 18, 2025

Uh oh!

irenjj commented Mar 18, 2025

Uh oh!

alamb commented Mar 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alamb commented Mar 18, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb commented Mar 19, 2025

Uh oh!

alamb commented Mar 20, 2025

Uh oh!

Uh oh!

berkaysynnada commented Mar 28, 2025

Uh oh!

irenjj commented Mar 28, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

alamb left a comment •

edited

Loading

alamb commented Mar 18, 2025 •

edited

Loading