Implement Parquet filter pushdown via new filter pushdown APIs #15769

adriangb · 2025-04-18T19:57:05Z

Moves predicate pushdown into parquet being something specialized that ListingTable and Parquet to working for any TableProvider and any file format the implements the APIs. The checks for compatibility also happen all within the parquet data source machinery, instead of leaking implementations via supports_filters_pushdown.

adriangb

pointing out current issues to move forward with implementing parquet filter pushdown via the new APIs we've introduced

cc @alamb @berkaysynnada for ideas

adriangb · 2025-04-18T19:58:17Z

datafusion/datasource-parquet/src/file_format.rs

-        Arc::new(ParquetSource::default())
+        todo!() // need access of file schema?


This poses an issue.

TLDR is that in order to know if it can absorb a filter as exact ParquetSource needs to know not only the filter but also the file schema it's applied to (in particular to get the type of the columns since it can't handle structs).

adriangb · 2025-04-18T19:58:40Z

datafusion/datasource-parquet/src/source.rs

+        let remaining_description = if config.execution.parquet.pushdown_filters {
+            let mut remaining_filters = fd.filters.clone();
+            for filter in &remaining_filters {
+                if can_expr_be_pushed_down_with_schemas(filter, &conf.file_schema) {


This is where we need the file schema

alamb · 2025-04-19T11:28:31Z

Thanks @adriangb -- I am about to be offline for a week so I will review this when I return

adriangb · 2025-04-20T01:17:40Z

datafusion/core/src/datasource/listing/table.rs

-                // if we can't push it down completely with only the filename-based/path-based
-                // column names, then we should check if we can do parquet predicate pushdown
-                let supports_pushdown = self.options.format.supports_filters_pushdown(
-                    &self.file_schema,
-                    &self.table_schema,
-                    &[filter],
-                )?;
-
-                if supports_pushdown == FilePushdownSupport::Supported {
-                    return Ok(TableProviderFilterPushDown::Exact);
-                }


The point of this PR is that this moves from being something specialized that ListingTable does to anything that works for any TableProvider / they don't need to do anything special! The checks for compatibility also happen all within the parquet data source machinery, instead of leaking implementations via supports_filters_pushdown.

I have one question: aren't we expecting/preparing for, people to use ListingTable if they read Parquet files? Are we eventually planning to remove all format-specific handlings? Or this is a case only for filter pushdown?

If that's the case, why don't we fully remove supports_filters_pushdown() API at all

I think many users of DataFusion (based on our usage, talks I've seen and examples we have) use custom TableProvider implementations.

I would keep supports_filters_pushdown so that TableProviders can do Exact pruning of filters, e.g. using partition columns.

We can justify implementing other TableProviders for Parquet, but still I cannot understand why we need to degrade the capabilities of our ListingTable. Is't it always better pruning/simplifying things at the higher levels as possible?

I have one question: aren't we expecting/preparing for, people to use ListingTable if they read Parquet files? Are we eventually planning to remove all format-specific handlings? Or this is a case only for filter pushdown?

For what it is worth, we (InfluxData) doesn't use ListingTable to read parquet files, instead we provide our own equivalent and create the DataSourceExec's directly

I would keep supports_filters_pushdown so that TableProviders can do Exact pruning of filters, e.g. using partition columns.

Yes I think that is important too -- I don't think we should be removing any APIs from ListingTable

We can justify implementing other TableProviders for Parquet, but still I cannot understand why we need to degrade the capabilities of our ListingTable. Is't it always better pruning/simplifying things at the higher levels as possible?

I don't think this degrades the capabilities of the current listing table. I think the only implications are for anyone who used a custom FileFormat and impleented supports_filters_pushdown -- I suspect this is not very common and we can likely avoid consternation by mentioning it in the upgrade guide (see comment below)

adriangb · 2025-04-20T01:19:46Z

datafusion/core/src/datasource/physical_plan/parquet.rs

-                source = source.with_predicate(Arc::clone(&file_schema), predicate);
+                source = source.with_predicate(predicate);


This seemed like an easy win since I was able to just change this so that the schema is always passed in by the FileSourceConfigBuilder instead of only when with_predicate is called.
This was necessary becasue with_predicate is no longer called to attach a predicate, instaed it happens during an optimization pass so ParquetSource neesd to have it available at that point.
I left with_predicate in there to avoid churn and in case there is a use case for attaching a predicate directly through the scan instad of a as a FilterExec that later gets pushed into the scan.

adriangb · 2025-04-20T01:21:30Z

datafusion/datasource-parquet/src/mod.rs

@@ -244,7 +242,7 @@ impl ParquetExecBuilder {
            inner: DataSourceExec::new(Arc::new(base_config.clone())),
            base_config,
            predicate,
-            pruning_predicate: parquet.pruning_predicate,
+            pruning_predicate: None, // for backwards compat since `ParquetExec` is only for backwards compat anyway


Open to other suggestions (i.e. removing it). I felt like this minimizes breakage for folks still using ParquetExec, who are likely the same folks that want to do the least amount of work possible to upgrade.

yeah this is fine too in my opinion. It is almost time to remove ParquetExec anyways -- maybe we should just do it in this release 🤔

adriangb · 2025-04-20T01:22:44Z

datafusion/datasource-parquet/src/row_filter.rs

-        let table_schema = get_basic_table_schema();
-
-        let file_schema = Schema::new(vec![Field::new(
-            "list_col",
-            DataType::Struct(Fields::empty()),
-            true,
-        )]);


This test was wrong! It wanted to test that list_col prevents pushdown because it's a nested type. Instead it was prevented because list_col is not in the table / schema!

adriangb · 2025-04-20T01:26:18Z

datafusion/datasource-parquet/src/source.rs

-                let pruning_predicate_string = self
-                    .pruning_predicate
-                    .as_ref()
-                    .map(|pre| {
-                        let mut guarantees = pre
-                            .literal_guarantees()
-                            .iter()
-                            .map(|item| format!("{}", item))
-                            .collect_vec();
-                        guarantees.sort();
-                        format!(
-                            ", pruning_predicate={}, required_guarantees=[{}]",
-                            pre.predicate_expr(),
-                            guarantees.join(", ")
-                        )
-                    })
-                    .unwrap_or_default();


In #15561 (review) Andrew asked me to keep this, but now since the schema isn't even being passed in to with_predicate it's going to be hard to keep these. I suggest we just accept that they won't be present in the physical plans. If that's not okay what I could do is generate them on the fly in fmt_extra or generate them if with_predicate is called with a schema or with_schema is called with a predicate. But I'd like to avoid that unless someone thinks is worth it or has another suggestion.

I think it is important to keep these in the physical plans -- in particular what I think is important is to be able to check via the explain plan if pruning is happening by looking at the explain plan

Hmm okay. I'll see if I can make it happen...

adriangb · 2025-04-20T01:26:31Z

datafusion/datasource-parquet/src/source.rs

@@ -587,4 +560,49 @@ impl FileSource for ParquetSource {
            }
        }
    }
+
+    fn try_pushdown_filters(


cc @berkaysynnada for this implementation

adriangb · 2025-04-20T01:26:59Z

datafusion/datasource/src/file_format.rs

-    /// Check if the specified file format has support for pushing down the provided filters within
-    /// the given schemas. Added initially to support the Parquet file format's ability to do this.
-    fn supports_filters_pushdown(


Binning specialized code that was also leaking parquet stuff through DataSource and into TableProvider 😄

yes I agree

Since FileFormat is a pub trait, this is technically a breaking API change, but I do think it was a parquet specific optimization

I recommend we mark this PR as an API change and add a note to the upgrade guide https://github.com/apache/datafusion/blob/main/docs/source/library-user-guide/upgrading.md

I think it should basically say if you implemented FileFormat (which probably no one did) and ListingTable you will have to implement the newly added ExecutionPlan::try_pushdown_filter method into your execution plan directly if you want the filters to be pushed down

adriangb · 2025-04-20T01:27:14Z

datafusion/datasource/src/file_format.rs

-/// An enum to distinguish between different states when determining if certain filters can be
-/// pushed down to file scanning
-#[derive(Debug, PartialEq)]
-pub enum FilePushdownSupport {


Another one of these enums!

adriangb · 2025-04-20T01:29:00Z

datafusion/sqllogictest/test_files/parquet_filter_pushdown.slt

-02)--TableScan: t_pushdown projection=[a], full_filters=[t_pushdown.b > Int32(2)]
+02)--Projection: t_pushdown.a
+03)----Filter: t_pushdown.b > Int32(2)
+04)------TableScan: t_pushdown projection=[a, b], partial_filters=[t_pushdown.b > Int32(2)]


This is because the pushdown no longer happens at the logical level - it happens at the physical level. This makes sense, in part because the checks for suitability of pushdown are better at the physical level (there may be reasons to reject a pushdown at the physical level that are not present at the logical level, e.g. partition columns or encodings).

I think it makes sense and is ok that the logical plans show the filter not pushed down

adriangb · 2025-04-20T01:29:32Z

datafusion/sqllogictest/test_files/parquet_filter_pushdown.slt

-03)----DataSourceExec: file_groups={2 groups: [[WORKSPACE_ROOT/datafusion/sqllogictest/test_files/scratch/parquet_filter_pushdown/parquet_table/1.parquet], [WORKSPACE_ROOT/datafusion/sqllogictest/test_files/scratch/parquet_filter_pushdown/parquet_table/2.parquet]]}, projection=[a], file_type=parquet, predicate=b@1 > 2, pruning_predicate=b_null_count@1 != row_count@2 AND b_max@0 > 2, required_guarantees=[]
+03)----CoalesceBatchesExec: target_batch_size=8192
+04)------RepartitionExec: partitioning=RoundRobinBatch(4), input_partitions=2
+05)--------DataSourceExec: file_groups={2 groups: [[WORKSPACE_ROOT/datafusion/sqllogictest/test_files/scratch/parquet_filter_pushdown/parquet_table/1.parquet], [WORKSPACE_ROOT/datafusion/sqllogictest/test_files/scratch/parquet_filter_pushdown/parquet_table/2.parquet]]}, projection=[a], file_type=parquet, predicate=b@1 > 2 AND b@1 > 2


