adriangb commented on code in PR #15770:
URL: https://github.com/apache/datafusion/pull/15770#discussion_r2051461834
##########
datafusion/physical-expr/src/expressions/mod.rs:
##########
@@ -22,7 +22,7 @@ mod binary;
mod case;
mod cast;
mod column;
-mod dynamic_filters;
+pub mod dynamic_filters;
Review Comment:
This bit has me tripped up. I'm not sure where the right place to put
`dynamic_filters` is such that it's public for our internal use in operators
but private from the outside world 🤔
##########
datafusion/physical-plan/src/sorts/sort.rs:
##########
@@ -1205,9 +1221,10 @@ impl ExecutionPlan for SortExec {
self: Arc<Self>,
children: Vec<Arc<dyn ExecutionPlan>>,
) -> Result<Arc<dyn ExecutionPlan>> {
- let new_sort = SortExec::new(self.expr.clone(),
Arc::clone(&children[0]))
+ let mut new_sort = SortExec::new(self.expr.clone(),
Arc::clone(&children[0]))
.with_fetch(self.fetch)
.with_preserve_partitioning(self.preserve_partitioning);
+ new_sort.filter = Arc::clone(&self.filter);
Review Comment:
I missed this for a while and spent an hour trying to figure out why my test
was failing. IMO we should have a test that enforces the invariant that
`ExecutionPlan::with_new_children(Arc::clone(&node), node.children()) == node`
--
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
