dannycjones commented on code in PR #3144:
URL: https://github.com/apache/iceberg-rust/pull/3144#discussion_r4368060668
##########
crates/iceberg/src/expr/visitors/mod.rs:
##########
@@ -27,3 +31,102 @@ pub(crate) mod rewrite_not;
pub(crate) mod row_group_metrics_evaluator;
pub(crate) mod strict_metrics_evaluator;
pub(crate) mod strict_projection;
+
+/// Returns true if any literal could match the inclusive `[lower, upper]`
range.
+/// Missing bounds are treated as unbounded on that side.
+///
+/// `(None, None)` returns true because no bound is available to prune against.
+pub(crate) fn any_literal_in_bounds(
+ lower: Option<&Datum>,
+ upper: Option<&Datum>,
+ literals: &FnvHashSet<Datum>,
+) -> bool {
+ match (lower, upper) {
+ (Some(lower), Some(upper)) => literals
+ .iter()
+ .any(|datum| datum.ge(lower) && datum.le(upper)),
+ (Some(lower), None) => literals.iter().any(|datum| datum.ge(lower)),
+ (None, Some(upper)) => literals.iter().any(|datum| datum.le(upper)),
+ (None, None) => true,
+ }
+}
Review Comment:
I think we need to be careful here. This function doesn't handle where
comparison isn't defined.
If we take the first arm: we have both an upper and lower bound defined, but
we say that it is maybe in bounds if and ONLY IF the `datum >= Some(lower
bound)` AND `datum <= Some(upper bound)`, which is a problem if the values
aren't comparable. When not comparable, those fns return false because they
can't confirm that they hold.
I think instead we should have something like this, flipping the callers.
```suggestion
/// Returns true if every literal is provably outside the inclusive `[lower,
upper]`
/// range, so rows cannot match. Missing bounds are treated as unbounded on
that side.
///
/// Returns `false` (cannot prune) when there are no bounds, or when a
literal
/// does not compare with a bound.
pub(crate) fn all_literals_outside_bounds(
lower: Option<&Datum>,
upper: Option<&Datum>,
literals: &FnvHashSet<Datum>,
) -> bool {
match (lower, upper) {
(Some(lower), Some(upper)) => literals
.iter()
.all(|datum| datum.lt(lower) || datum.gt(upper)),
(Some(lower), None) => literals.iter().all(|datum| datum.lt(lower)),
(None, Some(upper)) => literals.iter().all(|datum| datum.gt(upper)),
(None, None) => false,
}
}
```
I've been encountering pain here with new data type changes which don't
necessarily have comparison defined, leading to incorrect pruning. For example,
where we don't currently define `date` to `timestamp` promotion (funnily
enough, just opened: https://github.com/apache/iceberg-rust/pull/3522), so the
data isn't comparable. I've also had issues with geo development, although we
should start blocking that before reaching here. (I like defense in depth
though.)
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
