physical-expr-common: remote `PhysicalExpr::snapshot` method by jayshrivastava · Pull Request #21975 · apache/datafusion

jayshrivastava · 2026-05-01T14:46:19Z

Which issue does this PR close?

Rationale for this change

PhysicalExpr::snapshot isn't required because it only applies to
dynamic filters. The only place that its used is in
pruning_predicate.rs. Since there's only one use case, we can just
downcast to DynamicFilterPhysicalExpr and call current().

What changes are included in this PR?

This change removes PhysicalExpr::snapshot and associated code. Callers of this now downcast to DynamicFilterPhysicalExpr and call current().

Are these changes tested?

Yes, the existing tests should cover the exact same functionality.
Filter pushdown still workers with dynamic filters; we use current()
instead of snapshot() to capture the expression.

Are there any user-facing changes?

Yes. PhysicalExpr::snapshot() no longer exists. Users should
downcast to DynamicFilterPhysicalExpr and call current() intead of
calling snapshot().

Similarly, in datafusion/ffi, FFI_PhysicalExpr no longer has a
snapshot() method.

github-actions · 2026-05-01T14:54:38Z

Thank you for opening this pull request!

Reviewer note: cargo-semver-checks reported the current version number is not SemVer-compatible with the changes in this pull request (compared against the base branch).

Details

     Cloning origin/main
    Building datafusion-ffi v53.1.0 (current)
       Built [  60.172s] (current)
     Parsing datafusion-ffi v53.1.0 (current)
      Parsed [   0.055s] (current)
    Building datafusion-ffi v53.1.0 (baseline)
       Built [  59.311s] (baseline)
     Parsing datafusion-ffi v53.1.0 (baseline)
      Parsed [   0.054s] (baseline)
    Checking datafusion-ffi v53.1.0 -> v53.1.0 (no change; assume patch)
     Checked [   0.245s] 222 checks: 220 pass, 1 fail, 1 warn, 30 skip

--- failure struct_pub_field_missing: pub struct's pub field removed or renamed ---

Description:
A publicly-visible struct has at least one public field that is no longer available under its prior name. It may have been renamed or removed entirely.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.47.0/src/lints/struct_pub_field_missing.ron

Failed in:
  field snapshot of struct FFI_PhysicalExpr, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-origin_main/1b4aa23fc54dabface2da814e74fe26e0b84c6a8/datafusion/ffi/src/physical_expr/mod.rs:115

--- warning repr_c_plain_struct_fields_reordered: struct fields reordered in repr(C) struct ---

Description:
A public repr(C) struct had its fields reordered. This can change the struct's memory layout, possibly breaking FFI use cases that depend on field position and order.
        ref: https://doc.rust-lang.org/reference/type-layout.html#reprc-structs
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.47.0/src/lints/repr_c_plain_struct_fields_reordered.ron

