proto: serialize and dedupe dynamic filters by jayshrivastava · Pull Request #20416 · apache/datafusion

jayshrivastava · 2026-02-17T21:48:44Z

Which issue does this PR close?

Informs: datafusion-contrib/datafusion-distributed#180
Informs: #21207
Closes: #20418

Rationale for this change

Consider you have a plan with a HashJoinExec and DataSourceExec

HashJoinExec(dynamic_filter_1 on a@0)
  (...left side of join)
  ProjectionExec(a := Column("a", source_index))
    DataSourceExec
      ParquetSource(predicate = dynamic_filter_2)

You serialize the plan, deserialize it, and execute it. What should happen is that the dynamic filter should "work", meaning:

When you deserialize the plan, both the HashJoinExec and DataSourceExec should have pointers to the same DynamicFilterPhysicalExpr
The DynamicFilterPhysicalExpr should be updated during execution by the HashJoinExec and the DataSourceExec should filter out rows

This does not happen today for a few reasons, a couple of which this PR aims to address

DynamicFilterPhysicalExpr is not survive round-tripping. The internal exprs get inlined (ex. it may be serialized as Literal) due to the PhysicalExpr::snapshot() API
Even if DynamicFilterPhysicalExpr survives round-tripping, the one pushed down to the DataSourceExec often has different children. In this case, you have two DynamicFilterPhysicalExpr which
do not survive deduping, causing referential integrity to be lost.

This PR aims to fix those problems by:

Removing the snapshot() call from the serialization process
Adding protos for DynamicFilterPhysicalExpr so it can be serialized and deserialized
Adding a new concept, a PhysicalExprId, which has two identifiers,
a "shallow" identifier to indicate two equal expressions which may
have different children, and an "exact" identifier to indicate two
exprs that are exactly the same.
Updating the deduping deserializer and protos to now be aware of the
new "shallow" id, deduping exprs which are the same but have
different children accordingly.

This change adds tests which roundtrip dynamic filters and assert that
referential integrity is maintained.

Future work:

Serialize dynamic filters in HashJoinExec and other ExecutionPlans which produce dynamic filters
Add tests which actually execute plans after deserialization and assert that dynamic filtering is functional
Add proto converters to the PhysicalExtensionCodec trait so implementors can utilize deduping logic

Are these changes tested?

Yes. This change adds tests which roundtrip dynamic filters and assert that referential integrity is maintained.

Are there any user-facing changes?

The default codec does not call snapshot() on PhysicalExpr during serialization anymore. This means that DynamicFilterPhysicalExpr are now serialized and deserialized without snapshotting

NGA-TRAN

Glad you tracked down the root cause. Great explanation of the two issue.
The changes align well with your description, and the tests look solid. I’ll let the folks who know the Dynamic Filtering details comment further on the specifics.

jayshrivastava · 2026-02-18T22:14:07Z


