Add PhysicalExpr optimizer and cast unwrapping #16530

adriangb · 2025-06-24T04:33:21Z

Sets us up to address #16004.

I've ported part of the code to a new shared module and reproduced the non-shared functionality for PhysicalExpr, also setting up building blocks to be able to add more simplifications / optimizations (the only one I can think of though is const evaluation and that doesn't seem like a big deal).

alamb · 2025-06-24T21:22:00Z

I will try and find time to review this tomorrow. Thank you @adriangb -- I can't keep up !

adriangb · 2025-06-26T14:05:51Z

1032b5d 👨🏻‍🍳

adriangb · 2025-06-26T14:12:56Z

One unfortunate thing: I can't add the optimizer to datafusion-pruning because datafusion-physical-optimizer depends on datafusion-pruning so it creates a cycle. I want to apply the simplifier inside of PruningPredicate to address #16004 (comment)

adriangb · 2025-06-26T15:01:43Z

One unfortunate thing: I can't add the optimizer to datafusion-pruning because datafusion-physical-optimizer depends on datafusion-pruning so it creates a cycle. I want to apply the simplifier inside of PruningPredicate to address #16004 (comment)

Fixed by moving this to physical-expr/src/simplifier/

alamb · 2025-06-27T21:33:27Z

I am sorry for the delay reviewing this, but there is a lot going on at the moment. We have quite a few PRs merging in main.

I am thinking maybe next week we should target slowing down the new features and start preparing for hardening / bug fixing for the 49 release...

adriangb · 2025-06-27T21:35:44Z

Sounds good to me! This can easily wait until after next week.

alamb

Thank you @adriangb -- I think this PR is a really nice step forward.
I left some suggestions but I don't think any are necessary.

It is so cool to see this progressing towards having a good story for per-file optimized predicates. 🚀

BTW I wonder if we should use this simplifier in the FilterPushdownSimplifier @xudong963 added a few days ago in : #16362

alamb · 2025-06-30T21:53:59Z

datafusion/expr-common/src/casts.rs

+// specific language governing permissions and limitations
+// under the License.
+
+//! Utilities for casting scalar literals to different data types


datafusion/datasource-parquet/src/opener.rs

datafusion/physical-expr/src/simplifier/mod.rs

alamb · 2025-06-30T22:13:25Z

datafusion/physical-expr/src/simplifier/unwrap_cast.rs

