Move partition handling out of PhysicalExprAdapter by adriangb · Pull Request #19128 · apache/datafusion

adriangb · 2025-12-06T13:00:37Z

This PR does some refactoring of PhysicalExprAdapter and PhysicalExprSimplifier that I found necessary and/or beneficial while working on #19111.

Changes made

Replace `PhysicalExprAdapter::with_partition_values` with `replace_columns_with_literals`

This is a nice improvement because it:

Makes the PhysicalExprAdapter trait that users might need to implement simpler (less boilerplate for users).
Decouples these two transformations so that we can replace partition values and then apply a projection without having to also do the schema mapping (it would be from the logical schema to the logical schema, confusing and a waste of compute). I ran into this need in Push down projection expressions into ParquetOpener #19111. I think there may be other ways of doing it (e.g. piping in the expected output schema from ParquetSource) but it felt nicer this way and I expect other places may also need the decoupled transformation.
I think we can use it in the future to implement Replace constant columns with literals #19089 (edit: evidently I was right, see identical function in feat: Add constant column extraction and rewriting for projections in ParquetOpener #19136).
It's less lines of code 😄

This will require any users calling PhysicalExprAdapter directly to change their code, I can add an entry to the upgrade guide.

Remove partition pruning logic from `FilePruner` and deprecate now unused `PrunableStatistics` and `CompositePruningStatistics`.

Since we replace partition values with literals we no longer need these structures, they get handled like any other literals.
This is a good chunk of code / complexity that we can bin off.

Use `TableSchema` instead of `SchemaRef` + `Vec<FieldRef>` in `ParquetOpener`

TableSchema is basically SchemaRef + Vec<FieldRef> already and since ParquetSource has a TableSchema its less code and less clones to propagate that into ParquetOpener

adriangb · 2025-12-06T13:33:12Z

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