@berkaysynnada any idea why we have extra CoalesceBatchesExec and RepartitionExec now?

I've a guess but not proved: CoalesceBatchesExec comes because of RepartitionExec, and RepartitionExec is inserted to satisfy partition count, which is 4. That's required by FilterExec now (which was pushed down at the logical level before), but that FilterExec is pushed down later after EnforceDistribution.

So, this makes me think about the correct order of physical rules. PushdownFilter should probably work before distribution&order satisfiers. But that could also bring some issues, I'm not sure.

PushdownFilter should probably work before distribution&order satisfiers

That makes sense to me. It does more "invasive" re-arranging of plans than those do.

I agree it is important to remove the coalesce / repartition

Maybe we can make a separate PR to move the filter pushdown code earlier in the physical planning

An alternate could be to update the filter pushdown optimizer pass somehow to remove these -- but I think it would be cleaner / easier to understand if they were never added in the first place

I opened #15938

datafusion/sqllogictest/test_files/aggregate.slt

adriangb · 2025-04-20T01:56:47Z

Thanks @adriangb -- I am about to be offline for a week so I will review this when I return

Enjoy your vacation! I think you'll like this diff:

berkaysynnada

Thank you @adriangb. I couldn't provide much design suggestions, since I cannot fully understand the need of these changes. If you provide more background information, I can help more maybe.

