Add `FilterExecBuilder` (#19608) by GaneshPatil7517 · Pull Request #19619 · apache/datafusion

GaneshPatil7517 · 2026-01-03T08:55:07Z

This PR introduces a new FilterExec constructor that accepts a projection at creation time,
ensuring compute_properties() executes only once.

What was added:

New try_new_with_projection() method for FilterExec
Projection is applied during plan creation
compute_properties() now runs a single time
No regressions in existing query plans
Tests validate performance and correctness
Documentation updated to reflect usage

Fixes: #19608

This commit fixes issue #19612 where accumulators that don't implement retract_batch exhibit buggy behavior in window frame queries. ## Problem When aggregate functions are used with window frames like `ROWS BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW`, DataFusion uses PlainAggregateWindowExpr which calls evaluate() multiple times on the same accumulator instance. Accumulators that use std::mem::take() in their evaluate() method consume their internal state, causing incorrect results on subsequent calls. ## Solution 1. **percentile_cont**: Modified evaluate() to use mutable reference instead of consuming the Vec. Added retract_batch() support for both PercentileContAccumulator and DistinctPercentileContAccumulator. 2. **string_agg**: Changed SimpleStringAggAccumulator::evaluate() to clone the accumulated string instead of taking it. ## Changes - datafusion/functions-aggregate/src/percentile_cont.rs: - Changed calculate_percentile() to take &mut [T::Native] instead of Vec<T::Native> - Updated PercentileContAccumulator::evaluate() to pass reference - Updated DistinctPercentileContAccumulator::evaluate() to clone values - Added retract_batch() implementation using HashMap for efficient removal - Updated PercentileContGroupsAccumulator::evaluate() for consistency - datafusion/functions-aggregate/src/string_agg.rs: - Changed evaluate() to use clone() instead of std::mem::take() - datafusion/sqllogictest/test_files/aggregate.slt: - Added test cases for percentile_cont with window frames - Added test comparing median() vs percentile_cont(0.5) behavior - Added test for string_agg cumulative window frame - docs/source/library-user-guide/functions/adding-udfs.md: - Added documentation about window-compatible accumulators - Explained evaluate() state preservation requirements - Documented retract_batch() implementation guidance Closes #19612

nuno-faria

Thanks @GaneshPatil7517 for taking a look at this. I left some comments below. I think it might be better to add a builder as proposed by @adriangb instead of the new constructor.

nuno-faria · 2026-01-03T11:16:12Z

datafusion/physical-plan/src/filter.rs

+    /// )?;
+    /// ```
+    #[expect(clippy::needless_pass_by_value)]
+    pub fn try_new_with_projection(


@adriangb suggested to add a builder instead of a new constructor and I tend to agree. @GaneshPatil7517 could you create a FilterExecBuilder, with:

a new function which would take the predicate and input, since they are mandatory.

several with_... methods for the projection, default_selectivity, batch_size, and fetch.

a build method which would return the FilterExec.

We then might even consider to have the original try_new use the builder instead, to reduce the amount of duplicate code.

nuno-faria · 2026-01-03T11:17:04Z

docs/source/library-user-guide/filterexec-with-projection.md

@@ -0,0 +1,147 @@
+<!---


I don't think a new user guide is needed just for this change.

nuno-faria · 2026-01-03T11:18:22Z

datafusion/functions-aggregate/src/percentile_cont.rs

        size_of_val(self) + self.all_values.capacity() * size_of::<T::Native>()
    }
+
+    fn retract_batch(&mut self, values: &[ArrayRef]) -> Result<()> {


I'm not sure how this relates to this PR.

nuno-faria · 2026-01-03T11:19:14Z

datafusion/sqllogictest/test_files/aggregate.slt


 statement ok
 drop table distinct_avg;
+


Also not sure why new aggregate tests are required.

nuno-faria · 2026-01-03T11:20:05Z

docs/source/library-user-guide/functions/adding-udfs.md

 [`create_udaf`]: https://docs.rs/datafusion/latest/datafusion/logical_expr/fn.create_udaf.html
 [`advanced_udaf.rs`]: https://github.com/apache/datafusion/blob/main/datafusion-examples/examples/udf/advanced_udaf.rs

+### Window Frame Compatible Accumulators


This also doesn't fit in this PR.

Adds FilterExecBuilder pattern with fluent API Allows setting projection, selectivity, batch_size, fetch in one build Refactors try_new to use builder internally (reduces duplication) Ensures compute_properties executes only once Fixes #19608

nuno-faria · 2026-01-03T15:27:06Z

@GaneshPatil7517 thanks for the changes. There are still some changes that I think are from a different PR, at functions-aggregate , aggregate.slt, and adding-udfs.md. Could you remove those as well?

GaneshPatil7517 · 2026-01-03T15:30:25Z

ok @nuno-faria ill remove it...

GaneshPatil7517 · 2026-01-04T13:36:56Z

@nuno-faria Please Review this...

nuno-faria · 2026-01-04T20:59:20Z

@alamb could you please enable the workflows in this PR? Thanks.

adriangb · 2026-01-04T21:07:13Z

I’ve set them to run

GaneshPatil7517 · 2026-01-05T02:01:32Z

there are 2 failing and 30 successful checks, let me solve this....

GaneshPatil7517 · 2026-01-05T04:49:31Z

Hi @nuno-faria & @adriangb ,
All checks have passed successfully on this PR. I’m really excited to see it get merged 🚀
Kindly request you to review and approve when you get a moment.

Thank you for your time and support! 🙌

adriangb

Should we deprecate with_projection and other methods on FilterExec and re-implement them in terms of the building until they are removed?

GaneshPatil7517 · 2026-01-05T05:20:54Z

ok ill work on that...

As suggested in PR review, deprecate with_projection(), with_default_selectivity(), and with_batch_size() methods on FilterExec. These methods now use FilterExecBuilder internally for backward compatibility while guiding users toward the builder pattern. - Marked methods as deprecated since 51.0.0 - Re-implemented using FilterExecBuilder to maintain functionality - All 114 filter tests passing - Provides gentle migration path for users

GaneshPatil7517 · 2026-01-05T05:25:54Z

hey @adriangb please can you review it i updated it....

GaneshPatil7517 · 2026-01-05T06:14:47Z

Hey @adriangb sir please can you cheack now...

- Replace .clone() with Arc::clone() to follow Rust best practices - Replace deprecated method calls in internal code with direct builder usage - Update with_new_children, try_swapping_with_projection, and EmbeddedProjection impl - All 114 filter tests passing - No clippy warnings or errors

nuno-faria · 2026-01-05T08:36:16Z

datafusion/physical-plan/src/filter.rs

+    #[deprecated(since = "52.0.0", note = "Use FilterExecBuilder::with_default_selectivity instead")]
    pub fn with_default_selectivity(
-        mut self,
+        self,
        default_selectivity: u8,
    ) -> Result<Self, DataFusionError> {
-        if default_selectivity > 100 {
-            return plan_err!(
-                "Default filter selectivity value needs to be less than or equal to 100"
-            );
-        }
-        self.default_selectivity = default_selectivity;
-        Ok(self)
+        FilterExecBuilder::new(Arc::clone(&self.predicate), Arc::clone(&self.input))
+            .with_projection(self.projection.clone())
+            .with_default_selectivity(default_selectivity)
+            .with_batch_size(self.batch_size)
+            .with_fetch(self.fetch)
+            .build()
    }


I'm not sure if the with_default_selectivity method should be deprecated, since before it simply updated the field. Now we would need to create a new FilterExec from scratch.

ok ill work on it...

The reviewer pointed out that with_default_selectivity() simply updates a field and doesn't need the overhead of creating a new FilterExec via the builder. Restored the original efficient implementation. Only with_projection() and with_batch_size() remain deprecated, as they benefit from the builder pattern's single property computation. - Restored original with_default_selectivity implementation - Updated upgrading.md to reflect only 2 deprecated methods - All 114 filter tests passing - No clippy warnings

nuno-faria · 2026-01-05T19:20:26Z

@adriangb could you run the workflows again?
@GaneshPatil7517 since with_projection and with_batch_size are being deprecated, we also need to update those uses in DataFusion itself. There are uses at physical_planner.rs, physical_plan/mod.rs, filter_pushdown/mod.rs, filter.rs (at test_builder_vs_with_projection), and at roundtrip_physical_plan.rs.

GaneshPatil7517 · 2026-01-05T19:23:26Z

@adriangb could you run the workflows again? @GaneshPatil7517 since with_projection and with_batch_size are being deprecated, we also need to update those uses in DataFusion itself. There are uses at physical_planner.rs, physical_plan/mod.rs, filter_pushdown/mod.rs, filter.rs (at test_builder_vs_with_projection), and at roundtrip_physical_plan.rs.

ok ill updating it...

Per reviewer feedback, updated all internal uses of deprecated with_projection() and with_batch_size() methods to use FilterExecBuilder instead: - datafusion/core/src/physical_planner.rs: Use FilterExecBuilder for filter creation - datafusion/proto/src/physical_plan/mod.rs: Use FilterExecBuilder in proto deserialization - datafusion/physical-plan/src/filter.rs: Updated test to use builder pattern Also restored with_default_selectivity() to non-deprecated status since it simply updates a field value without the overhead of rebuilding FilterExec. All tests passing, no clippy warnings.

…es in tests

GaneshPatil7517 · 2026-01-06T14:27:12Z

hey @adriangb could you run the workflows again.....?

GaneshPatil7517 · 2026-01-06T16:27:52Z

Hi @nuno-faria & @adriangb ,
All checks have passed successfully on this PR. I’m really excited to see it get merged 🚀
Kindly request you to review and approve when you get a moment.
Thank you for your time and support! 🙌

alamb · 2026-01-07T21:14:16Z

run benchmark sql_planner

alamb-ghbot · 2026-01-07T21:14:23Z

🤖 ./gh_compare_branch_bench.sh compare_branch_bench.sh Running
Linux aal-dev 6.14.0-1018-gcp #19~24.04.1-Ubuntu SMP Wed Sep 24 23:23:09 UTC 2025 x86_64 x86_64 x86_64 GNU/Linux
Comparing add-filterexec-with-projection (6a341a9) to 1037f0a diff
BENCH_NAME=sql_planner
BENCH_COMMAND=cargo bench --features=parquet --bench sql_planner
BENCH_FILTER=
BENCH_BRANCH_NAME=add-filterexec-with-projection
Results will be posted here when complete

alamb

Thanks @GaneshPatil7517 and @nuno-faria and @adriangb -- this looks really nice to me

alamb · 2026-01-07T21:15:26Z

datafusion/physical-plan/src/filter.rs

+    /// # Deprecated
+    /// Use [`FilterExecBuilder::with_projection`] instead
+    #[deprecated(
+        since = "52.0.0",


this technically will now go out in 53 I think

alamb · 2026-01-07T21:15:42Z

docs/source/library-user-guide/upgrading.md


 You can see the current [status of the `52.0.0`release here](https://github.com/apache/datafusion/issues/18566)

+### `FilterExec` builder methods deprecated


We need to move these to the 53 release section

alamb-ghbot · 2026-01-07T21:49:08Z

Benchmark script failed with exit code 101.

Last 10 lines of output:

Click to expand

  3 (3.00%) high severe

Benchmarking physical_plan_clickbench_q50
Benchmarking physical_plan_clickbench_q50: Warming up for 3.0000 s

thread 'main' (580581) panicked at datafusion/core/benches/sql_planner.rs:62:14:
called `Result::unwrap()` on an `Err` value: Context("type_coercion", Internal("Expect TypeSignatureClass::Native(LogicalType(Native(String), String)) but received NativeType::Binary, DataType: BinaryView"))
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

error: bench failed, to rerun pass `-p datafusion --bench sql_planner`

nuno-faria

Thanks @GaneshPatil7517. I still have comments, this time about the with_projection method.

nuno-faria · 2026-01-07T22:11:10Z

datafusion/physical-plan/src/filter.rs

+        // Check if the projection is valid
+        can_project(&self.schema(), projection.as_ref())?;
+
+        let projection = match projection {
+            Some(projection) => match &self.projection {
+                Some(p) => Some(projection.iter().map(|i| p[*i]).collect()),
+                None => Some(projection),
+            },
+            None => None,
+        };
+
+        FilterExecBuilder::new(Arc::clone(&self.predicate), Arc::clone(&self.input))
+            .with_projection(projection)
+            .with_default_selectivity(self.default_selectivity)
+            .with_batch_size(self.batch_size)
+            .with_fetch(self.fetch)
+            .build()


I'm having some doubts about deprecating with_projection of FilterExec (I highlight here the with_projection of impl EmbeddedProjection, which has the same code). Mainly about this part here:

let projection = match projection { Some(projection) => match &self.projection { Some(p) => Some(projection.iter().map(|i| p[*i]).collect()), None => Some(projection), }, None => None, };

It appears that the projection passed to the method tries to project the existing projection. I'm not sure why this is the case, but I think if we deprecate with_projection we lose the ability to do this. But I also haven't seen a with_projection called on an existing FilterExec, it is always accompanied with a try_new.

@adriangb @alamb what do you think?

I guess FilterExecBuilder could do the same thing? I can see why that would be the correct behavior.

But in this case FilterExecBuilder always builds a new filter, while the previous with_projection could operate over an existing one.

I still don't get it. The old with_projection also built a new FilterExec:

let cache = Self::compute_properties( &self.input, &self.predicate, self.default_selectivity, projection.as_ref(), )?; Ok(Self { predicate: Arc::clone(&self.predicate), input: Arc::clone(&self.input), metrics: self.metrics.clone(), default_selectivity: self.default_selectivity, cache, projection, batch_size: self.batch_size, fetch: self.fetch, })

The only difference I see is that the old implementation composed projections, the new one replaces them. So maybe all we need to do is implement the projection composition in FilterExecBuilder::with_projection to match the existing behavior?

Thanks for the detailed analysis @adriangb and @nuno-faria! You're right - the old implementation composed projections while the new one replaces them. I'll update FilterExecBuilder::with_projection to compose the projections instead of replacing, matching the original behavior. This way we maintain backward compatibility while still having the cleaner builder API.

adriangb · 2026-01-13T04:45:02Z

Hi @GaneshPatil7517 I'm sorry this has taken many rounds of feedback including delays between the reviews. It can be hard to find time to be a good open source contributor / reviewer sometimes. I hope that wasn't too discouraging. I think this is a good idea and want to see it across the finish line.

…19854) - Closes #19608, - replaces #19619 --------- Co-authored-by: Ganesh Patil <[email protected]>

github-actions bot added documentation Improvements or additions to documentation sqllogictest SQL Logic Tests (.slt) functions Changes to functions implementation physical-plan Changes to the physical-plan crate labels Jan 3, 2026

GaneshPatil7517 mentioned this pull request Jan 3, 2026

Add a way to create a new FilterExec directly with a projection #19608

Closed

nuno-faria requested changes Jan 3, 2026

View reviewed changes

GaneshPatil7517 added 2 commits January 3, 2026 17:30

fix: add missing closing brace for tests module

bf8ea99

GaneshPatil7517 force-pushed the add-filterexec-with-projection branch from 74944f3 to bf8ea99 Compare January 3, 2026 12:08

GaneshPatil7517 added 2 commits January 3, 2026 18:04

fix: update tests to use FilterExecBuilder pattern correctly

4823862

fix(physical-plan): compute filter statistics using input schema

b393b38

chore: remove unrelated changes from PR

3dfc827

github-actions bot removed documentation Improvements or additions to documentation sqllogictest SQL Logic Tests (.slt) functions Changes to functions implementation labels Jan 3, 2026

GaneshPatil7517 added 2 commits January 5, 2026 07:36

fix: resolve cargo fmt and clippy issues in filter.rs

fbda73e

fix: use Arc::clone instead of .clone() on ref-counted pointers

5273fe8

adriangb reviewed Jan 5, 2026

View reviewed changes

GaneshPatil7517 force-pushed the add-filterexec-with-projection branch from 5f35662 to 26e5c46 Compare January 5, 2026 05:22

GaneshPatil7517 force-pushed the add-filterexec-with-projection branch from 9596ceb to aa4a892 Compare January 5, 2026 06:46

nuno-faria reviewed Jan 5, 2026

View reviewed changes

GaneshPatil7517 force-pushed the add-filterexec-with-projection branch from f961dc9 to a601075 Compare January 5, 2026 09:14

GaneshPatil7517 force-pushed the add-filterexec-with-projection branch from 3701930 to dac2557 Compare January 6, 2026 01:18

github-actions bot added core Core DataFusion crate proto Related to proto crate labels Jan 6, 2026

GaneshPatil7517 added 3 commits January 6, 2026 06:50

Merge branch 'main' into add-filterexec-with-projection

30b1188

fix: address CI failures - format code and fix deprecated method usag…

f9a2710

…es in tests

Merge branch 'main' into add-filterexec-with-projection

6a341a9

adriangb approved these changes Jan 6, 2026

View reviewed changes

adriangb mentioned this pull request Jan 6, 2026

Split BatchPartitioner::try_new(..) into BatchPartitioner::new_hash and BatchPartitioner::new_round_robin #19664

Closed

alamb requested a review from nuno-faria January 7, 2026 21:14

alamb changed the title ~~Add one-step FilterExec creation with projection (#19608)~~ Add FilterExecBuilder (#19608) Jan 7, 2026

alamb approved these changes Jan 7, 2026

View reviewed changes

nuno-faria reviewed Jan 7, 2026

View reviewed changes

GaneshPatil7517 closed this by deleting the head repository Jan 13, 2026

adriangb mentioned this pull request Jan 16, 2026

Add FilterExecBuilder to avoid recomputing properties multiple times #19854

Merged

github-merge-queue bot pushed a commit that referenced this pull request Jan 18, 2026

Add FilterExecBuilder to avoid recomputing properties multiple times (#…

936f959

…19854) - Closes #19608, - replaces #19619 --------- Co-authored-by: Ganesh Patil <[email protected]>


		You can see the current [status of the `52.0.0`release here](https://github.com/apache/datafusion/issues/18566)

		### `FilterExec` builder methods deprecated

Conversation

GaneshPatil7517 commented Jan 3, 2026

Uh oh!

nuno-faria 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!

nuno-faria commented Jan 3, 2026

Uh oh!

GaneshPatil7517 commented Jan 3, 2026

Uh oh!

GaneshPatil7517 commented Jan 4, 2026

Uh oh!

nuno-faria commented Jan 4, 2026

Uh oh!

adriangb commented Jan 4, 2026

Uh oh!

GaneshPatil7517 commented Jan 5, 2026

Uh oh!

GaneshPatil7517 commented Jan 5, 2026

Uh oh!

adriangb left a comment

Choose a reason for hiding this comment

Uh oh!

GaneshPatil7517 commented Jan 5, 2026

Uh oh!

GaneshPatil7517 commented Jan 5, 2026

Uh oh!

GaneshPatil7517 commented Jan 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GaneshPatil7517 Jan 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nuno-faria commented Jan 5, 2026

Uh oh!

GaneshPatil7517 commented Jan 5, 2026

Uh oh!

GaneshPatil7517 commented Jan 6, 2026

Uh oh!

GaneshPatil7517 commented Jan 6, 2026

Uh oh!

alamb commented Jan 7, 2026

Uh oh!

alamb-ghbot commented Jan 7, 2026

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!

alamb-ghbot commented Jan 7, 2026

Uh oh!

nuno-faria 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!

GaneshPatil7517 commented Jan 5, 2026 •

edited

Loading

GaneshPatil7517 Jan 5, 2026 •

edited

Loading