From 45d0a1551eeb3945ee38e85977b006d68a9b641b Mon Sep 17 00:00:00 2001 From: Anoop Johnson Date: Fri, 9 Oct 2026 12:06:15 -0700 Subject: [PATCH] test(reader): widen FLBA decimal page-index coverage and clarify guard docs Address review feedback on #3328: - Reframe the width-guard doc and inline comment as defence-in-depth against a non-conforming writer - Hoist the duplicated min/max decode into a single closure. - Add page-index tests --- .../src/expr/visitors/page_index_evaluator.rs | 178 +++++++++++++++--- 1 file changed, 152 insertions(+), 26 deletions(-) diff --git a/crates/iceberg/src/expr/visitors/page_index_evaluator.rs b/crates/iceberg/src/expr/visitors/page_index_evaluator.rs index 88d1eea9ff..4e51042336 100644 --- a/crates/iceberg/src/expr/visitors/page_index_evaluator.rs +++ b/crates/iceberg/src/expr/visitors/page_index_evaluator.rs @@ -438,28 +438,20 @@ impl<'a> PageIndexEvaluator<'a> { .enumerate() .zip(row_counts.iter()) { - // A bound that won't decode (e.g. a min/max stat truncated - // by column_index_truncate_length, whose byte prefix no - // longer preserves the two's-complement decimal ordering) + // An undecodable bound (only a non-conforming writer + // produces one, see fixed_len_byte_array_decimal_bound_to_datum) // means this column's page index can't be trusted, so skip // pruning for the whole column rather than abort the scan. + let decode = |val: &[u8]| { + Self::fixed_len_byte_array_decimal_bound_to_datum( + field_type, + val, + type_length, + ) + }; let (min, max) = match ( - min.map(|val| { - Self::fixed_len_byte_array_decimal_bound_to_datum( - field_type, - val, - type_length, - ) - }) - .transpose(), - max.map(|val| { - Self::fixed_len_byte_array_decimal_bound_to_datum( - field_type, - val, - type_length, - ) - }) - .transpose(), + min.map(decode).transpose(), + max.map(decode).transpose(), ) { (Ok(min), Ok(max)) => (min, max), (Err(err), _) | (_, Err(err)) => { @@ -548,9 +540,11 @@ impl<'a> PageIndexEvaluator<'a> { /// Parquet stores Iceberg decimals with precision > 18 as big-endian /// two's-complement `FIXED_LEN_BYTE_ARRAY` of width `type_length`. /// - /// Returns an error for a bound whose width differs from `type_length`: that - /// signals a `column_index_truncate_length` truncation, and a truncated byte - /// prefix decodes to the wrong number under two's-complement. + /// Returns an error for a bound whose width differs from `type_length`. + /// Conforming writers never truncate a decimal column index (a truncated + /// two's-complement prefix would not preserve numeric order), so a short + /// bound comes from a non-conforming writer and would decode to the wrong + /// value. Reject it rather than prune on a bad bound. fn fixed_len_byte_array_decimal_bound_to_datum( field_type: &PrimitiveType, bytes: &[u8], @@ -1241,6 +1235,18 @@ mod tests { precision: u8, scale: i8, unscaled: &[i128], + ) -> Result<(Arc, NamedTempFile)> { + let pages: Vec> = unscaled.iter().copied().map(Some).collect(); + create_decimal_parquet_file_with_null_pages(precision, scale, &pages) + } + + /// Like [`create_decimal_parquet_file`], but each entry in `pages` is one + /// 1024-row page: `Some(v)` writes that repeated unscaled value, `None` + /// writes an all-null page (whose column index carries no min/max bound). + fn create_decimal_parquet_file_with_null_pages( + precision: u8, + scale: i8, + pages: &[Option], ) -> Result<(Arc, NamedTempFile)> { let arrow_schema = Arc::new(ArrowSchema::new(vec![Field::new( "col_decimal", @@ -1258,11 +1264,16 @@ mod tests { let mut writer = ArrowWriter::try_new(file, arrow_schema.clone(), Some(props)).unwrap(); - for &unscaled in unscaled { + for page in pages { let array = Arc::new( - Decimal128Array::from_iter_values(std::iter::repeat_n(unscaled, 1024)) - .with_precision_and_scale(precision, scale) - .unwrap(), + match page { + Some(unscaled) => { + Decimal128Array::from_iter_values(std::iter::repeat_n(*unscaled, 1024)) + } + None => Decimal128Array::from(vec![None; 1024]), + } + .with_precision_and_scale(precision, scale) + .unwrap(), ) as ArrayRef; let batch = RecordBatch::try_new(arrow_schema.clone(), vec![array]).unwrap(); // Write rows one at a time so the writer splits into per-value pages. @@ -2186,6 +2197,121 @@ mod tests { Ok(()) } + #[test] + fn eval_inequality_prunes_precision_38_fixed_len_byte_array_decimal_pages() -> Result<()> { + // precision 38 is the max Iceberg decimal precision -> 16-byte + // FIXED_LEN_BYTE_ARRAY bounds, the width extreme where decoding fills + // the full i128 with no sign-extension padding. Values near +/-(10^38-1) + // carry significant bits in the high-order bytes, and span both signs. + let max = 10_i128.pow(38) - 1; + let (metadata, _temp_file) = create_decimal_parquet_file(38, 2, &[-max, 0, max])?; + let (column_index, offset_index, row_group_metadata) = get_test_metadata(&metadata); + let (iceberg_schema, field_id_map) = build_decimal_schema_and_field_map(38, 2)?; + + // `> 0` keeps only the +max page; the -max and 0 pages are pruned. + let filter = Reference::new("col_decimal") + .greater_than(decimal_datum(0, 2, 38)?) + .bind(iceberg_schema.clone(), false)?; + + let result = PageIndexEvaluator::eval( + &filter, + &column_index, + &offset_index, + row_group_metadata, + &field_id_map, + iceberg_schema.as_ref(), + )?; + + assert_eq!(result, vec![ + RowSelector::skip(2048), + RowSelector::select(1024) + ]); + + Ok(()) + } + + #[test] + fn eval_inequality_prunes_precision_widened_fixed_len_byte_array_decimal_pages() -> Result<()> { + // Precision widening keeps a file's original FIXED_LEN_BYTE_ARRAY width, + // so a decimal(20,2) file's 9-byte bounds are read here as decimal(30,2). + // The guard keys off the file's type_length (9), not the field + // precision's canonical width (13). + let (metadata, _temp_file) = create_decimal_parquet_file(20, 2, &[-400, 100, 300])?; + let (column_index, offset_index, row_group_metadata) = get_test_metadata(&metadata); + // Read the 9-byte column as decimal(30,2). + let (iceberg_schema, field_id_map) = build_decimal_schema_and_field_map(30, 2)?; + + // Pages hold -4.00, 1.00, 3.00. `> 1.50` keeps only the 3.00 page. + let filter = Reference::new("col_decimal") + .greater_than(decimal_datum(150, 2, 30)?) + .bind(iceberg_schema.clone(), false)?; + + let result = PageIndexEvaluator::eval( + &filter, + &column_index, + &offset_index, + row_group_metadata, + &field_id_map, + iceberg_schema.as_ref(), + )?; + + assert_eq!(result, vec![ + RowSelector::skip(2048), + RowSelector::select(1024) + ]); + + Ok(()) + } + + #[test] + fn eval_is_null_selects_all_null_fixed_len_byte_array_decimal_page() -> Result<()> { + // precision 30 -> FIXED_LEN_BYTE_ARRAY page bounds. Page 0 is all-null, + // so its column index carries no min/max; the arm must pass those absent + // bounds straight to the predicate rather than try to decode them. Pages + // 1-2 hold 2.00 and 3.00. + let (metadata, _temp_file) = + create_decimal_parquet_file_with_null_pages(30, 2, &[None, Some(200), Some(300)])?; + let (column_index, offset_index, row_group_metadata) = get_test_metadata(&metadata); + + // The column is optional so IS NULL binds to a real predicate (on a + // required field it would simplify to always-false). + let iceberg_schema = Arc::new( + Schema::builder() + .with_fields([Arc::new(NestedField::new( + 1, + "col_decimal", + Type::Primitive(PrimitiveType::Decimal { + precision: 30, + scale: 2, + }), + false, + ))]) + .build()?, + ); + let field_id_map = HashMap::from_iter([(1, 0)]); + + // IS NULL keeps the all-null page and prunes the two value pages. + let filter = Reference::new("col_decimal") + .is_null() + .bind(iceberg_schema.clone(), false)?; + + let result = PageIndexEvaluator::eval( + &filter, + &column_index, + &offset_index, + row_group_metadata, + &field_id_map, + iceberg_schema.as_ref(), + )?; + + assert_eq!(result, vec![ + RowSelector::select(1024), + RowSelector::skip(2048) + ]); + + Ok(()) + } + /// Builds a decimal query [`Datum`] from an `unscaled` mantissa, matching /// the field's `scale` and `precision`. fn decimal_datum(unscaled: i128, scale: u32, precision: u32) -> Result {