anoopj commented on code in PR #3247:
URL: https://github.com/apache/iceberg-rust/pull/3247#discussion_r4076307429
##########
crates/iceberg/src/expr/visitors/page_index_evaluator.rs:
##########
@@ -1185,6 +1289,141 @@ mod tests {
Ok(())
}
+ #[test]
+ fn eval_inequality_prunes_binary_pages_with_non_utf8_bounds() ->
Result<()> {
+ let (metadata, _temp_file) = create_binary_parquet_file()?;
+ let (column_index, offset_index, row_group_metadata) =
get_test_metadata(&metadata);
+
+ let iceberg_schema = Arc::new(
+ Schema::builder()
+ .with_fields([Arc::new(NestedField::new(
+ 1,
+ "col_binary",
+ Type::Primitive(PrimitiveType::Binary),
+ true,
+ ))])
+ .build()?,
+ );
+ let field_id_map = HashMap::from_iter([(1, 0)]);
+
+ // Page 0 bounds are [0x01], page 1 bounds are [0xff, 0x00]; only page
1
+ // exceeds [0x80]. Decoding [0xff, 0x00] as UTF-8 would panic.
+ let filter = Reference::new("col_binary")
+ .greater_than(Datum::binary(vec![0x80u8]))
+ .bind(iceberg_schema.clone(), false)?;
+
+ let result = PageIndexEvaluator::eval(
+ &filter,
+ &column_index,
+ &offset_index,
+ row_group_metadata,
+ &field_id_map,
+ iceberg_schema.as_ref(),
+ )?;
+
+ let expected = vec![RowSelector::skip(1024),
RowSelector::select(1024)];
+
+ assert_eq!(result, expected);
+
+ Ok(())
+ }
+
+ #[test]
+ fn eval_skips_pruning_for_byte_array_decimal_bounds() -> Result<()> {
+ // A non-spec writer can store a decimal column as BYTE_ARRAY. Those
+ // bounds can't be decoded safely (Parquet may truncate them, and
+ // truncation only preserves lexicographic byte order, not decimal
+ // order), so the evaluator skips page pruning rather than prune pages
+ // that might match.
+ let (metadata, _temp_file) = create_binary_parquet_file()?;
+ let (column_index, offset_index, row_group_metadata) =
get_test_metadata(&metadata);
+
+ let iceberg_schema = Arc::new(
+ Schema::builder()
+ .with_fields([Arc::new(NestedField::new(
+ 1,
+ "col_decimal",
+ Type::Primitive(PrimitiveType::Decimal {
+ precision: 10,
+ scale: 2,
+ }),
+ true,
+ ))])
+ .build()?,
+ );
+ let field_id_map = HashMap::from_iter([(1, 0)]);
+
+ // A predicate that would prune every page if the bounds were decoded.
+ let filter = Reference::new("col_decimal")
+ .greater_than(Datum::decimal_with_precision(
+
crate::spec::decimal_utils::decimal_from_i128_with_scale(99999, 2),
+ 10,
+ )?)
+ .bind(iceberg_schema.clone(), false)?;
+
+ let result = PageIndexEvaluator::eval(
+ &filter,
+ &column_index,
+ &offset_index,
+ row_group_metadata,
+ &field_id_map,
+ iceberg_schema.as_ref(),
+ )?;
+
+ // Both 1024-row pages survive: no page pruning is applied.
+ assert_eq!(result, vec![RowSelector::select(2048)]);
+
+ Ok(())
+ }
+
+ #[test]
+ fn eval_skips_pruning_for_fixed_len_byte_array() -> Result<()> {
+ // FIXED_LEN_BYTE_ARRAY page indexes back spec-conforming fixed, uuid,
+ // and decimal(P > 18) columns. The evaluator can't interpret those
+ // bounds, so it skips page pruning rather than abort the scan.
+ let (metadata, _temp_file) =
create_fixed_len_byte_array_parquet_file()?;
+ let (column_index, offset_index, row_group_metadata) =
get_test_metadata(&metadata);
+
+ let iceberg_schema = Arc::new(
+ Schema::builder()
+ .with_fields([Arc::new(NestedField::new(
+ 1,
+ "col_fixed",
+ Type::Primitive(PrimitiveType::Fixed(2)),
+ true,
+ ))])
+ .build()?,
+ );
+ let field_id_map = HashMap::from_iter([(1, 0)]);
+
+ // A predicate that would prune every page if the bounds were decoded.
+ let filter = Reference::new("col_fixed")
+ .greater_than(Datum::fixed(vec![0xffu8, 0xff]))
+ .bind(iceberg_schema.clone(), false)?;
+
+ let result = PageIndexEvaluator::eval(
+ &filter,
+ &column_index,
+ &offset_index,
+ row_group_metadata,
+ &field_id_map,
+ iceberg_schema.as_ref(),
+ )?;
+
+ // All rows survive: no page pruning is applied for
FIXED_LEN_BYTE_ARRAY.
+ assert_eq!(result, vec![RowSelector::select(2048)]);
+
+ Ok(())
+ }
+
+ #[test]
+ fn byte_array_bound_errors_on_invalid_utf8_string() {
Review Comment:
Done. Replaced the helper-only assertion with
`eval_skips_pruning_for_non_utf8_string_bound`. Dropped the old
`byte_array_bound_errors_on_invalid_utf8_string` test since the arm now
swallows that error.
--
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]