It seems there are some critical planning changes here, and it's better getting approvals by more people for this PR.

berkaysynnada · 2025-04-21T10:34:49Z

datafusion/datasource-parquet/src/source.rs

+        };
+        let config_pushdown_enabled = config.execution.parquet.pushdown_filters;
+        let table_pushdown_enabled = self.pushdown_filters();
+        if table_pushdown_enabled || config_pushdown_enabled {


OR'ing this is correct?

The current behavior is not documented anywhere, I tried to match the existing tests:

datafusion/datafusion/sqllogictest/test_files/parquet_filter_pushdown.slt

Lines 44 to 88 in 9730404

# pushdown_filters (currently) defaults to false, but we set it here to be explicit

statement ok

set datafusion.execution.parquet.pushdown_filters = false;

statement ok

CREATE EXTERNAL TABLE t(a varchar, b int, c float) STORED AS PARQUET

LOCATION 'test_files/scratch/parquet_filter_pushdown/parquet_table/';

## Create table with pushdown enabled (pushdown setting is part of the table)

statement ok

set datafusion.execution.parquet.pushdown_filters = true;

## Create table without pushdown

statement ok

CREATE EXTERNAL TABLE t_pushdown(a varchar, b int, c float) STORED AS PARQUET

LOCATION 'test_files/scratch/parquet_filter_pushdown/parquet_table/';

# restore defaults

statement ok

set datafusion.execution.parquet.pushdown_filters = false;

# When filter pushdown is not enabled, ParquetExec only filters based on

# metadata, so a FilterExec is required to filter the

# output of the `ParquetExec`

query T

select a from t where b > 2 ORDER BY a;

----

baz

foo

NULL

NULL

NULL

query TT

EXPLAIN select a from t_pushdown where b > 2 ORDER BY a;

----

logical_plan

01)Sort: t_pushdown.a ASC NULLS LAST

02)--TableScan: t_pushdown projection=[a], full_filters=[t_pushdown.b > Int32(2)]

physical_plan

01)SortPreservingMergeExec: [a@0 ASC NULLS LAST]

02)--SortExec: expr=[a@0 ASC NULLS LAST], preserve_partitioning=[true]

03)----DataSourceExec: file_groups={2 groups: [[WORKSPACE_ROOT/datafusion/sqllogictest/test_files/scratch/parquet_filter_pushdown/parquet_table/1.parquet], [WORKSPACE_ROOT/datafusion/sqllogictest/test_files/scratch/parquet_filter_pushdown/parquet_table/2.parquet]]}, projection=[a], file_type=parquet, predicate=b@1 > 2, pruning_predicate=b_null_count@1 != row_count@2 AND b_max@0 > 2, required_guarantees=[]

berkaysynnada · 2025-04-21T10:38:19Z

datafusion/datasource-parquet/src/source.rs

