rich7420 commented on code in PR #3507:
URL: https://github.com/apache/iceberg-rust/pull/3507#discussion_r4349685853
##########
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:
Thanks for catching this! I added a page with both `100` and `3_000_000_000`
and asserted its raw bounds, where the max read as `i32` sorts below the min.
The widening follow-up will need to zero-extend these.
##########
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:
Thanks! I'll keep this PR focused on the conservative fix and handle the
`promote_to` widening in a follow-up, zero-extending `UINT_32` bounds as you
noted.
--
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]
