adriangb commented on code in PR #15685:
URL: https://github.com/apache/datafusion/pull/15685#discussion_r2039832734
##########
datafusion/physical-expr/src/expressions/dynamic_filters.rs:
##########
@@ -105,47 +97,44 @@ impl DynamicFilterPhysicalExpr {
inner: Arc<dyn PhysicalExpr>,
) -> Self {
Self {
- // columns: children,
- // remapped_columns: None, // Initially no remapped children
- remapped_schema: None,
- // remapped_filter: None,
- inner: Arc::new(RwLock::new(inner)),
+ inner,
data_type: Arc::new(RwLock::new(None)),
nullable: Arc::new(RwLock::new(None)),
}
}
// udpate schema
- pub fn with_schema(
- &self,
- schema: SchemaRef,
- ) -> Self {
- Self {
- remapped_schema: Some(schema),
- inner: Arc::clone(&self.inner),
- data_type: Arc::clone(&self.data_type),
- nullable: Arc::clone(&self.nullable),
- }
- }
+ // pub fn with_schema(&self, schema: SchemaRef) -> Self {
+ // Self {
+ // remapped_schema: Some(schema),
+ // inner: Arc::clone(&self.inner),
+ // data_type: Arc::clone(&self.data_type),
+ // nullable: Arc::clone(&self.nullable),
+ // }
+ // }
// get the source filter
pub fn current(&self) -> Result<Arc<dyn PhysicalExpr>> {
- let inner = self
- .inner
- .read()
- .map_err(|_| {
- datafusion_common::DataFusionError::Execution(
- "Failed to acquire read lock for inner".to_string(),
- )
- })?
- .clone();
+ let inner = Arc::clone(&self.inner);
+
+ // let inner = self
+ // .inner
+ // .read()
+ // .map_err(|_| {
+ // datafusion_common::DataFusionError::Execution(
+ // "Failed to acquire read lock for inner".to_string(),
+ // )
+ // })?
+ // .clone();
Ok(inner)
}
// update source filter
- pub fn update(&self, filter: PhysicalExprRef) {
- let mut w = self.inner.write().unwrap();
- *w = filter;
+ // create a new one
+ pub fn update(&mut self, filter: PhysicalExprRef) {
+ self.inner = filter;
Review Comment:
How will writers have mutable access to this if they have to package it up
in an `Arc`?
##########
datafusion/physical-expr/src/expressions/dynamic_filters.rs:
##########
@@ -36,16 +36,8 @@ use super::Column;
/// A dynamic [`PhysicalExpr`] that can be updated by anyone with a reference
to it.
#[derive(Debug)]
pub struct DynamicFilterPhysicalExpr {
- /// The original children of this PhysicalExpr, if any.
- /// This is necessary because the dynamic filter may be initialized with a
placeholder (e.g. `lit(true)`)
- /// and later remapped to the actual expressions that are being filtered.
- /// But we need to know the children (e.g. columns referenced in the
expression) ahead of time to evaluate the expression correctly.
- // columns: Vec<Arc<dyn PhysicalExpr>>,
- // /// If any of the children were remapped / modified (e.g. to adjust for
projections) we need to keep track of the new children
- // /// so that when we update `current()` in subsequent iterations we can
re-apply the replacements.
- remapped_schema: Option<SchemaRef>,
/// The source of dynamic filters.
- inner: Arc<RwLock<PhysicalExprRef>>,
+ inner: PhysicalExprRef,
Review Comment:
@jayzhan211 how can this have multiple readers and a writer updating with
some sort of write lock?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org