+            let mut conf = self.clone();
+            let mut allowed_filters = vec![];
+            let mut remaining_filters = vec![];
+            for filter in &fd.filters {


fd.take_filters() to avoid clone's below

berkaysynnada · 2025-04-21T10:39:19Z

datafusion/datasource-parquet/src/source.rs

+        fd: FilterDescription,
+        config: &datafusion_common::config::ConfigOptions,
+    ) -> datafusion_common::Result<FilterPushdownResult<Arc<dyn FileSource>>> {
+        let Some(file_schema) = self.file_schema.clone() else {


I'm asking to learn: in which cases ParquetSource doesn't have the schema?

I think they always end up with a schema now, but the current APIs don't require it via the constructor and instead it gets passed in via FileScanConfigBuilder. I tried piping it into the constructor but makes things difficult, there's APIs that rely on ParquetSource::default() and such. So TLDR is it's a bit gross but this is the least chrun way to do it and we can always come back later and clean the rest up.

maybe we file a follow on ticket

datafusion/datasource-parquet/src/source.rs

berkaysynnada · 2025-04-21T10:48:11Z

datafusion/core/src/datasource/listing/table.rs

-                // if we can't push it down completely with only the filename-based/path-based
-                // column names, then we should check if we can do parquet predicate pushdown
-                let supports_pushdown = self.options.format.supports_filters_pushdown(
-                    &self.file_schema,
-                    &self.table_schema,
-                    &[filter],
-                )?;
-
-                if supports_pushdown == FilePushdownSupport::Supported {
-                    return Ok(TableProviderFilterPushDown::Exact);
-                }


I have one question: aren't we expecting/preparing for, people to use ListingTable if they read Parquet files? Are we eventually planning to remove all format-specific handlings? Or this is a case only for filter pushdown?

berkaysynnada · 2025-04-21T10:51:34Z

datafusion/core/src/datasource/listing/table.rs

-                // if we can't push it down completely with only the filename-based/path-based
-                // column names, then we should check if we can do parquet predicate pushdown
-                let supports_pushdown = self.options.format.supports_filters_pushdown(
-                    &self.file_schema,
-                    &self.table_schema,
-                    &[filter],
-                )?;
-
-                if supports_pushdown == FilePushdownSupport::Supported {
-                    return Ok(TableProviderFilterPushDown::Exact);
-                }


If that's the case, why don't we fully remove supports_filters_pushdown() API at all

berkaysynnada · 2025-04-21T11:00:10Z

datafusion/datasource-parquet/src/source.rs

    /// Optional predicate for row filtering during parquet scan
    pub(crate) predicate: Option<Arc<dyn PhysicalExpr>>,
-    /// Optional predicate for pruning row groups (derived from `predicate`)


good to see these are unifying

berkaysynnada · 2025-04-21T11:02:35Z

datafusion/datasource-parquet/src/source.rs

+    /// The schema of the file.
+    /// In particular, this is the schema of the table without partition columns,
+    /// *not* the physical schema of the file.
+    pub(crate) file_schema: Option<SchemaRef>,


There is also another schema in FileScanConfig. Are they both reflects the file schema, not physical schema? and can we somehow unify them?

This is the same schema that FileScanConfig passes into ParquetSource

datafusion/sqllogictest/test_files/aggregate.slt

berkaysynnada · 2025-04-21T11:39:58Z

datafusion/sqllogictest/test_files/parquet_filter_pushdown.slt

-03)----DataSourceExec: file_groups={2 groups: [[WORKSPACE_ROOT/datafusion/sqllogictest/test_files/scratch/parquet_filter_pushdown/parquet_table/1.parquet], [WORKSPACE_ROOT/datafusion/sqllogictest/test_files/scratch/parquet_filter_pushdown/parquet_table/2.parquet]]}, projection=[a], file_type=parquet, predicate=b@1 > 2, pruning_predicate=b_null_count@1 != row_count@2 AND b_max@0 > 2, required_guarantees=[]
+03)----CoalesceBatchesExec: target_batch_size=8192
+04)------RepartitionExec: partitioning=RoundRobinBatch(4), input_partitions=2
+05)--------DataSourceExec: file_groups={2 groups: [[WORKSPACE_ROOT/datafusion/sqllogictest/test_files/scratch/parquet_filter_pushdown/parquet_table/1.parquet], [WORKSPACE_ROOT/datafusion/sqllogictest/test_files/scratch/parquet_filter_pushdown/parquet_table/2.parquet]]}, projection=[a], file_type=parquet, predicate=b@1 > 2 AND b@1 > 2


I've a guess but not proved: CoalesceBatchesExec comes because of RepartitionExec, and RepartitionExec is inserted to satisfy partition count, which is 4. That's required by FilterExec now (which was pushed down at the logical level before), but that FilterExec is pushed down later after EnforceDistribution.

So, this makes me think about the correct order of physical rules. PushdownFilter should probably work before distribution&order satisfiers. But that could also bring some issues, I'm not sure.

adriangb · 2025-04-22T17:59:08Z

#15812 surfaced another reason why building the predicates from the files schemas is necessary. I think once we merge this we can tackle that.

berkaysynnada

There are good things here, but the main change doesn't seem correct to me. Why are we reducing the capabilities of logical optimizations? I think these planning changes will harm some people. How does it block the dynamic filtering approach?

