mingmwang commented on code in PR #3912:
URL: https://github.com/apache/arrow-datafusion/pull/3912#discussion_r1013706030
##########
datafusion/physical-expr/src/expressions/binary.rs:
##########
@@ -665,87 +656,81 @@ impl PhysicalExpr for BinaryExpr {
children[1].clone(),
)))
}
-}
-
-impl PartialEq<dyn Any> for BinaryExpr {
- fn eq(&self, other: &dyn Any) -> bool {
- down_cast_any_ref(other)
- .downcast_ref::<Self>()
- .map(|x| self.left.eq(&x.left) && self.op == x.op &&
self.right.eq(&x.right))
- .unwrap_or(false)
- }
-}
-
-struct BinaryExprStats {
- op: Operator,
- left: Arc<dyn PhysicalExpr>,
- right: Arc<dyn PhysicalExpr>,
-}
-impl PhysicalExprStats for BinaryExprStats {
- fn boundaries(&self, columns: &[ColumnStatistics]) ->
Option<ExprBoundaries> {
+ /// Return the boundaries of this binary expression's result. If the
expression itself
+ /// is a comparison which changes the boundaries of one of its inputs (a =
20 would pin
+ /// a to 20), then it might update the input's boundaries directly on the
context.
+ fn analyze(&self, context: &mut AnalysisContext) -> Option<ExprBoundaries>
{
Review Comment:
> Since AnalysisContext is in terms of the "current schema" it will likely
almost always end up being modified if the execution plan modifies the schema
(which most do).
>
> I find that the mix of "input" and "output" of a `mut &AnalysisContext` to
be confusing to explain.
>
> Since an `AnalysisContext` has `ExprBoundaries` in it anyways, I recommend
the signature be
>
> ```rust
> fn analyze(&self) -> AnalysisContext {
> ...
> }
> ```
>
> And then pass each input can produce the context / boundary analysis for
itself and the parent operators can combine them
>
> Given that the inputs will produced an owned AnalysisContext it will be
clear what is input and what is output and I think it can be made quite fast in
Rust
I would prefer this change also. It makes the signature more clear. It is
similar to the `EquivalenceProperties` in the ExecutionPlan, mixing "input" and
"output" is very confusing.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
