berkaysynnada commented on code in PR #7812:
URL: https://github.com/apache/arrow-datafusion/pull/7812#discussion_r1356850485


##########
datafusion/physical-expr/src/physical_expr.rs:
##########
@@ -75,15 +75,49 @@ pub trait PhysicalExpr: Send + Sync + Display + Debug + 
PartialEq<dyn Any> {
         children: Vec<Arc<dyn PhysicalExpr>>,
     ) -> Result<Arc<dyn PhysicalExpr>>;
 
-    /// Computes bounds for the expression using interval arithmetic.
+    /// Computes the output interval for the expression, given the input
+    /// intervals.
+    ///
+    /// # Arguments
+    ///
+    /// * `children` are the intervals for the children (inputs) of this
+    /// expression.
+    ///
+    /// # Example
+    ///
+    /// If the expression is `a + b`, and the input intervals are `a: [1, 2]`
+    /// and `b: [3, 4]`, then the output interval would be `[4, 6]`.
     fn evaluate_bounds(&self, _children: &[&Interval]) -> Result<Interval> {
         not_impl_err!("Not implemented for {self}")
     }
 
-    /// Updates/shrinks bounds for the expression using interval arithmetic.
+    /// Updates bounds for child expressions, given a known interval for this
+    /// expression.
+    ///
+    /// This is used to propagate constraints down through an
+    /// expression tree.
+    ///
+    /// # Arguments
+    ///
+    /// * `interval` is the currently known interval for this expression.
+    /// * `children` are the current intervals for the children of this 
expression
+    ///
+    /// # Returns
+    ///
+    /// A Vec of new intervals for the children, in order.
+    ///
     /// If constraint propagation reveals an infeasibility, returns [None] for
-    /// the child causing infeasibility. If none of the children intervals
-    /// change, may return an empty vector instead of cloning `children`.
+    /// the child causing infeasibility.
+    ///
+    /// If none of the child intervals change as a result of propagation, may
+    /// return an empty vector instead of cloning `children`.
+    ///
+    /// # Example

Review Comment:
   The example you gave is clear and correct, thank you ☺️ 
   For more detail:
   - For plus operation, specifically, we would first do
   - [a_lower, a_upper] <- ([expression_lower, expression_upper] - [b_lower, 
b_upper]) ∩ [a_lower, a_upper], and then
   - [b_lower, b_upper] <- ([expression_lower, expression_upper] -  [a_lower, 
a_upper]) ∩ [b_lower, b_upper].



##########
datafusion/physical-expr/src/physical_expr.rs:
##########
@@ -75,15 +75,49 @@ pub trait PhysicalExpr: Send + Sync + Display + Debug + 
PartialEq<dyn Any> {
         children: Vec<Arc<dyn PhysicalExpr>>,
     ) -> Result<Arc<dyn PhysicalExpr>>;
 
-    /// Computes bounds for the expression using interval arithmetic.
+    /// Computes the output interval for the expression, given the input
+    /// intervals.
+    ///
+    /// # Arguments
+    ///
+    /// * `children` are the intervals for the children (inputs) of this
+    /// expression.
+    ///
+    /// # Example
+    ///
+    /// If the expression is `a + b`, and the input intervals are `a: [1, 2]`
+    /// and `b: [3, 4]`, then the output interval would be `[4, 6]`.
     fn evaluate_bounds(&self, _children: &[&Interval]) -> Result<Interval> {
         not_impl_err!("Not implemented for {self}")
     }
 
-    /// Updates/shrinks bounds for the expression using interval arithmetic.
+    /// Updates bounds for child expressions, given a known interval for this
+    /// expression.
+    ///
+    /// This is used to propagate constraints down through an
+    /// expression tree.
+    ///
+    /// # Arguments
+    ///
+    /// * `interval` is the currently known interval for this expression.
+    /// * `children` are the current intervals for the children of this 
expression
+    ///
+    /// # Returns
+    ///
+    /// A Vec of new intervals for the children, in order.
+    ///
     /// If constraint propagation reveals an infeasibility, returns [None] for
-    /// the child causing infeasibility. If none of the children intervals
-    /// change, may return an empty vector instead of cloning `children`.
+    /// the child causing infeasibility.
+    ///
+    /// If none of the child intervals change as a result of propagation, may
+    /// return an empty vector instead of cloning `children`.
+    ///
+    /// # Example

Review Comment:
   The example you gave is clear and correct, thank you ☺️ 
   For more detail:
   - For plus operation, specifically, we would first do
   - [a_lower, a_upper] <- ([expression_lower, expression_upper] - [b_lower, 
b_upper]) ∩ [a_lower, a_upper], and then
   - [b_lower, b_upper] <- ([expression_lower, expression_upper] -  [a_lower, 
a_upper]) ∩ [b_lower, b_upper].



-- 
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]

Reply via email to