Skip to content

Conversation

@AdamGS
Copy link
Contributor

@AdamGS AdamGS commented Feb 9, 2026

Which issue does this PR close?

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):

tpc-ds/q76/cs/16        time:   [27.112 µs 27.159 µs 27.214 µs]
                        change: [−13.533% −13.167% −12.801%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 7 outliers among 100 measurements (7.00%)
  1 (1.00%) low mild
  4 (4.00%) high mild
  2 (2.00%) high severe

tpc-ds/q76/ws/16        time:   [26.175 µs 26.280 µs 26.394 µs]
                        change: [−14.312% −13.833% −13.346%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 1 outliers among 100 measurements (1.00%)
  1 (1.00%) low mild

tpc-ds/q76/cs/128       time:   [195.79 µs 196.17 µs 196.56 µs]
                        change: [−14.362% −14.080% −13.816%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 5 outliers among 100 measurements (5.00%)
  1 (1.00%) low severe
  1 (1.00%) low mild
  3 (3.00%) high mild

tpc-ds/q76/ws/128       time:   [197.08 µs 197.61 µs 198.23 µs]
                        change: [−13.531% −13.142% −12.737%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 3 outliers among 100 measurements (3.00%)
  1 (1.00%) low mild
  2 (2.00%) high mild

What changes are included in this PR?

  1. simplify_const_expr now only checks itself and whether all of its children are literals, because it assumes the order of simplification is bottoms-up.
  2. Removes some code from the public API, see the last section for the full details.

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_expr and simplify_const_expr_with_dummy.

  1. Makes all datafusion-physical-expr::simplifier sub-modules (not and const_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.
  2. Removes has_column_references completely, its trivial to re-implement and isn't used anywhere in the codebase.

@github-actions github-actions bot added the physical-expr Changes to the physical-expr crates label Feb 9, 2026
@AdamGS
Copy link
Contributor Author

AdamGS commented Feb 9, 2026

(if any maintainer can add the api change label here, that would be appreciated)

@AdamGS AdamGS force-pushed the adamg/issue-20134 branch from 3d7e56b to 0610595 Compare February 9, 2026 11:32
Copy link
Member

@martin-g martin-g left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@martin-g
Copy link
Member

martin-g commented Feb 9, 2026

I suggest removing some of the physical expression simplification code from the public API, which I believe reduces the maintenance burden here.

I do agree with you but please also consider deprecating those functions for a release or two and then remove the pub modifier and the #[deprecated] attribute. This will give any existing users of those APIs time to migrate or explain why they need these APIs.

@AdamGS
Copy link
Contributor Author

AdamGS commented Feb 9, 2026

Of course, will push that first thing tomorrow morning

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

physical-expr Changes to the physical-expr crates

Projects

None yet

Development

Successfully merging this pull request may close these issues.

simplify_const_expr does unnecessary recursive work

2 participants