Preserve the name of grouping sets in SimplifyExpressions #14888
Conversation
Whenever we use `recompute_schema` or `with_exprs_and_inputs`, this ensures that we obtain the same schema.
| }; | ||
|
|
||
| plan.map_expressions(|expr| { | ||
| // Preserve the aliasing of grouping sets. |
There was a problem hiding this comment.
This line seems like it does the opposite (doesn't preserve the original names 🤔 )
There was a problem hiding this comment.
What makes you think so?
There was a problem hiding this comment.
Grouping sets needs to maintain the alias of the children expressions as the field names needs to be based on that rather than the outer expression.
There was a problem hiding this comment.
What makes you think so?
I was thinking that the code skips calling rewrite_expr (which calls name preserver) for the GroupingSets (so thus does not preserve the aliases of the Expr::Grouping itself
To be clear I think the code in PR looks good to me, I am just discussing if we can make the comment better
There was a problem hiding this comment.
Ok, I meant it in the sense that Expr::GroupinSets is a container of the actual grouping sets (which are a Vec<Expr> or Vec<Vec<Expr>>) and we want to preserve their names. But I'm happy to reword it however you like.
There was a problem hiding this comment.
no need to reword if I am the only one confused :)
|
Thanks again @joroKr21 |
Whenever we use
recompute_schemaorwith_exprs_and_inputs, this ensures that we obtain the same schema.Which issue does this PR close?
Followup to #14734
Rationale for this change
In #14734 we introduced more aggressive constant folding which applies also to aliases. That lead to the discovery that
SimplifyExpressionsdoesn't preserve the name of grouping sets which are relevant for the computed schema. Note that this could be an issue also before #14734 because the grouping set might have no alias and then if the expression were changed by constant folding, it would still affect the result schema.What changes are included in this PR?
In
SimplifyExpressionswe check for grouping sets and then map over their children with theNamePreserver.Otherwise, same as before.
Are these changes tested?
Added a unit test.
Are there any user-facing changes?
No user facing changes, the schema of grouping sets should be more stable after optimizations.