Skip to content

fix(transform): avoid overflow in adjust_boundary at type extremes #3645

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
hsdfat wants to merge 1 commit into
base: main
Choose a base branch
from
+150 −15
Open

fix(transform): avoid overflow in adjust_boundary at type extremes #3645

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
165 changes: 150 additions & 15 deletions crates/iceberg/src/spec/transform.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()),
},
Comment on lines 779 to 828

@mbutrovich mbutrovich Nov 6, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Expand Down Expand Up @@ -1194,7 +1204,11 @@ enum AdjustedProjection {

#[cfg(test)]
mod tests {
use std::sync::Arc;

use super::*;
use crate::expr::accessor::StructAccessor;
use crate::spec::NestedField;

fn check_boundary(op: PredicateOperator, input: Datum, expected: Datum) {
let result = Transform::adjust_boundary(&op, &input).unwrap().unwrap();
Expand Down Expand Up @@ -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);
}
}
Comment on lines +1339 to +1367

@mbutrovich mbutrovich Nov 6, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


/// Renders `ordinal` through the public API with the given declared type.
fn human(transform: Transform, primitive: PrimitiveType, ordinal: i32) -> String {
transform.to_human_string(&Type::Primitive(primitive), Some(&Literal::int(ordinal)))
Expand Down
Loading