berkaysynnada · 2025-04-24T11:33:29Z

datafusion/core/src/datasource/listing/table.rs

-                // if we can't push it down completely with only the filename-based/path-based
-                // column names, then we should check if we can do parquet predicate pushdown
-                let supports_pushdown = self.options.format.supports_filters_pushdown(
-                    &self.file_schema,
-                    &self.table_schema,
-                    &[filter],
-                )?;
-
-                if supports_pushdown == FilePushdownSupport::Supported {
-                    return Ok(TableProviderFilterPushDown::Exact);
-                }


We can justify implementing other TableProviders for Parquet, but still I cannot understand why we need to degrade the capabilities of our ListingTable. Is't it always better pruning/simplifying things at the higher levels as possible?

adriangb · 2025-04-24T14:06:39Z

I'm not really sure how this degrades anything. The end result is the same, users won't see any difference.

What ListingTable does currently is misguided and wrong since it is not really at the logical level as you say, instead it pierces the logical / physical separation (see how it converts Expr to PhysicalExpr, etc). It even produces bugs (I believe the pushdown of struct fields may currently be broken, or at least the implementation is confusing and the test is completely wrong).

I think there is pushdown that makes sense at the logical level, namely partition pruning. And I left that for TableProvider to continue to do. But the pruning that relies on a PhysicalExpr seems to me like it should be happening at the physical layer not the logical. It kinda gets away with it because it's the last thing that happens at the logical layer I think, but it's still smelly.

We might be able to leave the stuff in TableProvider in place but we'll be dealing with duplication and confusing methods on DataSource, which is already a complex bit of code. When I first tried to implement it this way I ran into cases with duplicate pushdown and other confusing scenarios. Probably it could have been resolved but I felt like why make one of the most complex bits in DataFusion even more complex instead of simplifying it where possible.

berkaysynnada · 2025-04-24T16:40:53Z

I'm not really sure how this degrades anything. The end result is the same, users won't see any difference.

Logical planning results are changing. We are also using DF end-to-end, but there could always be people only relying on logical plans of DF.

We might be able to leave the stuff in TableProvider in place but we'll be dealing with duplication and confusing methods on DataSource, which is already a complex bit of code. When I first tried to implement it this way I ran into cases with duplicate pushdown and other confusing scenarios. Probably it could have been resolved but I felt like why make one of the most complex bits in DataFusion even more complex instead of simplifying it where possible.

I'm also challenging to decide to be which side because of that complexity :D

I will take a look to other parts in this PR, and try to find a solution for the points like https://github.com/apache/datafusion/pull/15769/files#r2051612926. Maybe there isn't something wrong at all as you said, there is no harm to double check

Unlike the other PRs in this work, we might be touching some core components here. So, having a few more people review and approval would make us feel more confident.

adriangb · 2025-04-24T16:51:56Z

I do think you make a good point of "can we keep the current thing and add the new one". It's worth a shot, at least to split the PR into two. And if that's too complicated or if we just want to simplify we can evaluate from there.

alamb

Thank you @adriangb -- this change makes sense to me and I think is an improvement

The code that is moved was used to avoid adding a FilterExec when the table provider would be able to do exact filter pushdown

I am 1/2 the way through the review of this PR -- I hope to finish up shortly

alamb · 2025-05-01T12:55:39Z

datafusion/core/src/datasource/listing/table.rs

-                // if we can't push it down completely with only the filename-based/path-based
-                // column names, then we should check if we can do parquet predicate pushdown
-                let supports_pushdown = self.options.format.supports_filters_pushdown(
-                    &self.file_schema,
-                    &self.table_schema,
-                    &[filter],
-                )?;
-
-                if supports_pushdown == FilePushdownSupport::Supported {
-                    return Ok(TableProviderFilterPushDown::Exact);
-                }


I have one question: aren't we expecting/preparing for, people to use ListingTable if they read Parquet files? Are we eventually planning to remove all format-specific handlings? Or this is a case only for filter pushdown?

For what it is worth, we (InfluxData) doesn't use ListingTable to read parquet files, instead we provide our own equivalent and create the DataSourceExec's directly

I would keep supports_filters_pushdown so that TableProviders can do Exact pruning of filters, e.g. using partition columns.

Yes I think that is important too -- I don't think we should be removing any APIs from ListingTable

alamb · 2025-05-01T13:00:30Z

datafusion/core/src/datasource/listing/table.rs

@@ -982,18 +980,6 @@ impl TableProvider for ListingTable {
                    return Ok(TableProviderFilterPushDown::Exact);
                }

-                // if we can't push it down completely with only the filename-based/path-based


This change makes sense to me -- when @itsjunetime originally implemented this code, there was some complexity because there was no way to do filter pushdown in ExecutionPlans so in my mind this approach was a (clever) workaround

The comments even hint that this is a parquet specific special case

