physical-expr-common: deprecate PhysicalExpr::snapshot method

[jayshrivastava]

Which issue does this PR close?

See #21807

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 marks PhysicalExpr::snapshot, snapshot_physical_expr, and snapshot_physical_expr_opt as
deprecated. Callers should 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?

PhysicalExpr::snapshot, snapshot_physical_expr, and snapshot_physical_expr_opt
are deprecated and should not be used.

[github-actions]

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 apache/main
Building datafusion-ffi v53.1.0 (current)
Built [ 60.141s] (current)
Parsing datafusion-ffi v53.1.0 (current)
Parsed [ 0.058s] (current)
Building datafusion-ffi v53.1.0 (baseline)
Built [ 59.338s] (baseline)
Parsing datafusion-ffi v53.1.0 (baseline)
Parsed [ 0.059s] (baseline)
Checking datafusion-ffi v53.1.0 -> v53.1.0 (no change; assume patch)
Checked [ 0.285s] 222 checks: 222 pass, 30 skip
Summary no semver update required
Finished [ 121.689s] datafusion-ffi
Building datafusion-physical-expr v53.1.0 (current)
Built [ 25.325s] (current)
Parsing datafusion-physical-expr v53.1.0 (current)
Parsed [ 0.047s] (current)
Building datafusion-physical-expr v53.1.0 (baseline)
Built [ 24.774s] (baseline)
Parsing datafusion-physical-expr v53.1.0 (baseline)
Parsed [ 0.046s] (baseline)
Checking datafusion-physical-expr v53.1.0 -> v53.1.0 (no change; assume patch)
Checked [ 0.415s] 222 checks: 222 pass, 30 skip
Summary no semver update required
Finished [ 51.638s] datafusion-physical-expr
Building datafusion-physical-expr-common v53.1.0 (current)
Built [ 19.844s] (current)
Parsing datafusion-physical-expr-common v53.1.0 (current)
Parsed [ 0.021s] (current)
Building datafusion-physical-expr-common v53.1.0 (baseline)
Built [ 19.530s] (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.250s] 222 checks: 220 pass, 2 fail, 0 warn, 30 skip
--- failure function_marked_deprecated: function #[deprecated] added ---
Description:
A function is now #[deprecated]. Downstream crates will get a compiler warning when using this function.
ref: https://doc.rust-lang.org/reference/attributes/diagnostics.html#the-deprecated-attribute
impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.47.0/src/lints/function_marked_deprecated.ron
Failed in:
function datafusion_physical_expr_common::physical_expr::snapshot_physical_expr_opt in /home/runner/work/datafusion/datafusion/datafusion/physical-expr-common/src/physical_expr.rs:597
function datafusion_physical_expr_common::physical_expr::snapshot_physical_expr in /home/runner/work/datafusion/datafusion/datafusion/physical-expr-common/src/physical_expr.rs:586
--- failure trait_method_marked_deprecated: trait method #[deprecated] added ---
Description:
A trait method is now #[deprecated]. Downstream crates will get a compiler warning when using this method.
ref: https://doc.rust-lang.org/reference/attributes/diagnostics.html#the-deprecated-attribute
impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.47.0/src/lints/trait_method_marked_deprecated.ron
Failed in:
method snapshot in trait datafusion_physical_expr_common::physical_expr::PhysicalExpr in /home/runner/work/datafusion/datafusion/datafusion/physical-expr-common/src/physical_expr.rs:74
Summary semver requires new minor version: 0 major and 2 minor checks failed
Finished [ 40.711s] datafusion-physical-expr-common
Building datafusion-physical-optimizer v53.1.0 (current)
Built [ 37.012s] (current)
Parsing datafusion-physical-optimizer v53.1.0 (current)
Parsed [ 0.024s] (current)
Building datafusion-physical-optimizer v53.1.0 (baseline)
Built [ 39.422s] (baseline)
Parsing datafusion-physical-optimizer v53.1.0 (baseline)
Parsed [ 0.024s] (baseline)
Checking datafusion-physical-optimizer v53.1.0 -> v53.1.0 (no change; assume patch)
Checked [ 0.135s] 222 checks: 222 pass, 30 skip
Summary no semver update required
Finished [ 78.001s] datafusion-physical-optimizer
Building datafusion-pruning v53.1.0 (current)
Built [ 37.564s] (current)
Parsing datafusion-pruning v53.1.0 (current)
Parsed [ 0.012s] (current)
Building datafusion-pruning v53.1.0 (baseline)
Built [ 36.128s] (baseline)
Parsing datafusion-pruning v53.1.0 (baseline)
Parsed [ 0.013s] (baseline)
Checking datafusion-pruning v53.1.0 -> v53.1.0 (no change; assume patch)
Checked [ 0.097s] 222 checks: 222 pass, 30 skip
Summary no semver update required
Finished [ 75.037s] datafusion-pruning

[adriangb]

@jayshrivastava can you rebase please?

[jayshrivastava]

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?

[jayshrivastava]

I did not remove/edit

1. Tests that call snapshot()
2. Implementations of snapshot() ex. for DynamicFilterPhysicalExpr
   Please let me know if this is the correct way to deprecate things!

[adriangb]

Yes I'd say that's right!

[adriangb]

Thanks @jayshrivastava !

My main concern with merging this right now is that it doesn't seem like a big pain point / blocker anymore to have this API. And removing it is code churn. Especially if we find a user for it later. I'm thinking in particular about other "thin, runtime only" expression wrappers, e.g. AdaptiveSelectivityFilterExpr in https://github.com/apache/datafusion/pull/20160/changes#diff-6bf8881dc0e8a0da8ea68022633bbdc4a42e91287c059f6bc55050586d184bd7. I feel there may even be others that already exist in DataFusion that we just haven't wired up 🤔

[jayshrivastava]

My main concern with merging this right now is that it doesn't seem like a big pain point / blocker anymore to have this API

I agree. It was a small PR so I went ahead and implemented it. It's not a blocker for me. If you feel that it's better to leave it unmerged, I'm okay with that!

[jayshrivastava]

Closed as per the above comment!