From 00cae598edfd054119e16b0206a252cee460f919 Mon Sep 17 00:00:00 2001 From: Jayant Shrivastava Date: Thu, 9 Jul 2026 15:42:16 +0000 Subject: [PATCH 1/3] Remove unstable public methods for `DynamicFilterPhysicalExpr` after proto migration See https://github.com/apache/datafusion/issues/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] { &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]> { self.remapped_children.as_deref() } ``` --- .../src/expressions/dynamic_filters/mod.rs | 59 +++++-------------- .../physical-expr/src/expressions/mod.rs | 1 - 2 files changed, 14 insertions(+), 46 deletions(-) diff --git a/datafusion/physical-expr/src/expressions/dynamic_filters/mod.rs b/datafusion/physical-expr/src/expressions/dynamic_filters/mod.rs index ce81a22094b72..6d1e7ddf73028 100644 --- a/datafusion/physical-expr/src/expressions/dynamic_filters/mod.rs +++ b/datafusion/physical-expr/src/expressions/dynamic_filters/mod.rs @@ -93,17 +93,17 @@ pub struct DynamicFilterPhysicalExpr { /// `datafusion-proto` can read and rebuild this state. Do not treat this type /// or its layout as a stable API. #[derive(Clone, Debug)] -pub struct Inner { +struct Inner { /// A unique identifier for the expression. - pub expression_id: u64, + expression_id: u64, /// A counter that gets incremented every time the expression is updated so that we can track changes cheaply. /// This is used for [`PhysicalExpr::snapshot_generation`] to have a cheap check for changes. - pub generation: u64, - pub expr: Arc, + generation: u64, + expr: Arc, /// Flag for quick synchronous check if filter is complete. /// This is redundant with the watch channel state, but allows us to return immediately /// from `wait_complete()` without subscribing if already complete. - pub is_complete: bool, + is_complete: bool, } impl Inner { @@ -393,29 +393,9 @@ impl DynamicFilterPhysicalExpr { write!(f, " ]") } - /// 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] { - &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]> { - self.remapped_children.as_deref() - } - /// Rebuild a `DynamicFilterPhysicalExpr` from its stored parts. Used by /// proto deserialization. - /// - /// **Warning:** intended only for `datafusion-proto` (de)serialization. - /// Not a stable API. - pub fn from_parts( + fn from_parts( children: Vec>, remapped_children: Option>>, inner: Inner, @@ -440,14 +420,6 @@ impl DynamicFilterPhysicalExpr { nullable: Arc::new(RwLock::new(None)), } } - - /// Return a clone of the atomically-captured `Inner` state. - /// - /// **Warning:** intended only for `datafusion-proto` (de)serialization. - /// Not a stable API. - pub fn inner(&self) -> Inner { - self.inner.read().clone() - } } impl PhysicalExpr for DynamicFilterPhysicalExpr { @@ -1210,22 +1182,19 @@ mod test { // Capture the parts and reconstruct. `expression_id` rides in `inner`. let reconstructed = DynamicFilterPhysicalExpr::from_parts( - reassigned.original_children().to_vec(), - reassigned.remapped_children().map(|r| r.to_vec()), - reassigned.inner(), + reassigned.children.to_vec(), + reassigned.remapped_children.as_ref().map(|r| r.to_vec()), + reassigned.inner.read().clone(), ); + assert_eq!(reassigned.children, reconstructed.children); assert_eq!( - reassigned.original_children(), - reconstructed.original_children(), - ); - assert_eq!( - reassigned.remapped_children(), - reconstructed.remapped_children(), + reassigned.remapped_children, + reconstructed.remapped_children, ); assert_eq!(reassigned.expression_id(), reconstructed.expression_id()); - let r = reassigned.inner(); - let c = reconstructed.inner(); + let r = reassigned.inner.read().clone(); + let c = reconstructed.inner.read().clone(); assert_eq!(r.generation, c.generation); assert_eq!(r.is_complete, c.is_complete); assert_eq!(format!("{:?}", r.expr), format!("{:?}", c.expr)); diff --git a/datafusion/physical-expr/src/expressions/mod.rs b/datafusion/physical-expr/src/expressions/mod.rs index 05a04f88dcadf..035dd5d5072b0 100644 --- a/datafusion/physical-expr/src/expressions/mod.rs +++ b/datafusion/physical-expr/src/expressions/mod.rs @@ -47,7 +47,6 @@ pub use column::{Column, col, with_new_schema}; pub use datafusion_expr::utils::format_state_name; pub use dynamic_filters::{ DynamicFilterPhysicalExpr, DynamicFilterTracker, DynamicFilterTracking, - Inner as DynamicFilterInner, }; pub use in_list::{InListExpr, in_list}; pub use is_not_null::{IsNotNullExpr, is_not_null}; From 2c83c64dac4ec18e9d046cbe00c3c7af5bb5af45 Mon Sep 17 00:00:00 2001 From: Jayant Shrivastava Date: Thu, 9 Jul 2026 16:01:24 +0000 Subject: [PATCH 2/3] remove warning comment --- .../physical-expr/src/expressions/dynamic_filters/mod.rs | 4 ---- 1 file changed, 4 deletions(-) diff --git a/datafusion/physical-expr/src/expressions/dynamic_filters/mod.rs b/datafusion/physical-expr/src/expressions/dynamic_filters/mod.rs index 6d1e7ddf73028..dbea192d4947d 100644 --- a/datafusion/physical-expr/src/expressions/dynamic_filters/mod.rs +++ b/datafusion/physical-expr/src/expressions/dynamic_filters/mod.rs @@ -88,10 +88,6 @@ pub struct DynamicFilterPhysicalExpr { /// `expression_id` lives here because it identifies the actual filter expression `expr`. /// Derived `DynamicFilterPhysicalExpr`s (e.g. via [`PhysicalExpr::with_new_children`]) are /// the same logical filter and must report the same `expression_id`. -/// -/// **Warning:** exposed publicly solely so that proto (de)serialization in -/// `datafusion-proto` can read and rebuild this state. Do not treat this type -/// or its layout as a stable API. #[derive(Clone, Debug)] struct Inner { /// A unique identifier for the expression. From a59f8124ce34c55c0cec0a064802fa28c9f35c7a Mon Sep 17 00:00:00 2001 From: Jayant Shrivastava Date: Thu, 9 Jul 2026 16:44:35 +0000 Subject: [PATCH 3/3] chore: retrigger CI