I think the new pattern of handling predicates more generally in this PR is cleaner and will support more cases. Since this code is only currently executed

Perhaps @cisaacson has some other thoughts

alamb · 2025-05-01T13:05:38Z

datafusion/datasource/src/file_format.rs

-    /// Check if the specified file format has support for pushing down the provided filters within
-    /// the given schemas. Added initially to support the Parquet file format's ability to do this.
-    fn supports_filters_pushdown(


yes I agree

Since FileFormat is a pub trait, this is technically a breaking API change, but I do think it was a parquet specific optimization

I recommend we mark this PR as an API change and add a note to the upgrade guide https://github.com/apache/datafusion/blob/main/docs/source/library-user-guide/upgrading.md

I think it should basically say if you implemented FileFormat (which probably no one did) and ListingTable you will have to implement the newly added ExecutionPlan::try_pushdown_filter method into your execution plan directly if you want the filters to be pushed down

alamb · 2025-05-01T13:09:47Z

datafusion/core/src/datasource/listing/table.rs

-                // if we can't push it down completely with only the filename-based/path-based
-                // column names, then we should check if we can do parquet predicate pushdown
-                let supports_pushdown = self.options.format.supports_filters_pushdown(
-                    &self.file_schema,
-                    &self.table_schema,
-                    &[filter],
-                )?;
-
-                if supports_pushdown == FilePushdownSupport::Supported {
-                    return Ok(TableProviderFilterPushDown::Exact);
-                }


We can justify implementing other TableProviders for Parquet, but still I cannot understand why we need to degrade the capabilities of our ListingTable. Is't it always better pruning/simplifying things at the higher levels as possible?

I don't think this degrades the capabilities of the current listing table. I think the only implications are for anyone who used a custom FileFormat and impleented supports_filters_pushdown -- I suspect this is not very common and we can likely avoid consternation by mentioning it in the upgrade guide (see comment below)

alamb · 2025-05-01T13:11:16Z

datafusion/datasource-parquet/src/mod.rs

@@ -244,7 +242,7 @@ impl ParquetExecBuilder {
            inner: DataSourceExec::new(Arc::new(base_config.clone())),
            base_config,
            predicate,
-            pruning_predicate: parquet.pruning_predicate,
+            pruning_predicate: None, // for backwards compat since `ParquetExec` is only for backwards compat anyway


yeah this is fine too in my opinion. It is almost time to remove ParquetExec anyways -- maybe we should just do it in this release 🤔

alamb · 2025-05-01T13:17:11Z

datafusion/datasource-parquet/src/source.rs

-                let pruning_predicate_string = self
-                    .pruning_predicate
-                    .as_ref()
-                    .map(|pre| {
-                        let mut guarantees = pre
-                            .literal_guarantees()
-                            .iter()
-                            .map(|item| format!("{}", item))
-                            .collect_vec();
-                        guarantees.sort();
-                        format!(
-                            ", pruning_predicate={}, required_guarantees=[{}]",
-                            pre.predicate_expr(),
-                            guarantees.join(", ")
-                        )
-                    })
-                    .unwrap_or_default();


I think it is important to keep these in the physical plans -- in particular what I think is important is to be able to check via the explain plan if pruning is happening by looking at the explain plan

clicked wrong button

alamb

Ok, I went through this PR and TLDR is I think it is an improvement. Thank you very much @adriangb

I left several suggestions on how to improve it / the tests, but I also think we could do that as a follow on PR.

I think this is a really nice step forward -- and while it is taking a long time I am confident it will be worth it in the end

alamb · 2025-05-01T15:19:52Z

datafusion/datasource-parquet/src/source.rs

+        fd: FilterDescription,
+        config: &datafusion_common::config::ConfigOptions,
+    ) -> datafusion_common::Result<FilterPushdownResult<Arc<dyn FileSource>>> {
+        let Some(file_schema) = self.file_schema.clone() else {


maybe we file a follow on ticket

alamb · 2025-05-01T21:01:01Z

datafusion/sqllogictest/test_files/parquet_filter_pushdown.slt

-02)--TableScan: t_pushdown projection=[a], full_filters=[t_pushdown.b > Int32(2)]
+02)--Projection: t_pushdown.a
+03)----Filter: t_pushdown.b > Int32(2)
+04)------TableScan: t_pushdown projection=[a, b], partial_filters=[t_pushdown.b > Int32(2)]


I think it makes sense and is ok that the logical plans show the filter not pushed down

alamb · 2025-05-01T21:01:39Z

datafusion/sqllogictest/test_files/parquet_filter_pushdown.slt

