JeonDaehong commented on code in PR #3507:
URL: https://github.com/apache/iceberg-rust/pull/3507#discussion_r4348277198
##########
crates/iceberg/src/expr/visitors/page_index_evaluator.rs:
##########
@@ -596,7 +596,9 @@ impl<'a> PageIndexEvaluator<'a> {
let bound = if use_lower_bound { min } else { max };
if let Some(bound) = bound {
- if cmp_fn(&bound, datum) {
+ // A bound that can't be compared with the literal (e.g. a
+ // file column narrower than the field type) might match.
+ if bound.partial_cmp(datum).is_none() || cmp_fn(&bound,
datum) {
Review Comment:
#3483 has landed now, so `Datum::promote_to` is available. Widening INT32 →
`long` and FLOAT → `double` bounds here would keep pruning these pages instead
of keeping all of them. That's fine as the follow-up you describe, but see the
UINT_32 note on the test below before reusing `int32_bound_to_datum`.
##########
crates/iceberg/src/expr/visitors/page_index_evaluator.rs:
##########
@@ -937,11 +939,13 @@ impl BoundPredicateVisitor for PageIndexEvaluator<'_> {
// if all values are outside the bounds, no rows can
match
return Ok(false);
}
- (Some(min), _) if !literals.iter().any(|datum|
datum.ge(&min)) => {
+ // `all(lt)` rather than `!any(ge)`, so that a bound that
+ // can't be compared with the literals keeps the page.
+ (Some(min), _) if literals.iter().all(|datum|
datum.lt(&min)) => {
Review Comment:
Checked: for comparable bounds `all(lt)` and `!any(ge)` agree (`Datum`
floats use `total_cmp`), and an empty literal set still prunes as before, so
only the incomparable case changes.
##########
crates/iceberg/src/expr/visitors/page_index_evaluator.rs:
##########
@@ -2312,6 +2341,123 @@ mod tests {
Ok(())
}
+ #[test]
+ fn eval_keeps_int32_pages_of_promoted_long_field() -> Result<()> {
+ // An `int` -> `long` promotion leaves older files with INT32 page
+ // bounds, which can't be compared with a `long` literal.
+ let (metadata, _temp_file) = create_parquet_file_from_values(&[100,
200, 300, 400], |v| {
+ Arc::new(Int32Array::from(v))
+ })?;
+
+ assert_keeps_all_pages(&metadata, PrimitiveType::Long, 4096, [
+ Reference::new("col").less_than(Datum::long(250)),
+ Reference::new("col").greater_than_or_equal_to(Datum::long(250)),
+ // A literal outside the `int` range.
+ Reference::new("col").less_than(Datum::long(i64::from(i32::MAX) +
1)),
+ Reference::new("col").is_in([Datum::long(200), Datum::long(400)]),
+ ])
+ }
+
+ #[test]
+ fn eval_keeps_float_pages_of_promoted_double_field() -> Result<()> {
+ // A `float` -> `double` promotion leaves older files with FLOAT page
+ // bounds, which can't be compared with a `double` literal.
+ let (metadata, _temp_file) =
+ create_parquet_file_from_values(&[1.0_f32, 2.0, 3.0, 4.0], |v| {
+ Arc::new(Float32Array::from(v))
+ })?;
+
+ assert_keeps_all_pages(&metadata, PrimitiveType::Double, 4096, [
+ Reference::new("col").greater_than(Datum::double(2.5)),
+ Reference::new("col").less_than_or_equal_to(Datum::double(2.5)),
+ Reference::new("col").is_in([Datum::double(2.0),
Datum::double(4.0)]),
+ ])
+ }
+
+ #[test]
+ fn eval_keeps_uint32_pages_of_long_field() -> Result<()> {
+ // Parquet `UINT_32` columns map to `long` but have INT32 page bounds.
+ let (metadata, _temp_file) =
+ create_parquet_file_from_values(&[100_u32, 3_000_000_000], |v| {
Review Comment:
For the widening follow-up: UINT_32 page bounds are the raw INT32 bits with
an unsigned column order, so they can't be widened with `i64::from(i32)`. I
wrote a page that mixes `100` and `3_000_000_000` through the same helper. The
column index has `min = 100`, `max = -1294967296`
(`ColumnOrder::TYPE_DEFINED_ORDER(UNSIGNED)`), so sign-extending makes the max
smaller than the min, and `x > 2_000_000_000` would prune a page that holds
`3_000_000_000`. These pages need zero extension (`u32 as i64`) when the column
order is unsigned.
Since every page here holds a single value, this test can't tell the two
apart. Adding a mixed page would pin it before the widening lands.
--
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]
