Avoid copies in CountWildcardRule via TreeNode API
#10066
Conversation
alamb
force-pushed
the
alamb/count_with_wildcard
branch
from
ee2003d to
692fc11
Compare
| } | ||
| fn analyze_internal(plan: LogicalPlan) -> Result<Transformed<LogicalPlan>> { | ||
| let name_preserver = NamePreserver::new(&plan); | ||
| plan.map_expressions(|expr| { |
There was a problem hiding this comment.
Not only does this logic avoid the clone (old_expr.clone()) I also think it is easier to understand what is happening
| @@ -47,181 +38,43 @@ impl CountWildcardRule { | |||
|
|
|||
| impl AnalyzerRule for CountWildcardRule { | |||
| fn analyze(&self, plan: LogicalPlan, _: &ConfigOptions) -> Result<LogicalPlan> { | |||
| plan.transform_down(&analyze_internal).data() | |||
| plan.transform_down_with_subqueries(&analyze_internal) | |||
There was a problem hiding this comment.
By using the wonderful transform_down_with_subqueries from @peter-toth this rule can avoid having to recuse into subqueries directly by itself
| let window_expr = window | ||
| .window_expr | ||
| .iter() | ||
| .map(|expr| rewrite_preserving_name(expr.clone(), &mut rewriter)) |
There was a problem hiding this comment.
removed this clone (and the ones below)
| type Node = Expr; | ||
|
|
||
| fn f_up(&mut self, old_expr: Expr) -> Result<Transformed<Expr>> { | ||
| Ok(match old_expr.clone() { |
There was a problem hiding this comment.
here is another clone that is avoided
| @@ -64,7 +64,7 @@ impl ApplyFunctionRewrites { | |||
| let original_name = name_preserver.save(&expr)?; | |||
|
|
|||
| // recursively transform the expression, applying the rewrites at each step | |||
| let result = expr.transform_up(&|expr| { | |||
There was a problem hiding this comment.
drive by cleanup based on @crepererum 's comment on #10038 (comment)
| let transformed_expr = expr.transform_up(&|expr| match expr { | ||
| Expr::WindowFunction(mut window_function) | ||
| if window_function.args.len() == 1 | ||
| && is_wildcard(&window_function.args[0]) => |
There was a problem hiding this comment.
do we need to match the function Count as well?
There was a problem hiding this comment.
This is a good catch.
I think technically it is not necessary as no other aggregates supports *. However, I added a check for the aggregate name and added a test for SUM(*) in a05bcbe
Which issue does this PR close?
Part of #9637
Rationale for this change
Let's make planning faster by not copying so much in the optimizer using the new TreeNode API.
As an added bonus, now that the traversal is encoded in
TreeNodedoing this replacement also requires much less codeWhat changes are included in this PR?
Are these changes tested?
Covered by existing CI
Are there any user-facing changes?
No (maybe tiny bit faster planning)