Optimize PhysicalExprSimplifier#20111
Merged
Dandandan merged 6 commits intoapache:mainfrom Feb 3, 2026
Merged
Conversation
Signed-off-by: Adam Gutglick <adamgsal@gmail.com>
AdamGS
force-pushed
the
adamg/optimize-phyiscal-expr-simplifier
branch
from
d7d40e7 to
8d0e19d
Compare
Contributor
Author
|
64505b5 is a 36% improvement in the benchmark on my laptop (I suspect the macos allocator isn't great) |
Contributor
Author
|
seems like I broke something bigger here, I'll give this PR the attention it deserves later in the week. |
AdamGS
commented
Feb 2, 2026
| @@ -44,13 +44,13 @@ use crate::expressions::{BinaryExpr, InListExpr, Literal, NotExpr, in_list, lit} | |||
| /// TreeNodeRewriter, multiple passes will automatically be applied until no more | |||
| /// transformations are possible. | |||
| pub fn simplify_not_expr( | |||
Contributor
Author
There was a problem hiding this comment.
this is a breaking change
AdamGS
commented
Feb 2, 2026
| } | ||
| Ok(Transformed::no(e)) | ||
| }) | ||
| if let Some(binary) = expr.as_any().downcast_ref::<BinaryExpr>() |
Contributor
Author
There was a problem hiding this comment.
this significantly changes the behavior of the function, we can move the logic into an internal private function and keep this public one, nvm, unlike the others this one is pub(crate)
Contributor
Author
There was a problem hiding this comment.
this is not a recursive function anymore, we control the order externally
AdamGS
commented
Feb 2, 2026
AdamGS
commented
Feb 2, 2026
AdamGS
commented
Feb 2, 2026
| // specific language governing permissions and limitations | ||
| // under the License. | ||
|
|
||
| //! This is an attempt at reproducing some predicates generated by TPC-DS query #76, |
Contributor
Author
There was a problem hiding this comment.
added the benchmark to make it easy for others to use it, it could be evolved over time if we find more usecases people care about
Contributor
Author
|
This PR shows about 56% improvement compared to |
Contributor
|
run benchmarks |
|
🤖 |
Contributor
|
run benchmark tpcds tpch |
AdamGS
changed the title
PhysicalExprSimplifier
Dandandan
reviewed
Feb 2, 2026
| } | ||
| } | ||
|
|
||
| fn can_evaluate_as_constant(expr: &Arc<dyn PhysicalExpr>) -> bool { |
Contributor
There was a problem hiding this comment.
Can we create a ticket to avoid the double recursion here?
Contributor
Author
|
I pushed another check for consts that seems to have really really good performance (from 56% improvement to 97%), but I'm suspicious, trying to figure out what is going on. |
|
🤖: Benchmark completed Details
|
|
🤖 |
AdamGS
commented
Feb 2, 2026
| batch: &RecordBatch, | ||
| ) -> Result<Transformed<Arc<dyn PhysicalExpr>>> { | ||
| // If expr is already a const literal or can't be evaluated into one. | ||
| if expr.as_any().is::<Literal>() || (!can_evaluate_as_constant(&expr)) { |
Contributor
Author
There was a problem hiding this comment.
adding this check for literal value makes a huge difference here, which also what I saw before in #20078, it can also be moved into can_evaluate_as_constant.
I think what happens is that there are trees that used to marked as transformed even though it was just a a literal being evaluated to itself.
Contributor
Author
There was a problem hiding this comment.
@Dandandan I'm convinced this is the biggest win and a significant win here. Right now even something like lit(5) will run through 5 simplification steps, which are completely avoidable. I've also verified it locally.
Contributor
There was a problem hiding this comment.
That makes a lot of sense!
AdamGS
force-pushed
the
adamg/optimize-phyiscal-expr-simplifier
branch
from
2089cd1 to
1999178
Compare
|
🤖: Benchmark completed Details
|
|
🤖 |
|
🤖: Benchmark completed Details
|
AdamGS
commented
Feb 2, 2026
| unwrap_cast::unwrap_cast_in_comparison(node, schema) | ||
| })? | ||
| .transform_data(|node| const_evaluator::simplify_const_expr(&node))?; | ||
| simplify_const_expr_with_dummy(node, &batch) |
Contributor
Author
There was a problem hiding this comment.
I agree, I think we can find the subtrees that do and then just skip the others, thinking through this now but it might have to wait for tomorrow
Dandandan
approved these changes
Feb 3, 2026
Contributor
Author
|
@Dandandan I'll move the last piece related to |
Contributor
|
Thanks a lot! |
Contributor
Author
|
Thank you for the careful review and feedback! Never really touched that part of DataFusion before, so its very cool to be able to get it in so quickly. |
12 tasks
Contributor
|
This is a very nice change |
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
4 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?
Rationale for this change
An attempt at reducing the cost of physical expression simplification
What changes are included in this PR?
lit(5)end up running through the loop 5 times. This takes this PR to ~96% improvement on the benchmark.simplify_const_exprcall.simplify_not_exprandsimplify_const_exprnow take anArcinstead of&ArcAre these changes tested?
All existing tests pass with minor modifications.
Are there any user-facing changes?
Two of the individual recursive simplification functions (
simplify_not_exprandsimplify_const_expr) are public. This PR breaks their signature, but I think we should consider also making them private.