Remove unstable public methods for DynamicFilterPhysicalExpr after proto migration

[jayshrivastava]

Which issue does this PR close?

• ClosesRemove unstable public methods for DynamicFilterPhysicalExpr after proto migration #23422
• ClosesPort DynamicFilterPhysicalExpr to use try_to_proto / try_from_proto #22434

Rationale for this change

See #22418. After migrating DynamicFilterPhysicalExpr serialization from relying on leaking public internals, we can now remove these unstable methods because they aren't used outside the package.

pub struct Inner {
...
}
/// Return the filter's original children (before any remapping).
///
/// **Warning:** intended only for `datafusion-proto` (de)serialization.
/// Not a stable API.
pub fn original_children(&self) -> &[Arc<dyn PhysicalExpr>] {
&self.children
}
/// Return the filter's remapped children, if any have been set via
/// [`PhysicalExpr::with_new_children`].
///
/// **Warning:** intended only for `datafusion-proto` (de)serialization.
/// Not a stable API.
pub fn remapped_children(&self) -> Option<&[Arc<dyn PhysicalExpr>]> {
self.remapped_children.as_deref()
}

What changes are included in this PR?

See above.

Are these changes tested?

• Existing unit tests are migrated to private accessors
• There's no usage of these methods out side of datafusion/physical-expr/src/expressions/dynamic_filters

Are there any user-facing changes?

Unstable public methods and public types for DynamicFilterPhysicalExpr are private now.

[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-physical-expr v54.0.0 (current)
Built [ 34.396s] (current)
Parsing datafusion-physical-expr v54.0.0 (current)
Parsed [ 0.048s] (current)
Building datafusion-physical-expr v54.0.0 (baseline)
Built [ 26.908s] (baseline)
Parsing datafusion-physical-expr v54.0.0 (baseline)
Parsed [ 0.049s] (baseline)
Checking datafusion-physical-expr v54.0.0 -> v54.0.0 (no change; assume patch)
Checked [ 0.537s] 223 checks: 221 pass, 2 fail, 0 warn, 30 skip
--- failure inherent_method_missing: pub method removed or renamed ---
Description:
A publicly-visible method or associated fn 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.48.0/src/lints/inherent_method_missing.ron
Failed in:
DynamicFilterPhysicalExpr::original_children, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/d99ecf66a84cf963b65f867c2b81f3711ceba778/datafusion/physical-expr/src/expressions/dynamic_filters/mod.rs:400
DynamicFilterPhysicalExpr::remapped_children, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/d99ecf66a84cf963b65f867c2b81f3711ceba778/datafusion/physical-expr/src/expressions/dynamic_filters/mod.rs:409
DynamicFilterPhysicalExpr::from_parts, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/d99ecf66a84cf963b65f867c2b81f3711ceba778/datafusion/physical-expr/src/expressions/dynamic_filters/mod.rs:418
DynamicFilterPhysicalExpr::inner, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/d99ecf66a84cf963b65f867c2b81f3711ceba778/datafusion/physical-expr/src/expressions/dynamic_filters/mod.rs:448
--- failure struct_missing: pub struct removed or renamed ---
Description:
A publicly-visible struct cannot be imported by its prior path. A `pub use` may have been removed, or the struct 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.48.0/src/lints/struct_missing.ron
Failed in:
struct datafusion_physical_expr::expressions::DynamicFilterInner, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/d99ecf66a84cf963b65f867c2b81f3711ceba778/datafusion/physical-expr/src/expressions/dynamic_filters/mod.rs:96
Summary semver requires new major version: 2 major and 0 minor checks failed
Finished [ 62.957s] datafusion-physical-expr

[jayshrivastava]

Note we still have public methods like this

/// Get the dynamic filter expression for testing purposes.
/// Returns the dynamic filter expression for this hash join, if set.
pub fn dynamic_filter_expr(&self) -> Option<&Arc<DynamicFilterPhysicalExpr>> {}
/// Set the dynamic filter on this hash join.
///
/// Resets any internal state that depends on any existing dynamic filter.
///
/// Validates that the filter's children reference valid columns in
/// the probe (right) side's schema.
pub fn with_dynamic_filter_expr(
mut self,
filter: Arc<DynamicFilterPhysicalExpr>,
) -> Result<Self> {}

on dynamic filter producers like HashJoinExec.

I don't want to commit to removing those right now. They might come in handy for distributed datafusion since apply_expressions was reverted. I'm still thinking about the right APIs to have in datafusion for distibuted datafusion to access dynamic filters.

[jayshrivastava]

cc @adriangb

[adriangb]

Thanks! Since we did our best to mark these as not stable and they're relatively new I think we can make an exception to the API health policy. If this breaks things for anyone we'll apologize and add them back as deprecated. Seems worth the tradeoff.

[jayshrivastava]

Since we did our best to mark these as not stable and they're relatively new I think we can make an exception to the API health policy. If this breaks things for anyone we'll apologize and add them back as deprecated. Seems worth the tradeoff.

+1!

TYFR!