jayzhan211 commented on PR #15685: URL: https://github.com/apache/datafusion/pull/15685#issuecomment-2798405570
https://github.com/apache/datafusion/pull/15568#discussion_r2038773841 # why the change is equivalent to your in the high level idea. > 1. DynamicFilterPhysicalExpr gets initialized at planning time with a known set of children but a placeholder expression (lit(true)) The same. > 2. with_new_children is called making a new DynamicFilterPhysicalExpr but with the children replaced (let's ignore how that happens internally for now) We need to replace children, and we can achieve this and get the result by `snapshot` > 3. update is called on the original reference with an expression that references the original children. This is propagated to all references, including those with new children, because of the Arc<RwLock<...>>. We can keep `Arc<RwLock<T>>` and update the inner or even create another new source filter. > 4. evaluate is called on one of the references that previously had with_new_children called on it. Since update was called, which swapped out inner, the children of this new inner need to be remapped to the children that we currently expose externally. We can call `evaluate` on `snapshot` because `snapshot` is already remapped. Your version need to remap for each `evaluate` called, but `snapshot` in my version done it once, and we evaluate on it without remap. # The improvement of this chanage 1. We have correct `with_new_children` because we update the source filter now. 2. `DynamicFilterPhysicalExpr` is basically `filter expression: Arc<dyn PhysicalExpr>>`. We have a simple interface with the same capability now. # Concern 1. `reassign_predicate_columns` is replaced by `snapshot` but you think we can't do this kind of change because of API churn. I think this is not an issue because we are not using in many places. 2. Do we need Lock for source filter? I think we can create another new `DynamicFilterPhysicalExpr` at all. But maybe there is some reasons we can't, we can discuss further on this. -- 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