-        &mut self,
-        expr: Arc<dyn PhysicalExpr>,
-    ) -> Result<Arc<dyn PhysicalExpr>> {
+    pub fn simplify(&self, expr: Arc<dyn PhysicalExpr>) -> Result<Arc<dyn PhysicalExpr>> {


This is backwards compatible (aside from compiler warnings that the variable doesn't need to be mutable) and means we can keep multiple references, arc it, etc.

adriangb · 2025-12-06T17:13:48Z

This is blocked by one of #19129 or #19130, I think either should make it work. Currently some tests fail because we can't prune based on 1 = 2.

As a follow up to this we should be able to delete PartitionPruningStatistics and hence remove CompositePruningStatistics and simplify FilePruner. They'll need to go through the deprecation process, and we'll need to document that you should call replace_columns_with_literals instead.

adriangb · 2025-12-06T22:28:46Z

I've merged #19129 into this branch to get CI passing. I'll fix the conflicts once we merge that.

adriangb · 2025-12-08T14:38:39Z

I've merged #19129 into this branch to get CI passing. I'll fix the conflicts once we merge that.

I've reverted this since #19130 got merged and covers this functionality.

adriangb · 2025-12-08T14:40:03Z

datafusion/physical-expr-adapter/src/schema_rewriter.rs

+/// - `Result<Arc<dyn PhysicalExpr>>`: The rewritten physical expression with columns replaced by literals.
+pub fn replace_columns_with_literals(
+    expr: Arc<dyn PhysicalExpr>,
+    replacements: &HashMap<&str, &ScalarValue>,


Open to accepting HashMap<String, ScalarValue> or something if that makes lifetimes easier

alamb · 2025-12-09T13:30:44Z

Starting to check this one out

alamb

I went through this one carefully and it makes a lot of sense to me -- "Partition Columns" is a higher level concept in my mind than expression rewriting. Thank you @adriangb

Replacing column references with expressions is a better match

While reviewing this PR, I worked some on the documentation and made a PR targeting this one for your contemplation (I can update it to target main if you want to merge this PR as is):

pydantic#47

Finally, should we mark this PR as an API change / update the upgrading.md doc? I can't keep track of what versions these APIs were changed

alamb · 2025-12-09T13:48:41Z

datafusion/physical-expr-adapter/src/schema_rewriter.rs


+/// Replace column references in the given physical expression with literal values.
+///
+/// This is used to substitute partition column references with their literal values during expression rewriting.


Another potential usecase is filling values for columns with default values (rather than NULL)

Great point! I'm going to update datafusion-examples/examples/custom_data_source/default_column_values.rs to use this new method precisely for that.

alamb · 2025-12-09T13:56:36Z

datafusion/physical-expr-adapter/src/schema_rewriter.rs

+    replacements: &HashMap<&str, &ScalarValue>,
+) -> Result<Arc<dyn PhysicalExpr>> {
+    expr.transform(|expr| {
+        if let Some(column) = expr.as_any().downcast_ref::<Column>()


the let chains are quite cool

alamb · 2025-12-09T14:04:09Z

datafusion/datasource-parquet/src/opener.rs

+                .map(|(field, value)| (field.name().as_str(), value))
+                .collect();
+
+            self.predicate


is it worth skipping this clone/rewrite if there are no partition_values (aka if partition_values.is_empty()?)

alamb · 2025-12-09T14:08:39Z

datafusion/datasource-parquet/src/opener.rs

+                    // Make a FilePruner only if there is either a dynamic expr in the predicate or the file has file-level statistics.
+                    // File-level statistics may allow us to prune the file without loading any row groups or metadata.
+                    // If there is a dynamic filter we may be able to prune the file later as the dynamic filter is updated.
+                    // This does allow the case where there is a dynamic filter but no statistics, in which case


It took me a bit to parse this comment

I left a suggestion of how we might be able to improve it

alamb · 2025-12-09T14:17:12Z

datafusion/datasource-parquet/src/opener.rs

                limit: None,
                predicate: Some(predicate),
-                logical_file_schema: schema.clone(),
+                table_schema: TableSchema::from_file_schema(schema.clone()),


Suggested change

table_schema: TableSchema::from_file_schema(schema.clone()),

table_schema: TableSchema::from_file_schema(Arc::clone(schema.clone)),

not sure why clippy didn't catch this

alamb · 2025-12-09T14:17:28Z

datafusion/datasource-parquet/src/opener.rs

                predicate: Some(predicate),
-                logical_file_schema: file_schema.clone(),
+                table_schema: TableSchema::new(
+                    file_schema.clone(),


Suggested change

file_schema.clone(),

Arc::clone(file_schema),

(and several other examples below)

alamb · 2025-12-09T16:32:36Z

datafusion-examples/examples/custom_data_source/default_column_values.rs

-                        return Ok(Transformed::yes(default_literal));
-                    }
-                }
+        // Pre-compute replacements for missing columns with default values


this is great

github-actions bot added core Core DataFusion crate datasource Changes to the datasource crate physical-expr Changes to the physical-expr crates labels Dec 6, 2025

adriangb changed the title ~~Split out partition column handling from PhysicalExprAdapter trait~~ Clean up PhysicalExprAdapter and PhysicalExprSimplifier Dec 6, 2025

adriangb commented Dec 6, 2025

View reviewed changes

github-actions bot added the common Related to common crate label Dec 6, 2025

This was referenced Dec 7, 2025

Support Push down expression evaluation in TableProviders #14993

Closed

feat: Add constant column extraction and rewriting for projections in ParquetOpener #19136

Merged

adriangb force-pushed the move-adapter-partition-col-logic branch from eb5ff93 to 0ec0329 Compare December 8, 2025 14:38

adriangb commented Dec 8, 2025

View reviewed changes

adriangb force-pushed the move-adapter-partition-col-logic branch from 569010f to 1949a45 Compare December 8, 2025 15:27

github-actions bot removed the physical-expr Changes to the physical-expr crates label Dec 8, 2025

adriangb force-pushed the move-adapter-partition-col-logic branch from 387fc6d to c981725 Compare December 8, 2025 20:25

adriangb requested review from alamb and xudong963 December 8, 2025 21:15

adriangb changed the title ~~Clean up PhysicalExprAdapter and PhysicalExprSimplifier~~ Clean up PhysicalExprAdapter Dec 9, 2025

adriangb changed the title ~~Clean up PhysicalExprAdapter~~ Move partition handling out of PhysicalExprAdapter Dec 9, 2025

Move partition handling out of PhysicalExprAdapter

5595f37

adriangb force-pushed the move-adapter-partition-col-logic branch from 6147632 to 5595f37 Compare December 9, 2025 00:32

Add note

6e36bd4

alamb mentioned this pull request Dec 9, 2025

Andrew Lamb Weekly-ish Open Source plan - 2025-12-08 #19210

Closed

40 tasks

alamb mentioned this pull request Dec 9, 2025

Improve physical schema adapter documenation pydantic/datafusion#47

Merged

alamb approved these changes Dec 9, 2025

View reviewed changes

alamb and others added 2 commits December 9, 2025 09:05

Improve physical schema adapter documenation (#47)

2cc259e

update docstring

635e8cb

github-actions bot added the documentation Improvements or additions to documentation label Dec 9, 2025

update test, fix docs

69bdf57

alamb reviewed Dec 9, 2025

View reviewed changes

adriangb removed the request for review from xudong963 December 9, 2025 17:18

adriangb added this pull request to the merge queue Dec 9, 2025

Merged via the queue into apache:main with commit dc78613 Dec 9, 2025
32 checks passed

	table_schema: TableSchema::from_file_schema(schema.clone()),
	table_schema: TableSchema::from_file_schema(Arc::clone(schema.clone)),

Conversation

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

Uh oh!

Changes made

Replace PhysicalExprAdapter::with_partition_values with replace_columns_with_literals

Remove partition pruning logic from FilePruner and deprecate now unused PrunableStatistics and CompositePruningStatistics.

Use TableSchema instead of SchemaRef + Vec<FieldRef> in ParquetOpener

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adriangb commented Dec 6, 2025

Uh oh!

adriangb commented Dec 6, 2025

Uh oh!

adriangb commented Dec 8, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb commented Dec 9, 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!

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

adriangb commented Dec 6, 2025 •

edited

Loading

Replace `PhysicalExprAdapter::with_partition_values` with `replace_columns_with_literals`

Remove partition pruning logic from `FilePruner` and deprecate now unused `PrunableStatistics` and `CompositePruningStatistics`.

Use `TableSchema` instead of `SchemaRef` + `Vec<FieldRef>` in `ParquetOpener`