-03)----DataSourceExec: file_groups={2 groups: [[WORKSPACE_ROOT/datafusion/sqllogictest/test_files/scratch/parquet_filter_pushdown/parquet_table/1.parquet], [WORKSPACE_ROOT/datafusion/sqllogictest/test_files/scratch/parquet_filter_pushdown/parquet_table/2.parquet]]}, projection=[a], file_type=parquet, predicate=b@1 > 2, pruning_predicate=b_null_count@1 != row_count@2 AND b_max@0 > 2, required_guarantees=[]
+03)----CoalesceBatchesExec: target_batch_size=8192
+04)------RepartitionExec: partitioning=RoundRobinBatch(4), input_partitions=2
+05)--------DataSourceExec: file_groups={2 groups: [[WORKSPACE_ROOT/datafusion/sqllogictest/test_files/scratch/parquet_filter_pushdown/parquet_table/1.parquet], [WORKSPACE_ROOT/datafusion/sqllogictest/test_files/scratch/parquet_filter_pushdown/parquet_table/2.parquet]]}, projection=[a], file_type=parquet, predicate=b@1 > 2 AND b@1 > 2


I agree it is important to remove the coalesce / repartition

Maybe we can make a separate PR to move the filter pushdown code earlier in the physical planning

An alternate could be to update the filter pushdown optimizer pass somehow to remove these -- but I think it would be cleaner / easier to understand if they were never added in the first place

alamb · 2025-05-01T21:04:09Z

datafusion/sqllogictest/test_files/push_down_filter.slt

-logical_plan TableScan: t projection=[a], full_filters=[t.a != Int32(100)]
+logical_plan
+01)Filter: t.a != Int32(100)
+02)--TableScan: t projection=[a], partial_filters=[t.a != Int32(100)]


I recommend we change these tests to show the physical plans (not the logical plans) as that would more accurately show the pushdown happening. Maybe also something we could do as a separate PR

@alamb do you know why these don't display the phyiscal plan already? Is something parsing them out?

berkaysynnada · 2025-05-03T13:35:25Z

@adriangb do you have time to address the last suggestions? I understand the mistake here, and I think we should take this in asap

adriangb · 2025-05-03T15:07:48Z

@adriangb do you have time to address the last suggestions? I understand the mistake here, and I think we should take this in asap

I am going to try to address the last round of review later today on a flight. In particular:

Implement Parquet filter pushdown via new filter pushdown APIs #15769 (comment): change the relevant tests to show the physical plan
Implement Parquet filter pushdown via new filter pushdown APIs #15769 (comment): try to show a predicate in explain plans (it will not exactly match the final predicate though since the latter will be generated based on the physical file schema)

Is there anything I'm missing? Is this what you meant by the mistake?

berkaysynnada · 2025-05-03T15:18:59Z

Is there anything I'm missing? Is this what you meant by the mistake?

I meant the need of this PR.

Is there anything I'm missing?
#15769 (comment)

a ticket would be good IMO as well
#15769 (comment)

do you wanna give it a try to change the orders of rules?

adriangb · 2025-05-03T15:45:44Z

If you can make a PR to change the order of the rules and open that ticket I would appreciate it I won't be able to for several hours 🙏

adriangb · 2025-05-04T10:23:22Z

I updated the order of the pushdown rules in this PR, it worked to get rid of the extra nodes.

I've also added the upgrade guide and the pushdown preview is being shown in the physical plans.

@alamb I think the only point missing is #15769 (comment) which I need a big of guidance on

github-actions bot added core Core DataFusion crate datasource Changes to the datasource crate labels Apr 18, 2025

adriangb commented Apr 18, 2025

View reviewed changes

adriangb mentioned this pull request Apr 18, 2025

TopK dynamic filter pushdown attempt 2 #15770

Open

adriangb changed the title ~~re-implement filter pushdown for parquet~~ Implement filter pushdown for TopK Apr 19, 2025

adriangb changed the title ~~Implement filter pushdown for TopK~~ re-implement filter pushdown for parquet Apr 19, 2025

github-actions bot added the proto Related to proto crate label Apr 19, 2025

adriangb changed the title ~~re-implement filter pushdown for parquet~~ Implement Parquet filter pushdown via new filter pushdown APIs Apr 19, 2025

github-actions bot added the sqllogictest SQL Logic Tests (.slt) label Apr 19, 2025

adriangb commented Apr 20, 2025

View reviewed changes

adriangb marked this pull request as ready for review April 20, 2025 01:30

adriangb force-pushed the parquet-filter-pushdown branch from 071aa19 to ff090a7 Compare April 20, 2025 01:31

adriangb commented Apr 20, 2025

View reviewed changes

datafusion/sqllogictest/test_files/aggregate.slt Outdated Show resolved Hide resolved

berkaysynnada reviewed Apr 21, 2025

View reviewed changes

adriangb mentioned this pull request Apr 22, 2025

Pruning of floating point Parquet columns is incorrect when NaN is present #15812

Open

adriangb force-pushed the parquet-filter-pushdown branch from 1af7766 to 3fde445 Compare April 22, 2025 20:53

berkaysynnada reviewed Apr 24, 2025

View reviewed changes

alamb mentioned this pull request Apr 29, 2025

Weekly Plan: Andrew Lamb 2025-04-28 #15880

Open

26 tasks

