fix(aggregate): show aliased expr in explain#21739
fix(aggregate): show aliased expr in explain#21739kumarUjjawal wants to merge 3 commits intoapache:mainfrom
Conversation
github-actions
bot
added
physical-expr
|
cc @pepijnve |
|
It has much more cases than I realized initially 😢 |
|
I updated the code with new changes along with pr body |
github-actions
bot
added
documentation
…plain # Conflicts: # datafusion/optimizer/src/decorrelate_lateral_join.rs
|
I tried two approaches for this. The first approach was by hiding the bit in alias metadata. It fixed the display problem, but it also mixed planner-only state with user metadata. That made the behavior harder to reason about and forced extra handling in places like equality, hashing, serialization, and rewrite logic. In practice, a simple display fix startedaffecting alias identity and metadata flow in too many places. The current approach adds an explicit internal flag to alias. This is a small API break, but it makes the model much clearer: whether an alias is user written or planner generated is now part of the type itself, not hidden in metadata. That keeps the display logic direct, avoids hidden state leaking into unrelated code paths, and makes future maintenance safer because the intent is visible and compiler checked. Would love to hear your thoughts @pepijnve |
|
@kumarUjjawal could you give an example of where/when the internal flag is necessary? |
|
I think I might have found it in your second commit. Where this change was reverted. 
Wouldn't we want the left version though? The physical plan is actually executing |
| .map(|expr| expr.human_display()) | ||
| .map(|expr| { | ||
| let human_display = expr.human_display(); | ||
| if human_display.is_empty() { |
There was a problem hiding this comment.
Should human_display be Option<String>?
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn test_aggregate_explain_shows_aliased_expression() -> Result<()> { |
There was a problem hiding this comment.
These might be more concise as SLTs
2 participants
Which issue does this PR close?
Rationale for this change
Physical explain output only showed the alias for aliased aggregates. That made it hard to understand the plan, especially when the aggregate had a filter, explicit RESPECT NULLS, or a custom UDAF display.
What changes are included in this PR?
Are these changes tested?
Yes
Are there any user-facing changes?
Yes.
This is a public API change for users who build or pattern match Alias directly. The upgrade guide has been updated with the needed changes.