Simpler to see expressions in explain tree mode
#15163
Conversation
|
Thank you @irenjj - I will check this out tomorrow |
There was a problem hiding this comment.
Thank you so much @irenjj -- this looks really nice ❤️
I left some documentation suggestions.
Other than that, can you please hook up this format to one of the TreeExplain implementations so we
- Can see what a plan would look like
- Make sure the API is usable
|
Thanks @alamb , have added an example for Projection, it looks better than before. |
| 22)┌─────────────┴─────────────┐ | ||
| 23)│ SortExec │ | ||
| 24)│ -------------------- │ | ||
| 25)│ v1@0 ASC NULLS LAST │ |
There was a problem hiding this comment.
eventually it will be great to remove the @0 here too
There was a problem hiding this comment.
Maybe we can file a follow on PR to improve other plans 🤔
There was a problem hiding this comment.
Yep, I'd like to fix the expression printing format for all these plans in the next PR, since most plans' printing format are supported, filed #15238.
| 24)│ files: 1 │ | ||
| 25)│ format: csv │ | ||
| 26)└───────────────────────────┘ | ||
| 04)│ count(Int64(1)) ROWS │ |
There was a problem hiding this comment.
formatting this as 1 rather than Int64 will be great eventually too
There was a problem hiding this comment.
That's a good idea! The output format of count(Int64(1)) is not controlled by fmt_sql, it's a column generated by AggregateExec, I'll try to resolve it in next pr.
There was a problem hiding this comment.
The output of AggregateExec also seems to contain redundant information.
I debugged the code and found that the name of AggregateFunctionExpr is constructed in create_aggregate_expr_and_maybe_filter. In this function, debug information is generated for all Expr instances through Expr's SchemaDisplay.
To address this issue, I propose the following solution:
- Add a new member
sql_nametoAggregateFunctionExpr. - Introduce a new method
fmt_sql_name()forExpr, similar toschema_name(), and override it inAggregateFunctionto generatesql_name. - Modify
fmt_asinAggregateExecto outputaggr_expr.sql_nameinstead ofaggr_expr.name.
I'm not sure if this idea is correct. If you have any suggestions, that would be even better.❤️
There was a problem hiding this comment.
I'm not sure if this idea is correct. If you have any suggestions, that would be even better.❤️
I am not sure either -- maybe we can try it and see how it looks
| @@ -379,3 +396,53 @@ where | |||
|
|
|||
| DisplayWrapper(exprs.into_iter()) | |||
| } | |||
|
|
|||
| /// Prints a [`PhysicalExpr`] in a SQL-like format | |||
There was a problem hiding this comment.
I moved the code here and added an example
alamb
changed the title
tree mode
|
Thanks again @irenjj |
Which issue does this PR close?
treeexplain mode #15107Rationale for this change
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?