Remove recursive const check in simplify_const_expr
#20234
+92
−23
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
simplify_const_exprdoes unnecessary recursive work #20134 .Rationale for this change
The check for simplifying const expressions was recursive and expensive, repeatedly checking the expression's children in a recursive way.
I've tried other approached like pre-computing the result for all expressions outside of the loop and using that cache during the traversal, but I've found that it only yielded between 5-8% improvement while adding complexity, while this approach simplifies the code and seems to be more performant in my benchmarks (change is compared to current main branch):
What changes are included in this PR?
simplify_const_exprnow only checks itself and whether all of its children are literals, because it assumes the order of simplification is bottoms-up.Are these changes tested?
Existing test suite
Are there any user-facing changes?
I suggest removing some of the physical expression simplification code from the public API, which I believe reduces the maintenance burden here. These changes also helps removing code like the distinct
simplify_const_exprandsimplify_const_expr_with_dummy.datafusion-physical-expr::simplifiersub-modules (notandconst_evaluator) private, including their key functions. They are not used externally, and being able to change their behavior seems more valuable long term. The simplifier is also not currently an extension point as far as I can tell, so there's no value in providing atomic building blocks like them for now.has_column_referencescompletely, its trivial to re-implement and isn't used anywhere in the codebase.