mbutrovich commented on code in PR #3645:
URL: https://github.com/apache/iceberg-rust/pull/3645#discussion_r4496302567
##########
crates/iceberg/src/spec/transform.rs:
##########
@@ -1231,6 +1245,127 @@ mod tests {
}
}
+ #[test]
+ fn test_adjust_boundary_overflow_returns_none() {
+ let cases = [
+ (PredicateOperator::LessThan, Datum::int(i32::MIN)),
+ (PredicateOperator::GreaterThan, Datum::int(i32::MAX)),
+ (PredicateOperator::LessThan, Datum::date(i32::MIN)),
+ (PredicateOperator::GreaterThan, Datum::date(i32::MAX)),
+ (PredicateOperator::LessThan, Datum::long(i64::MIN)),
+ (PredicateOperator::GreaterThan, Datum::long(i64::MAX)),
+ (
+ PredicateOperator::LessThan,
+ Datum::timestamp_micros(i64::MIN),
+ ),
+ (
+ PredicateOperator::GreaterThan,
+ Datum::timestamp_micros(i64::MAX),
+ ),
+ (
+ PredicateOperator::LessThan,
+ Datum::timestamptz_micros(i64::MIN),
+ ),
+ (
+ PredicateOperator::GreaterThan,
+ Datum::timestamptz_micros(i64::MAX),
+ ),
+ (
+ PredicateOperator::LessThan,
+ Datum::timestamp_nanos(i64::MIN),
+ ),
+ (
+ PredicateOperator::GreaterThan,
+ Datum::timestamp_nanos(i64::MAX),
+ ),
+ (
+ PredicateOperator::LessThan,
+ Datum::timestamptz_nanos(i64::MIN),
+ ),
+ (
+ PredicateOperator::GreaterThan,
+ Datum::timestamptz_nanos(i64::MAX),
+ ),
+ ];
+ for (op, datum) in cases {
+ assert_eq!(
+ Transform::adjust_boundary(&op, &datum).unwrap(),
+ None,
+ "{op} {datum}"
+ );
+ }
+ }
+
+ #[test]
+ fn test_adjust_boundary_near_extremes() {
+ let cases = [
+ (
+ PredicateOperator::LessThan,
+ Datum::int(i32::MIN + 1),
+ Datum::int(i32::MIN),
+ ),
+ (
+ PredicateOperator::GreaterThan,
+ Datum::int(i32::MAX - 1),
+ Datum::int(i32::MAX),
+ ),
+ (
+ PredicateOperator::LessThan,
+ Datum::date(i32::MIN + 1),
+ Datum::date(i32::MIN),
+ ),
+ (
+ PredicateOperator::GreaterThan,
+ Datum::long(i64::MAX - 1),
+ Datum::long(i64::MAX),
+ ),
+ (
+ PredicateOperator::LessThan,
+ Datum::timestamp_nanos(i64::MIN + 1),
+ Datum::timestamp_nanos(i64::MIN),
+ ),
+ (
+ PredicateOperator::GreaterThan,
+ Datum::timestamp_micros(i64::MAX - 1),
+ Datum::timestamp_micros(i64::MAX),
+ ),
+ ];
+ for (op, input, expected) in cases {
+ check_boundary(op, input, expected);
+ }
+ }
+
+ #[test]
+ fn test_project_strict_comparison_past_extreme_returns_none() {
+ let cases = [
+ (
+ Transform::Day,
+ PrimitiveType::Date,
+ PredicateOperator::LessThan,
+ Datum::date(i32::MIN),
+ ),
+ (
+ Transform::Truncate(10),
+ PrimitiveType::Long,
+ PredicateOperator::GreaterThan,
+ Datum::long(i64::MAX),
+ ),
+ ];
+ for (transform, field_type, op, literal) in cases {
+ let field = Arc::new(NestedField::optional(1, "col",
Type::Primitive(field_type)));
+ let predicate = BoundPredicate::Binary(BinaryExpression::new(
+ op,
+ BoundReference::new(
+ "col",
+ field,
+ Arc::new(StructAccessor::new(1, PrimitiveType::Boolean)),
+ ),
+ literal,
+ ));
+ assert_eq!(transform.project("col", &predicate).unwrap(), None);
+ }
+ }
Review Comment:
Could this test move next to the other projection tests in
`transform/temporal.rs` and `transform/truncate.rs`, using
`TestProjectionFixture`? Its
[`binary_predicate`](https://github.com/apache/iceberg-rust/blob/5370b61638e205d7caf97889affe5da63aea9b16/crates/iceberg/src/transform/mod.rs#L106-L120)
builds the same `BoundPredicate` this loop builds by hand, and
[`assert_projection`](https://github.com/apache/iceberg-rust/blob/5370b61638e205d7caf97889affe5da63aea9b16/crates/iceberg/src/transform/mod.rs#L136-L147)
with `None` checks the result. That also drops the three imports this PR adds
to the test module (lines 1207-1211). The strict-projection cases from the
comment above would fit in the same place.
The name also says "strict", but the test calls `project`, and
`strict_project` is a separate method in this file. Something like
`test_projection_comparison_past_type_extreme` would avoid the mix-up.
##########
crates/iceberg/src/spec/transform.rs:
##########
@@ -771,48 +771,58 @@ impl Transform {
/// the literal value within the given `Datum`. For operators that do not
/// imply a boundary shift (`Eq`, `LessThanOrEq`, `GreaterThanOrEq`,
/// `StartsWith`, `NotStartsWith`), the original datum is returned
- /// unmodified.
+ /// unmodified. When the shift would pass the type's minimum or maximum,
+ /// the comparison matches no value and `None` is returned, so no
+ /// projection is made.
fn adjust_boundary(op: &PredicateOperator, datum: &Datum) ->
Result<Option<Datum>> {
let adjusted_boundary = match op {
PredicateOperator::LessThan => match (datum.data_type(),
datum.literal()) {
- (PrimitiveType::Int, PrimitiveLiteral::Int(v)) =>
Some(Datum::int(v - 1)),
- (PrimitiveType::Long, PrimitiveLiteral::Long(v)) =>
Some(Datum::long(v - 1)),
+ (PrimitiveType::Int, PrimitiveLiteral::Int(v)) =>
v.checked_sub(1).map(Datum::int),
+ (PrimitiveType::Long, PrimitiveLiteral::Long(v)) => {
+ v.checked_sub(1).map(Datum::long)
+ }
(PrimitiveType::Decimal { .. }, PrimitiveLiteral::Int128(_))
=> {
Some(Self::try_decrement_number(datum)?)
}
- (PrimitiveType::Date, PrimitiveLiteral::Int(v)) =>
Some(Datum::date(v - 1)),
+ (PrimitiveType::Date, PrimitiveLiteral::Int(v)) => {
+ v.checked_sub(1).map(Datum::date)
+ }
(PrimitiveType::Timestamp, PrimitiveLiteral::Long(v)) => {
- Some(Datum::timestamp_micros(v - 1))
+ v.checked_sub(1).map(Datum::timestamp_micros)
}
(PrimitiveType::Timestamptz, PrimitiveLiteral::Long(v)) => {
- Some(Datum::timestamptz_micros(v - 1))
+ v.checked_sub(1).map(Datum::timestamptz_micros)
}
(PrimitiveType::TimestampNs, PrimitiveLiteral::Long(v)) => {
- Some(Datum::timestamp_nanos(v - 1))
+ v.checked_sub(1).map(Datum::timestamp_nanos)
}
(PrimitiveType::TimestamptzNs, PrimitiveLiteral::Long(v)) => {
- Some(Datum::timestamptz_nanos(v - 1))
+ v.checked_sub(1).map(Datum::timestamptz_nanos)
}
_ => Some(datum.to_owned()),
},
PredicateOperator::GreaterThan => match (datum.data_type(),
datum.literal()) {
- (PrimitiveType::Int, PrimitiveLiteral::Int(v)) =>
Some(Datum::int(v + 1)),
- (PrimitiveType::Long, PrimitiveLiteral::Long(v)) =>
Some(Datum::long(v + 1)),
+ (PrimitiveType::Int, PrimitiveLiteral::Int(v)) =>
v.checked_add(1).map(Datum::int),
+ (PrimitiveType::Long, PrimitiveLiteral::Long(v)) => {
+ v.checked_add(1).map(Datum::long)
+ }
(PrimitiveType::Decimal { .. }, PrimitiveLiteral::Int128(_))
=> {
Some(Self::try_increment_number(datum)?)
}
- (PrimitiveType::Date, PrimitiveLiteral::Int(v)) =>
Some(Datum::date(v + 1)),
+ (PrimitiveType::Date, PrimitiveLiteral::Int(v)) => {
+ v.checked_add(1).map(Datum::date)
+ }
(PrimitiveType::Timestamp, PrimitiveLiteral::Long(v)) => {
- Some(Datum::timestamp_micros(v + 1))
+ v.checked_add(1).map(Datum::timestamp_micros)
}
(PrimitiveType::Timestamptz, PrimitiveLiteral::Long(v)) => {
- Some(Datum::timestamptz_micros(v + 1))
+ v.checked_add(1).map(Datum::timestamptz_micros)
}
(PrimitiveType::TimestampNs, PrimitiveLiteral::Long(v)) => {
- Some(Datum::timestamp_nanos(v + 1))
+ v.checked_add(1).map(Datum::timestamp_nanos)
}
(PrimitiveType::TimestamptzNs, PrimitiveLiteral::Long(v)) => {
- Some(Datum::timestamptz_nanos(v + 1))
+ v.checked_add(1).map(Datum::timestamptz_nanos)
}
_ => Some(datum.to_owned()),
},
Review Comment:
Could the checked arithmetic go into `try_decrement_number` and
`try_increment_number` instead? Apart from the decimal case, these two arms
repeat
[`try_increment_number`](https://github.com/apache/iceberg-rust/blob/ce2fd6804144f1edcb0d68cb271466c24b27e11c/crates/iceberg/src/spec/transform.rs#L957-L990)
and
[`try_decrement_number`](https://github.com/apache/iceberg-rust/blob/ce2fd6804144f1edcb0d68cb271466c24b27e11c/crates/iceberg/src/spec/transform.rs#L995-L1028)
type for type, and those helpers still compute `v + 1` and `v - 1` unchecked.
[`truncate_number_strict`](https://github.com/apache/iceberg-rust/blob/ce2fd6804144f1edcb0d68cb271466c24b27e11c/crates/iceberg/src/spec/transform.rs#L1060-L1074)
calls them for `<=` and `>=`, so strict projection hits the same overflow. At
the head commit, `Transform::strict_project` panics in a debug build for
`truncate[10]` with `x <= i32::MAX` and `x >= i64::MIN`, and for `day` with `x
<= date(i32::MAX)` and `x >= timestamp_micros(i64::MIN)`.
If both helpers return `Result<Option<Datum>>` with `None` on overflow, the
two arms here become:
```rust
PredicateOperator::LessThan => Self::try_decrement_number(datum)?,
PredicateOperator::GreaterThan => Self::try_increment_number(datum)?,
```
The decimal case can use `v.checked_add(1).map(|m|
datum.with_decimal_mantissa(m)).transpose()`. `truncate_number_strict` then
returns `Ok(None)` when a helper does. `None` is safe on the strict side too,
because
[`StrictProjection`](https://github.com/apache/iceberg-rust/blob/5370b61638e205d7caf97889affe5da63aea9b16/crates/iceberg/src/expr/visitors/strict_projection.rs#L74-L92)
starts from `AlwaysFalse` and skips a part with no projection. [Strict
projection](https://github.com/apache/iceberg/blob/9203eaff9eb97fef9d1f5a3ef4460e8453adaed9/format/spec.md?plain=1#L1421)
may then fail to report that every row of a file matches, but it never reports
a match that's wrong. This also removes the 14 arms the PR adds here.
--
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]
