Improve constant folding for associative operations#23215
Improve constant folding for associative operations#23215hadrian-reppas wants to merge 4 commits into
Conversation
| impl<'n> TreeNodeVisitor<'n> for AdjacentLiteralVisitor { | ||
| type Node = Expr; | ||
|
|
||
| fn f_down(&mut self, node: &'n Self::Node) -> Result<TreeNodeRecursion> { |
There was a problem hiding this comment.
last_expr_was_literal is not reset when visiting a nonliteral. For (lit(1) + col("c1")) + lit(2), the rewrite appears to rebuild the same expression but still results in Transformed::yes
Instead you could make has_adjacent_literals inspect the flattened operand sequence for the same operator, and reset adjacency whenever a nonliteral operand appears.
There was a problem hiding this comment.
I kept the AdjacentLiteralVisitor and changed it so that it updates last_expr_was_literal correctly. is_associative_with_adjacent_literals needs to take the expression by reference because we call it from the big match statement in the Simplifier. I'm not sure I can construct the flattened operand sequence with just a reference to the expression. I suppose I could construct a Vec<&Expr>, but I'm not sure how much better that would be. Unless I could somehow write one function that does &Expr -> Vec<&Expr> and Expr -> Vec<Expr>.
| } | ||
|
|
||
| #[test] | ||
| fn test_simplify_nested_associative_expr() { |
There was a problem hiding this comment.
small: maybe add negative coverage for float addition and for separated literals like (1 + c1) + 2
|
Also I realized that you have to check that the types match because otherwise reassociating the expression can change when casts happen which can change the answer if there is overflow. For example: |
Which issue does this PR close?
Closes #17158
Rationale for this change
The query planner currently does not optimize expressions like
c1 || 'a' || 'b' || c2(which it could simplify it toc1 || 'ab' || c2). This is because theConstEvaluatorlooks forBinaryExprs where both arguments areLiterals. The expression above looks likeso it does not get optimized. Since string concatenation is associative, we can rewrite the expression to look like
which the
ConstEvaluatorcan optimize toWe start by flattening nested occurrences of a the same associative operator into a list. Then we rebuild the expression tree from this list so that adjacent
Literals appear together in the same subtree. This rewrite happens in theexpr_simplifier::Simplifierso that theConstEvaluatorcan do it's work on a subsequent pass.What changes are included in this PR?
This PR implements the
has_adjacent_literalsandreassociate_literalsfunctions.There are some remaining questions:
Are these changes tested?
Yes, but I'll add more tests.
Are there any user-facing changes?
No