You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@iceberg.apache.org by gi...@apache.org on 2026/10/11 10:37:27 UTC
(iceberg-rust) branch gh-readonly-queue/main/pr-3386-b2e90754b1f9eaea741d353d51ae644aa9cef479 created (now e42840bc9)
This is an automated email from the ASF dual-hosted git repository.
github-merge-queue[bot] pushed a change to branch gh-readonly-queue/main/pr-3386-b2e90754b1f9eaea741d353d51ae644aa9cef479
in repository https://gitbox.apache.org/repos/asf/iceberg-rust.git
at e42840bc9 test(reader): widen FLBA decimal page-index coverage and clarify docs (#3386)
This branch includes the following new commits:
new e42840bc9 test(reader): widen FLBA decimal page-index coverage and clarify docs (#3386)
The 1 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
(iceberg-rust) 01/01: test(reader): widen FLBA decimal page-index coverage and clarify docs (#3386)
Posted by gi...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
github-merge-queue[bot] pushed a commit to branch gh-readonly-queue/main/pr-3386-b2e90754b1f9eaea741d353d51ae644aa9cef479
in repository https://gitbox.apache.org/repos/asf/iceberg-rust.git
commit e42840bc98cbf91a67c014162ed68036a8452d7a
Author: Anoop Johnson <an...@apache.org>
AuthorDate: Sun Oct 11 10:37:04 2026 +0000
test(reader): widen FLBA decimal page-index coverage and clarify docs (#3386)
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 88d1eea9f..4e5104233 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<ParquetMetaData>, NamedTempFile)> {
+ let pages: Vec<Option<i128>> = 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<i128>],
) -> Result<(Arc<ParquetMetaData>, 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<Datum> {