Failed in:
  FFI_PhysicalExpr.snapshot_generation moved from position 15 to 14, in /home/runner/work/datafusion/datafusion/datafusion/ffi/src/physical_expr/mod.rs:115
  FFI_PhysicalExpr.is_volatile_node moved from position 16 to 15, in /home/runner/work/datafusion/datafusion/datafusion/ffi/src/physical_expr/mod.rs:117
  FFI_PhysicalExpr.display moved from position 17 to 16, in /home/runner/work/datafusion/datafusion/datafusion/ffi/src/physical_expr/mod.rs:120
  FFI_PhysicalExpr.hash moved from position 18 to 17, in /home/runner/work/datafusion/datafusion/datafusion/ffi/src/physical_expr/mod.rs:123
  FFI_PhysicalExpr.clone moved from position 19 to 18, in /home/runner/work/datafusion/datafusion/datafusion/ffi/src/physical_expr/mod.rs:127
  FFI_PhysicalExpr.release moved from position 20 to 19, in /home/runner/work/datafusion/datafusion/datafusion/ffi/src/physical_expr/mod.rs:130
  FFI_PhysicalExpr.version moved from position 21 to 20, in /home/runner/work/datafusion/datafusion/datafusion/ffi/src/physical_expr/mod.rs:133
  FFI_PhysicalExpr.private_data moved from position 22 to 21, in /home/runner/work/datafusion/datafusion/datafusion/ffi/src/physical_expr/mod.rs:137
  FFI_PhysicalExpr.library_marker_id moved from position 23 to 22, in /home/runner/work/datafusion/datafusion/datafusion/ffi/src/physical_expr/mod.rs:141

     Summary semver requires new major version: 1 major and 0 minor checks failed
     Warning produced 1 major and 0 minor level warnings
    Finished [ 122.007s] datafusion-ffi
    Building datafusion-physical-expr v53.1.0 (current)
       Built [  24.727s] (current)
     Parsing datafusion-physical-expr v53.1.0 (current)
      Parsed [   0.043s] (current)
    Building datafusion-physical-expr v53.1.0 (baseline)
       Built [  25.058s] (baseline)
     Parsing datafusion-physical-expr v53.1.0 (baseline)
      Parsed [   0.044s] (baseline)
    Checking datafusion-physical-expr v53.1.0 -> v53.1.0 (no change; assume patch)
     Checked [   0.315s] 222 checks: 222 pass, 30 skip
     Summary no semver update required
    Finished [  51.800s] datafusion-physical-expr
    Building datafusion-physical-expr-common v53.1.0 (current)
       Built [  19.857s] (current)
     Parsing datafusion-physical-expr-common v53.1.0 (current)
      Parsed [   0.020s] (current)
    Building datafusion-physical-expr-common v53.1.0 (baseline)
       Built [  19.917s] (baseline)
     Parsing datafusion-physical-expr-common v53.1.0 (baseline)
      Parsed [   0.021s] (baseline)
    Checking datafusion-physical-expr-common v53.1.0 -> v53.1.0 (no change; assume patch)
     Checked [   0.209s] 222 checks: 220 pass, 2 fail, 0 warn, 30 skip

--- failure function_missing: pub fn removed or renamed ---

Description:
A publicly-visible function cannot be imported by its prior path. A `pub use` may have been removed, or the function itself may have been renamed or removed entirely.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.47.0/src/lints/function_missing.ron

Failed in:
  function datafusion_physical_expr_common::physical_expr::snapshot_physical_expr_opt, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-origin_main/1b4aa23fc54dabface2da814e74fe26e0b84c6a8/datafusion/physical-expr-common/src/physical_expr.rs:651
  function datafusion_physical_expr_common::physical_expr::snapshot_physical_expr, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-origin_main/1b4aa23fc54dabface2da814e74fe26e0b84c6a8/datafusion/physical-expr-common/src/physical_expr.rs:632

--- failure trait_method_missing: pub trait method removed or renamed ---

Description:
A trait method is no longer callable, and may have been renamed or removed entirely.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#trait-item-signature
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.47.0/src/lints/trait_method_missing.ron

Failed in:
  method snapshot of trait PhysicalExpr, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-origin_main/1b4aa23fc54dabface2da814e74fe26e0b84c6a8/datafusion/physical-expr-common/src/physical_expr.rs:400

     Summary semver requires new major version: 2 major and 0 minor checks failed
    Finished [  41.274s] datafusion-physical-expr-common
    Building datafusion-physical-optimizer v53.1.0 (current)
       Built [  38.694s] (current)
     Parsing datafusion-physical-optimizer v53.1.0 (current)
      Parsed [   0.026s] (current)
    Building datafusion-physical-optimizer v53.1.0 (baseline)
       Built [  37.180s] (baseline)
     Parsing datafusion-physical-optimizer v53.1.0 (baseline)
      Parsed [   0.022s] (baseline)
    Checking datafusion-physical-optimizer v53.1.0 -> v53.1.0 (no change; assume patch)
     Checked [   0.108s] 222 checks: 222 pass, 30 skip
     Summary no semver update required
    Finished [  77.745s] datafusion-physical-optimizer
    Building datafusion-pruning v53.1.0 (current)
       Built [  36.332s] (current)
     Parsing datafusion-pruning v53.1.0 (current)
      Parsed [   0.011s] (current)
    Building datafusion-pruning v53.1.0 (baseline)
       Built [  36.440s] (baseline)
     Parsing datafusion-pruning v53.1.0 (baseline)
      Parsed [   0.012s] (baseline)
    Checking datafusion-pruning v53.1.0 -> v53.1.0 (no change; assume patch)
     Checked [   0.075s] 222 checks: 222 pass, 30 skip
     Summary no semver update required
    Finished [  74.731s] datafusion-pruning