+}
+
+/// Swap comparison operators for right-side cast unwrapping
+fn swap_operator(op: Operator) -> Operator {


This could use Operator::swap, right? I don't think there is any reason to have it specially here

alamb · 2025-06-30T22:15:03Z

datafusion/physical-expr/src/simplifier/unwrap_cast.rs

+        extract_cast_info(binary.right()),
+    ) {
+        // For literal op cast(expr), we need to swap the operator
+        let swapped_op = swap_operator(*binary.op());


If we can't swap the operator I think the rewrite should stop (not just unwrap the comparison)

alamb · 2025-06-30T22:18:46Z

datafusion/expr-common/src/casts.rs

+use datafusion_common::ScalarValue;
+
+/// Convert a literal value from one data type to another
+pub fn try_cast_literal_to_type(


This almost seems like it is / should be a method on ScalarValue like ScalarValue::try_cast or something -- might make it more discoverable

We could do this as a follow on PR -- I see you just moved this to a new place

Makes sense! There's already a ScalarValue::cast_to which may do what we want but I'm hesitant to change it in this PR since that might have unintended consequences if the implementations differ. I'd rather do that in it's own PR. I opened #16635 to track.

alamb · 2025-06-30T22:20:54Z

datafusion/physical-expr/src/simplifier/unwrap_cast.rs

+            Arc::new(BinaryExpr::new(cast_expr, Operator::Gt, literal_expr));
+
+        // Apply unwrap cast optimization
+        let result = unwrap_cast_in_comparison(binary_expr, &schema).unwrap();


As a a minor comment it would be nice if we could reduce the boiler plate in these tests -- specifically, you could perhaps factor out the unwrapping and verifying that it was transformed

alamb · 2025-06-30T22:21:28Z

datafusion/physical-expr/src/simplifier/unwrap_cast.rs

+        // Create: cast(c1 as INT64) > INT64(10)
+        let column_expr = col("c1", &schema).unwrap();
+        let cast_expr = Arc::new(CastExpr::new(column_expr, DataType::Int64, None));
+        let literal_expr = lit(ScalarValue::Int64(Some(10)));


I think you can write this (and similar expressions) below much more sucinctly like this:

Suggested change

let literal_expr = lit(ScalarValue::Int64(Some(10)));

let literal_expr = lit(10i64);

alamb · 2025-06-30T22:27:40Z

datafusion/physical-expr/src/simplifier/unwrap_cast.rs

+    fn test_unwrap_cast_with_literal_on_left() {
+        let schema = test_schema();
+
+        // Create: INT64(10) < cast(c1 as INT64)


Can you also please test something where the cast doesn't work? Something with a column that is uint8 for example, and the literal can not fit into the column: `Int64(-5) < CAST(c4 as Int64)

Though I realize in this case the predicate would always be true then 🤔

datafusion/physical-expr/src/simplifier/unwrap_cast.rs

datafusion/pruning/src/pruning_predicate.rs

adriangb · 2025-07-01T05:37:50Z

@alamb thank you for the review. I think I've addressed all of the feedback 🙏🏻

alamb

I reran all the tests again and gave this a final once over and I think it looks great. Thanks again @adriangb

adriangb · 2025-07-02T19:57:41Z

Thanks @alamb!

github-actions bot added logical-expr Logical plan and expressions optimizer Optimizer rules labels Jun 24, 2025

This was referenced Jun 24, 2025

add cast unwraps to parquet predicate rewrites pydantic/datafusion#31

Closed

adapt filter expressions to file schema during parquet scan #16461

Merged

adriangb force-pushed the physical-optimizer branch from 8247e7e to 5c7d5c8 Compare June 24, 2025 13:51

adriangb mentioned this pull request Jun 24, 2025

EPIC: use cp_solver framework to develop a more sophisticated predicate simplification #16511

Open

adriangb force-pushed the physical-optimizer branch from 5c7d5c8 to 1032b5d Compare June 26, 2025 14:05

github-actions bot added the datasource Changes to the datasource crate label Jun 26, 2025

github-actions bot added the physical-expr Changes to the physical-expr crates label Jun 26, 2025

adriangb mentioned this pull request Jun 26, 2025

feature: implement PhysicalExpr const simplifier/evaluator #16567

Open

adriangb force-pushed the physical-optimizer branch from 24162e3 to 3dd706f Compare June 27, 2025 20:02

Add PhysicalExpr optimizer and cast unwrapping

6b2398a

adriangb force-pushed the physical-optimizer branch from 3dd706f to 6b2398a Compare June 27, 2025 20:45

alamb mentioned this pull request Jun 30, 2025

Fix: make datafusion-cli running inconsistent with clickbench benchma… #16599

Closed

alamb approved these changes Jun 30, 2025

View reviewed changes

adriangb mentioned this pull request Jul 1, 2025

Consider merging / replacing try_cast_literal_to_type with ScalarValue::cast_to #16635

Open

address pr feedback

46d0e41

adriangb commented Jul 1, 2025

View reviewed changes

datafusion/pruning/src/pruning_predicate.rs Show resolved Hide resolved

adriangb added 2 commits July 1, 2025 00:27

Update datafusion/pruning/src/pruning_predicate.rs

e57e321

more lit(Xi64)

8cecdfc

alamb approved these changes Jul 2, 2025

View reviewed changes

alamb merged commit 6870cc1 into apache:main Jul 2, 2025
29 checks passed

	let literal_expr = lit(ScalarValue::Int64(Some(10)));
	let literal_expr = lit(10i64);

Add PhysicalExpr optimizer and cast unwrapping #16530

Add PhysicalExpr optimizer and cast unwrapping #16530

Uh oh!

Conversation

adriangb commented Jun 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alamb commented Jun 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adriangb commented Jun 26, 2025

Uh oh!

adriangb commented Jun 26, 2025

Uh oh!

adriangb commented Jun 26, 2025

Uh oh!

alamb commented Jun 27, 2025

Uh oh!

adriangb commented Jun 27, 2025

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

adriangb commented Jul 1, 2025

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

adriangb commented Jul 2, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

adriangb commented Jun 24, 2025 •

edited

Loading

alamb commented Jun 24, 2025 •

edited

Loading