Add FilterExecBuilder to avoid recomputing properties multiple times #19854

adriangb · 2026-01-16T15:53:12Z

This commit fixes issue apache#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 apache#19612

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 apache#19608

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

- Updated deprecation version from 51.0.0 to 52.0.0 for FilterExec methods - Added comprehensive entry to upgrading.md explaining the migration path - All 114 filter tests passing

- 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

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

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

adriangb · 2026-01-16T15:55:49Z

@nuno-faria would you mind reviewing here? I hope b2342d6 addressed your concerns about with_projection.

nuno-faria

Thank you @adriangb, LGTM.

adriangb · 2026-01-16T19:44:44Z

@alamb could you approve this since @nuno-faria doesn't have approval rights I think?

alamb · 2026-01-16T19:53:58Z

BTW this is potentially related to work that @askalt is working on in

https://github.com/apache/datafusion/pull/19792/changes#diff-ae492510820d46b07e2d7420f4555fcb005408682bf3e8aed85faad7925d4545

(I think the approaches are complimentary, but they are both targeting the same observation that Recalculating PlanProperties is expensive)

alamb

Thanks @adriangb and @nuno-faria -- this looks good to me

alamb · 2026-01-16T21:03:31Z

datafusion/physical-plan/src/filter.rs

+    input: Arc<dyn ExecutionPlan>,
+    projection: Option<Vec<usize>>,
+    default_selectivity: u8,
+    batch_size: usize,


Since it would be so easy to overlook, I recommend either

making batch_size: Option<usze> and then throwing an internal error if it is not set

require batch_size in the constructor

In main it is also a module level constant by default that gets overridden with a setter. Forcing it as a constructor parameter would require a lot of breaking changes (pushing that constant onto the caller wherever there is not an obvious value available). Could we maybe leave it as is for now?

sure. Make sense to me

makes sense to me

alamb · 2026-01-16T21:05:42Z

datafusion/physical-plan/src/filter.rs

    pub fn with_projection(&self, projection: Option<Vec<usize>>) -> Result<Self> {
-        //  Check if the projection is valid
+        // Check if the projection is valid against current output schema
        can_project(&self.schema(), projection.as_ref())?;


I wonder if the checks in can_project should be in the FilterExecBuilder?

alamb · 2026-01-16T21:08:38Z

datafusion/physical-plan/src/filter.rs

+    ///
+    /// If no projection is currently set, the new projection is used directly.
+    /// If `None` is passed, the projection is cleared.
+    pub fn with_projection(mut self, projection: Option<Vec<usize>>) -> Self {


Given this follow on projection behavior, I wonder if calling this apply_projection or with_additional_projection would make that cleaer. I missed it first time through until I was looking at the tests

Good call, renamed!

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

adriangb · 2026-01-16T23:05:29Z

@alamb I addressed your feedback mostly.

I also implemented FilterExecBuilder::from(FilterExec) which simplified things a bit.
Left a note about re-using plan properties.

Edit: seems like I need to fix CI though...

alamb · 2026-01-18T17:09:55Z

Look like an improvement to me -- thanks @adriangb and @nuno-faria

alamb · 2026-01-18T17:08:17Z

datafusion/core/src/physical_planner.rs

                        // project the output columns excluding the async functions
                        // The async functions are always appended to the end of the schema.
-                        .with_projection(Some(
+                        .apply_projection(Some(


I like this "apply projection" name 👍

alamb · 2026-01-18T17:08:38Z

datafusion/physical-plan/src/filter.rs

+    input: Arc<dyn ExecutionPlan>,
+    projection: Option<Vec<usize>>,
+    default_selectivity: u8,
+    batch_size: usize,


makes sense to me

GaneshPatil7517 and others added 17 commits January 3, 2026 13:57

fix: add missing closing brace for tests module

bf8ea99

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

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

fbda73e

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

5273fe8

Fix deprecation version to 52.0.0 and add upgrading guide entry

0726087

- Updated deprecation version from 51.0.0 to 52.0.0 for FilterExec methods - Added comprehensive entry to upgrading.md explaining the migration path - All 114 filter tests passing

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

move composition of projections into FilterExecBuilder::with_projection

b2342d6

github-actions bot added documentation Improvements or additions to documentation core Core DataFusion crate proto Related to proto crate physical-plan Changes to the physical-plan crate labels Jan 16, 2026

Merge branch 'main' into filterexecbuilder

c941ca4

adriangb added 2 commits January 16, 2026 12:20

lint

243b6e8

fix unwrap

3d943a6

nuno-faria approved these changes Jan 16, 2026

View reviewed changes

Merge branch 'main' into filterexecbuilder

8600872

alamb added the performance Make DataFusion faster label Jan 16, 2026

alamb approved these changes Jan 16, 2026

View reviewed changes

adriangb added 2 commits January 16, 2026 17:58

add impl From<&FilterExec> for FilterExecBuilder, address feedback

e5be102

fix

3b27378

fix

dfe4a02

alamb added this pull request to the merge queue Jan 18, 2026

Merged via the queue into apache:main with commit 936f959 Jan 18, 2026
33 checks passed

alamb reviewed Jan 18, 2026

View reviewed changes

Add FilterExecBuilder to avoid recomputing properties multiple times #19854

Add FilterExecBuilder to avoid recomputing properties multiple times #19854

Uh oh!

Conversation

adriangb commented Jan 16, 2026 • edited by alamb Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adriangb commented Jan 16, 2026

Uh oh!

nuno-faria left a comment

Choose a reason for hiding this comment

Uh oh!

adriangb commented Jan 16, 2026

Uh oh!

alamb commented Jan 16, 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!

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!

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

Uh oh!

Uh oh!

alamb commented Jan 18, 2026

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

adriangb commented Jan 16, 2026 •

edited by alamb

Loading

adriangb commented Jan 16, 2026 •

edited

Loading