isidentical commented on code in PR #3912:
URL: https://github.com/apache/arrow-datafusion/pull/3912#discussion_r1002134012
##########
datafusion/physical-expr/src/expressions/binary.rs:
##########
@@ -771,32 +745,47 @@ fn compare_left_boundaries(
// it is actually smaller than what we have right now) and it is a
valid
// value (e.g. [0, 100] < -100 would update the boundaries to [0,
-100] if
// there weren't the selectivity check).
- if rhs_scalar_value < variadic_max && selectivity > 0.0 {
- (variadic_min, rhs_scalar_value)
+ if right < left_max && selectivity > 0.0 {
+ (left_min, right)
} else {
- (variadic_min, variadic_max)
+ (left_min, left_max)
}
}
Operator::Gt | Operator::GtEq => {
// Same as above, but this time we want to limit the lower bound.
- if rhs_scalar_value > variadic_min && selectivity > 0.0 {
- (rhs_scalar_value, variadic_max)
+ if right > left_min && selectivity > 0.0 {
+ (right, left_max)
} else {
- (variadic_min, variadic_max)
+ (left_min, left_max)
}
}
// For equality, we don't have the range problem so even if the
selectivity
// is 0.0, we can still update the boundaries.
- Operator::Eq => (rhs_scalar_value.clone(), rhs_scalar_value),
+ Operator::Eq => (right.clone(), right),
_ => unreachable!(),
};
- Some(ExprBoundaries {
- min_value: new_min,
- max_value: new_max,
- distinct_count,
- selectivity: Some(selectivity),
- })
+ // The context represents all the knowledge we have gathered during the
+ // analysis process, which we can now add more since the expression's upper
+ // and lower boundaries might have changed.
+ let left_bounds = ExprBoundaries::new(left_min, left_max,
left_bounds.distinct_count);
+ left.apply(context, &left_bounds);
Review Comment:
Slight note (after reading your comment and mine again): `apply()` is not a
function with expression level side effects. It changes stuff in the given
context. So if the `OR` passes two different contexts to its two sides, then
they will each make changes in their own isolated context (which after the
analysis is over, we can actually see and try to merge inside `OR`'s `analyze`
implementation).
--
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]