+/// An atomic snapshot of a [`DynamicFilterPhysicalExpr`] used to reconstruct the expression during
+/// serialization / deserialization.
+pub struct DynamicFilterSnapshot {


Added this type because I thought it looked cleaner / simpler than having getters on the DynamicFilterPhysicalExpr itself.

Maybe we should just add the getters and setters for the internal fields...

It seems like we end up exposing all of the internals anyway: if there was a way to return a dyn Serializable it'd be one thing, but the proto machinery still needs to know all of the fields to serialize.

I also think the name snapshot makes it sounds like this is what would be returned from PhysicalExpr::snapshot, but that is not the case.

Could be... you kind of have already this, just to an intermediate structure instead

gene-bordegaray

overall looks good, had one idea as to making the api more familiar to those new with it. Let me know what you think

gabotechs

Overall, I think we should find alternative ways of doing this that do not imply special-casing the dynamic filter serialization process.

I shared some ideas on how to do this, but they probably need to be fleshed out a bit more.

The things that I think this PR should be achieving is:

No special casing for dynamic filters in serialization/deserialization code
No changes to global protobuf messages just for the sake of dynamic filters, just a new normal entry in the enum PhysicalDynamicFilterNode

Things that IMO we should have, probably in a separate PR:

Stop playing with raw pointer addresses in dynamic filters and assign proper unique identifiers, probably using the uuid crate.

Fixups for the cherry-picked commits from PRs apache#19437, apache#20037, apache#20416, and #2 to work with branch-52's partition-index APIs: - Update remap_children callers to use instance method signature - Adapt DynamicFilterUpdate::Global enum for new code paths - Add missing partitioned_exprs/runtime_partition fields to new constructors - Remove null_aware field (not on branch-52) - Replace FilterExecBuilder with FilterExec::try_new - Remove non-compiling tests that depend on upstream-only APIs - Fix duplicate imports in roundtrip test file Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Fixups for the cherry-picked commits from PRs apache#19437, apache#20037, apache#20416, and jayshrivastava#2 to work with branch-52's partition-index APIs: - Update remap_children callers to use instance method signature - Adapt DynamicFilterUpdate::Global enum for new code paths - Add missing partitioned_exprs/runtime_partition fields to new constructors - Remove null_aware field (not on branch-52) - Replace FilterExecBuilder with FilterExec::try_new - Remove non-compiling tests that depend on upstream-only APIs - Fix duplicate imports in roundtrip test file Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Informs: datafusion-contrib/datafusion-distributed#180 Closes: apache#20418 Consider you have a plan with a `HashJoinExec` and `DataSourceExec` ``` HashJoinExec(dynamic_filter_1 on a@0) (...left side of join) ProjectionExec(a := Column("a", source_index)) DataSourceExec ParquetSource(predicate = dynamic_filter_2) ``` You serialize the plan, deserialize it, and execute it. What should happen is that the dynamic filter should "work", meaning: 1. When you deserialize the plan, both the `HashJoinExec` and `DataSourceExec` should have pointers to the same `DynamicFilterPhysicalExpr` 2. The `DynamicFilterPhysicalExpr` should be updated during execution by the `HashJoinExec` and the `DataSourceExec` should filter out rows This does not happen today for a few reasons, a couple of which this PR aims to address 1. `DynamicFilterPhysicalExpr` is not survive round-tripping. The internal exprs get inlined (ex. it may be serialized as `Literal`) due to the `PhysicalExpr::snapshot()` API 2. Even if `DynamicFilterPhysicalExpr` survives round-tripping, the one pushed down to the `DataSourceExec` often has different children. In this case, you have two `DynamicFilterPhysicalExpr` which do not survive deduping, causing referential integrity to be lost. This PR aims to fix those problems by: 1. Removing the `snapshot()` call from the serialization process 2. Adding protos for `DynamicFilterPhysicalExpr` so it can be serialized and deserialized 3. Adding a new concept, a `PhysicalExprId`, which has two identifiers, a "shallow" identifier to indicate two equal expressions which may have different children, and an "exact" identifier to indicate two exprs that are exactly the same. 4. Updating the deduping deserializer and protos to now be aware of the new "shallow" id, deduping exprs which are the same but have different children accordingly. This change adds tests which roundtrip dynamic filters and assert that referential integrity is maintained.

## Which issue does this PR close?  N/A ## Rationale for this change  Some PRs are being omitted from stale check because they were in a cache, and the workflow appears to not have permission to delete cache so they are forever stuck as unprocessed. For example in this run: https://github.com/apache/datafusion/actions/runs/24756695077/job/72431314533 Seeing this in logs: ``` [apache#20473] issue skipped due being processed during the previous run [apache#20460] pull request skipped due being processed during the previous run [apache#20448] issue skipped due being processed during the previous run [apache#20443] issue skipped due being processed during the previous run [apache#20435] issue skipped due being processed during the previous run [apache#20418] issue skipped due being processed during the previous run [apache#20417] pull request skipped due being processed during the previous run [apache#20416] pull request skipped due being processed during the previous run [apache#20403] pull request skipped due being processed during the previous run ``` And at the end we see this warning: ``` Warning: Error delete _state: [403] Resource not accessible by integration - https://docs.github.com/rest/actions/cache#delete-github-actions-caches-for-a-repository-using-a-cache-key ``` stale workflow uses a cache in case it hits the `operations-per-run` limit meant to prevent API rate limiting (we have default of 30), so it seems we previously hit this limit and some issues/PRs were cached, and have never been uncached since so are never processed again. See: https://github.com/actions/stale#operations-per-run ## What changes are included in this PR?  Give permission to stale workflow to run github actions (like delete cache). See recommended permissions: https://github.com/actions/stale#recommended-permissions ## Are these changes tested?  ## Are there any user-facing changes?

github-actions Bot added physical-expr Changes to the physical-expr crates proto Related to proto crate labels Feb 17, 2026

jayshrivastava changed the title ~~wip~~ proto: serialize dynamic filters Feb 18, 2026

jayshrivastava changed the title ~~proto: serialize dynamic filters~~ proto: serialize and dedupe dynamic filters Feb 18, 2026

jayshrivastava force-pushed the js/dedupe-dynamic-filter-inner-state branch from 158f2cf to e0c6be3 Compare February 18, 2026 00:18

NGA-TRAN approved these changes Feb 18, 2026

View reviewed changes

jayshrivastava commented Feb 18, 2026

View reviewed changes

gene-bordegaray reviewed Feb 19, 2026

View reviewed changes

Comment thread datafusion/physical-expr/src/expressions/dynamic_filters.rs Outdated

LiaCastaneda reviewed Feb 19, 2026

View reviewed changes

Comment thread datafusion/physical-expr/src/expressions/dynamic_filters.rs Outdated

Comment thread datafusion/physical-expr/src/expressions/dynamic_filters.rs Outdated

jayshrivastava force-pushed the js/dedupe-dynamic-filter-inner-state branch from 5d99a1f to 68e3f72 Compare February 19, 2026 21:01

LiaCastaneda reviewed Feb 19, 2026

View reviewed changes

LiaCastaneda reviewed Feb 23, 2026

View reviewed changes

Comment thread datafusion/physical-expr/src/expressions/dynamic_filters.rs Outdated

jayshrivastava marked this pull request as ready for review February 23, 2026 15:03

adriangb reviewed Feb 23, 2026

View reviewed changes

Comment thread datafusion/proto/src/physical_plan/mod.rs

jayshrivastava mentioned this pull request Feb 23, 2026

Serialize dynamic filters on execution plan nodes (HashJoin, Aggregate, Sort) jayshrivastava/datafusion#2

Draft

gabotechs mentioned this pull request Feb 24, 2026

feat: enable dynamic filter pushdown across serialization boundary datafusion-contrib/datafusion-distributed#348

Closed

gabotechs reviewed Feb 25, 2026

View reviewed changes

jayshrivastava force-pushed the js/dedupe-dynamic-filter-inner-state branch from c5d0e2f to fef4259 Compare February 26, 2026 18:48

jayshrivastava commented Feb 27, 2026

View reviewed changes

Comment thread datafusion/physical-expr/src/expressions/dynamic_filters.rs Outdated

jayshrivastava force-pushed the js/dedupe-dynamic-filter-inner-state branch from cb23b01 to 18b0289 Compare March 19, 2026 15:04

gabotechs mentioned this pull request Mar 25, 2026

feat: add generic runtime binding for physical expressions #20901

Open

gene-bordegaray mentioned this pull request Mar 27, 2026

[DISCUSSION] Future of Dynamic Filters Sync #21207

Open

jayshrivastava force-pushed the js/dedupe-dynamic-filter-inner-state branch 3 times, most recently from d75e7f8 to e0ec773 Compare April 14, 2026 17:21

jayshrivastava force-pushed the js/dedupe-dynamic-filter-inner-state branch from d64c33d to b419d4c Compare April 22, 2026 02:32

jayshrivastava force-pushed the js/dedupe-dynamic-filter-inner-state branch from b419d4c to dc683d3 Compare April 22, 2026 03:53

jayshrivastava added 2 commits April 22, 2026 04:31

fix ci

b4f84c4

clipp

244a848

adriangb mentioned this pull request Apr 22, 2026

proto: preserve DynamicFilterPhysicalExpr identity across round-trip #21786

Open

Conversation

jayshrivastava commented Feb 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

Rationale for this change

Are these changes tested?

Are there any user-facing changes?

Uh oh!

NGA-TRAN left a comment

Choose a reason for hiding this comment

Uh oh!

jayshrivastava Feb 18, 2026

Choose a reason for hiding this comment

Uh oh!

jayshrivastava Feb 20, 2026

Choose a reason for hiding this comment

Uh oh!

adriangb Feb 23, 2026

Choose a reason for hiding this comment

Uh oh!

adriangb Feb 23, 2026

Choose a reason for hiding this comment

Uh oh!

gabotechs Feb 25, 2026

Choose a reason for hiding this comment

Uh oh!

gene-bordegaray left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gabotechs left a comment • 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

jayshrivastava commented Feb 17, 2026 •

edited

Loading

gabotechs left a comment •

edited

Loading