alamb previously approved these changes May 1, 2025

View reviewed changes

alamb approved these changes May 1, 2025

View reviewed changes

adriangb mentioned this pull request May 3, 2025

fix query results for predicates referencing partition columns and data columns #15935

Open

adriangb mentioned this pull request May 4, 2025

Move physical plan filter pushdown optimizer rule to avoid adding unnecessary nodes #15938

Open

adriangb added 20 commits May 4, 2025 02:09

re-implement filter pushdown for parquet

68e7f71

resolve

9090ded

tweak

4bcfe5d

fix tests

4f2d395

update

5766582

revert unecessary slt updates

2a0ebf5

fix lint

7901254

re-generate; respect table option

c8741f4

add order by

2d9b8f7

make function private

d340c9c

Add note about schema

fa57ba0

remove more code

4d46d26

fix test

fb18f89

avoid clone

ea16326

rename var

dabaf18

include predicate in explain plans

0da08b5

add upgrade note

eac1380

remove mutable reference

158e4aa

fix test, lint

127c822

fmt

cc8ae9a

adriangb force-pushed the parquet-filter-pushdown branch from 47b03d9 to cc8ae9a Compare May 4, 2025 07:14

github-actions bot added the documentation Improvements or additions to documentation label May 4, 2025

move filter order

e9cb59b

github-actions bot added the optimizer Optimizer rules label May 4, 2025

		Arc::new(ParquetSource::default())
		todo!() // need access of file schema?

		source = source.with_predicate(Arc::clone(&file_schema), predicate);
		source = source.with_predicate(predicate);

	# pushdown_filters (currently) defaults to false, but we set it here to be explicit
	statement ok
	set datafusion.execution.parquet.pushdown_filters = false;

	statement ok
	CREATE EXTERNAL TABLE t(a varchar, b int, c float) STORED AS PARQUET
	LOCATION 'test_files/scratch/parquet_filter_pushdown/parquet_table/';

	## Create table with pushdown enabled (pushdown setting is part of the table)

	statement ok
	set datafusion.execution.parquet.pushdown_filters = true;

	## Create table without pushdown
	statement ok
	CREATE EXTERNAL TABLE t_pushdown(a varchar, b int, c float) STORED AS PARQUET
	LOCATION 'test_files/scratch/parquet_filter_pushdown/parquet_table/';

	# restore defaults
	statement ok
	set datafusion.execution.parquet.pushdown_filters = false;

	# When filter pushdown is not enabled, ParquetExec only filters based on
	# metadata, so a FilterExec is required to filter the
	# output of the `ParquetExec`

	query T
	select a from t where b > 2 ORDER BY a;
	----
	baz
	foo
	NULL
	NULL
	NULL

	query TT
	EXPLAIN select a from t_pushdown where b > 2 ORDER BY a;
	----
	logical_plan
	01)Sort: t_pushdown.a ASC NULLS LAST
	02)--TableScan: t_pushdown projection=[a], full_filters=[t_pushdown.b > Int32(2)]
	physical_plan
	01)SortPreservingMergeExec: [a@0 ASC NULLS LAST]
	02)--SortExec: expr=[a@0 ASC NULLS LAST], preserve_partitioning=[true]
	03)----DataSourceExec: file_groups={2 groups: [[WORKSPACE_ROOT/datafusion/sqllogictest/test_files/scratch/parquet_filter_pushdown/parquet_table/1.parquet], [WORKSPACE_ROOT/datafusion/sqllogictest/test_files/scratch/parquet_filter_pushdown/parquet_table/2.parquet]]}, projection=[a], file_type=parquet, predicate=b@1 > 2, pruning_predicate=b_null_count@1 != row_count@2 AND b_max@0 > 2, required_guarantees=[]

Implement Parquet filter pushdown via new filter pushdown APIs #15769

Are you sure you want to change the base?

Implement Parquet filter pushdown via new filter pushdown APIs #15769

Conversation

adriangb commented Apr 18, 2025 • edited Loading

adriangb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb commented Apr 19, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adriangb Apr 21, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adriangb commented Apr 20, 2025

berkaysynnada left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adriangb commented Apr 22, 2025 • edited Loading

berkaysynnada left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adriangb commented Apr 24, 2025 • edited Loading

berkaysynnada commented Apr 24, 2025 • edited Loading

adriangb commented Apr 24, 2025

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

berkaysynnada commented May 3, 2025

adriangb commented May 3, 2025

berkaysynnada commented May 3, 2025 • edited Loading

adriangb commented May 3, 2025

adriangb commented May 4, 2025

adriangb commented Apr 18, 2025 •

edited

Loading

adriangb Apr 21, 2025 •

edited

Loading

adriangb commented Apr 22, 2025 •

edited

Loading

adriangb commented Apr 24, 2025 •

edited

Loading

berkaysynnada commented Apr 24, 2025 •

edited

Loading

berkaysynnada commented May 3, 2025 •

edited

Loading