This is an automated email from the ASF dual-hosted git repository.
github-merge-queue[bot] pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/datafusion.git
The following commit(s) were added to refs/heads/main by this push:
new e1b773a29f Remove unstable public methods for
`DynamicFilterPhysicalExpr` after proto migration (#23423)
e1b773a29f is described below
commit e1b773a29f7be3e9b841bfc1cfd02d246bed1195
Author: Jayant Shrivastava <[email protected]>
AuthorDate: Thu Jul 9 13:19:44 2026 -0400
Remove unstable public methods for `DynamicFilterPhysicalExpr` after proto
migration (#23423)
## Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and
enhancements and this helps us generate change logs for our releases.
You can link an issue to this PR using the GitHub syntax. For example
`Closes #123` indicates that this PR will close issue #123.
-->
- Closes https://github.com/apache/datafusion/issues/23422
- Closes https://github.com/apache/datafusion/issues/22434
## Rationale for this change
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<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.
---
.../src/expressions/dynamic_filters/mod.rs | 63 +++++-----------------
datafusion/physical-expr/src/expressions/mod.rs | 1 -
2 files changed, 14 insertions(+), 50 deletions(-)
diff --git a/datafusion/physical-expr/src/expressions/dynamic_filters/mod.rs
b/datafusion/physical-expr/src/expressions/dynamic_filters/mod.rs
index ce81a22094..dbea192d49 100644
--- a/datafusion/physical-expr/src/expressions/dynamic_filters/mod.rs
+++ b/datafusion/physical-expr/src/expressions/dynamic_filters/mod.rs
@@ -88,22 +88,18 @@ 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)]
-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<dyn PhysicalExpr>,
+ generation: u64,
+ expr: Arc<dyn PhysicalExpr>,
/// 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 +389,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<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()
- }
-
/// 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<Arc<dyn PhysicalExpr>>,
remapped_children: Option<Vec<Arc<dyn PhysicalExpr>>>,
inner: Inner,
@@ -440,14 +416,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 +1178,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 05a04f88dc..035dd5d507 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};
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