adriangb · 2026-05-01T18:29:15Z

@jayshrivastava can you rebase please?

See apache#21807 `PhysicalExpr::snapshot` isn't required because it only applies to dynamic filters. The only place that its used is in `pruning_predicate.rs`. Since there's only one use case, we can just downcast to `DynamicFilterPhysicalExpr` and call `current()`. Remove `PhysicalExpr::snapshot` and associated code. Yes, the existing tests should cover the exact same functionality. Filter pushdown still workers with dynamic filters; we use `current()` instead of `snapshot()` to capture the expression. Yes. `PhysicalExpr::snapshot()` no longer exists. Users should downcast to `DynamicFilterPhysicalExpr` and call `current()` intead of calling `snapshot()`. Similarly, in `datafusion/ffi`, `FFI_PhysicalExpr` no longer has a `snapshot()` method.

jayshrivastava · 2026-05-01T18:56:08Z

Rebased!

adriangb

I think there's little reason to not deprecate and stop using instead of removing altogether: https://datafusion.apache.org/contributor-guide/api-health.html#deprecation-guidelines

As far as I can tell it wouldn't be a big pain or impede future development much to keep these methods around as deprecated?

github-actions Bot added physical-expr Changes to the physical-expr crates proto Related to proto crate ffi Changes to the ffi crate labels May 1, 2026

jayshrivastava force-pushed the js/remove-snapshot-method-from-physical-expr branch from 682e97c to 9aec9f2 Compare May 1, 2026 17:05

github-actions Bot added the optimizer Optimizer rules label May 1, 2026

jayshrivastava changed the title ~~Js/remove snapshot method from physical expr~~ physical-expr-common: remote PhysicalExpr::snapshot method May 1, 2026

jayshrivastava mentioned this pull request May 1, 2026

proto: serialize and dedupe dynamic filters v2 #21807

Merged

jayshrivastava force-pushed the js/remove-snapshot-method-from-physical-expr branch from 9aec9f2 to 988ae5d Compare May 1, 2026 18:55

jayshrivastava marked this pull request as ready for review May 1, 2026 18:56

github-actions Bot removed the proto Related to proto crate label May 1, 2026

adriangb requested changes May 1, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

physical-expr-common: remote `PhysicalExpr::snapshot` method#21975

physical-expr-common: remote `PhysicalExpr::snapshot` method#21975
jayshrivastava wants to merge 1 commit intoapache:mainfrom
jayshrivastava:js/remove-snapshot-method-from-physical-expr

jayshrivastava commented May 1, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented May 1, 2026 •

edited

Loading

Uh oh!

adriangb commented May 1, 2026

Uh oh!

jayshrivastava commented May 1, 2026

Uh oh!

adriangb left a comment •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

jayshrivastava commented May 1, 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

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

github-actions Bot commented May 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adriangb commented May 1, 2026

Uh oh!

jayshrivastava commented May 1, 2026

Uh oh!

adriangb 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

jayshrivastava commented May 1, 2026 •

edited

Loading

github-actions Bot commented May 1, 2026 •

edited

Loading

adriangb left a comment •

edited

Loading