Remove recursive const check in simplify_const_expr#20234
Merged
alamb merged 3 commits intoapache:mainfrom Feb 24, 2026
Merged
Conversation
Contributor
Author
|
(if any maintainer can add the |
AdamGS
force-pushed
the
adamg/issue-20134
branch
from
3d7e56b to
0610595
Compare
Member
I do agree with you but please also consider deprecating those functions for a release or two and then remove the |
Contributor
Author
|
Of course, will push that first thing tomorrow morning |
alamb
reviewed
Feb 24, 2026
| .transform_data(|node| unwrap_cast_in_comparison(node, schema))? | ||
| .transform_data(|node| { | ||
| simplify_const_expr_with_dummy(node, &batch) | ||
| const_evaluator::simplify_const_expr_immediate(node, &batch) |
Contributor
|
I'll plan to merge this PR in when it passes CI |
Contributor
Contributor
|
run benchmark sql_planner |
|
🤖 |
alamb
reviewed
Feb 24, 2026
| /// that only contain literals, the batch content is irrelevant. | ||
| /// | ||
| /// This is the same approach used in the logical expression `ConstEvaluator`. | ||
| pub(crate) fn create_dummy_batch() -> Result<RecordBatch> { |
Contributor
There was a problem hiding this comment.
While reviewing this I was wondering why we need a dummy batch at all -- I created a follow on PR to create it once and reuse it (should make expression simplification faster)
|
🤖: Benchmark completed Details
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
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.