Repository navigation
Conversation
A strict comparison past an integer, date or timestamp type's minimum or maximum now yields no boundary instead of overflowing, so inclusive projection returns no predicate rather than panicking. Part of apache#3637
mbutrovich
left a comment
mbutrovich
left a comment
There was a problem hiding this comment.
Thanks @hsdfat for picking this up.
On the question in the description, returning None looks right to me. InclusiveProjection skips a part with no projection, so the partition filter stays true. The inclusive projection rule only requires the partition predicate to match every row the scan predicate matches, and x < i32::MIN matches none.
| 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()), | ||
| }, |
There was a problem hiding this 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 and try_decrement_number type for type, and those helpers still compute v + 1 and v - 1 unchecked. truncate_number_strict 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:
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 starts from AlwaysFalse and skips a part with no projection. Strict projection 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.
| 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); | ||
| } | ||
| } |
There was a problem hiding this comment.
Could this test move next to the other projection tests in transform/temporal.rs and transform/truncate.rs, using TestProjectionFixture? Its binary_predicate builds the same BoundPredicate this loop builds by hand, and assert_projection 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.
Which issue does this PR close?
adjust_boundaryoverflow. The year/month transform range problem is left out, because it overlaps open PR fix(transform): compute year and month in UTC #3237.What changes are included in this PR?
Transform::adjust_boundaryturns< vinto<= v - 1and> vinto>= v + 1with unchecked arithmetic. At a type's minimum or maximum, that panics in debug builds and wraps in release. For int, long, date and the four timestamp types, the shift now useschecked_sub/checked_add. On overflow it returnsOk(None). Callers already treatNoneas "no projection", and the predicate can't match any value anyway (nothing is belowi32::MIN). The decimal arms are unchanged; they already go throughtry_decrement_number/try_increment_number.Are these changes tested?
Yes, with unit tests in
spec/transform.rs:test_adjust_boundary_overflow_returns_nonecovers<at MIN and>at MAX for each affected type.test_adjust_boundary_near_extremeschecks that MIN+1 and MAX-1 still shift normally.test_project_strict_comparison_past_extreme_returns_nonegoes throughTransform::projectfor a day transform on a date and a truncate transform on a long. Both panicked before this change.AI Disclosure
The change and tests were drafted with AI assistance (Claude). I reviewed every line and ran the transform tests, clippy and fmt. The point a reviewer may want to check: on overflow this returns
None(no projection) rather than an always-false predicate. That seemed the smallest change, since the caller already handlesNone.