laskoviymishka commented on code in PR #3328:
URL: https://github.com/apache/iceberg-rust/pull/3328#discussion_r4232149207
##########
crates/iceberg/src/expr/visitors/page_index_evaluator.rs:
##########
@@ -455,6 +544,40 @@ impl<'a> PageIndexEvaluator<'a> {
}
}
+ /// Converts a `FIXED_LEN_BYTE_ARRAY` decimal page bound into a [`Datum`].
+ /// 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
Review Comment:
The guard itself is right — a valid full-width FLBA bound always has exactly
`type_length` bytes, so a short bound can only mean truncation or a broken
writer, and skipping is the safe response. The wording oversells how reachable
it is, though: parquet-rs and parquet-mr's `BinaryTruncator` don't truncate
DECIMAL-annotated bounds, so for a real Iceberg decimal column this never
fires. I'd reframe both this doc comment and the test comment as
defence-in-depth against a non-conforming writer that stores a decimal as plain
FLBA, so nobody reads it as "Iceberg tables hit this path."
##########
crates/iceberg/src/expr/visitors/page_index_evaluator.rs:
##########
@@ -394,14 +408,89 @@ impl<'a> PageIndexEvaluator<'a> {
Ok(page_filter)
}
+ ColumnIndexMetaData::FIXED_LEN_BYTE_ARRAY(idx) => {
+ // Parquet stores Iceberg decimals with precision > 18 as
+ // FIXED_LEN_BYTE_ARRAY. Other field types on such a column
+ // (fixed, uuid) aren't decoded here, so skip page pruning for
+ // them rather than risk pruning pages that match.
+ if !matches!(field_type, PrimitiveType::Decimal { .. }) {
+ tracing::debug!(
+ field_id,
+ %field_type,
+ "Skipping page-index pruning: FIXED_LEN_BYTE_ARRAY
column index on a non-decimal field"
+ );
+ return Ok(None);
+ }
+
+ let Some(type_length) = type_length else {
+ tracing::debug!(
+ field_id,
+ %field_type,
+ "Skipping page-index pruning: unknown
FIXED_LEN_BYTE_ARRAY decimal column width"
+ );
+ return Ok(None);
+ };
+
+ let mut page_filter = Vec::with_capacity(row_counts.len());
+ for ((i, (min, max)), &row_count) in idx
+ .min_values_iter()
+ .zip(idx.max_values_iter())
+ .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)
+ // means this column's page index can't be trusted, so skip
+ // pruning for the whole column rather than abort the scan.
Review Comment:
The min and max arms are the same closure written out twice, and we build
two `Result`s just to log one `%err` and bail. `decode` captures only
`field_type` and `type_length` (both `Copy`), so the closure is `Copy` and you
can hoist it and reuse it on both sides:
```rust
let decode = |b: &[u8]|
Self::fixed_len_byte_array_decimal_bound_to_datum(field_type, b, type_length);
let (min, max) = match (min.map(decode).transpose(),
max.map(decode).transpose()) {
(Ok(min), Ok(max)) => (min, max),
(Err(err), _) | (_, Err(err)) => {
tracing::debug!(field_id, %field_type, page_index = i, %err, "...");
return Ok(None);
}
};
```
While you're in here: the helper's `DataInvalid` reads as corruption, but a
short bound is legitimate `column_index_truncate_length` behavior that we only
ever log and skip on — I'd soften the kind (or have the helper return
`Option<Datum>`) so a future `?`-caller isn't misled.
##########
crates/iceberg/src/expr/visitors/page_index_evaluator.rs:
##########
@@ -167,6 +168,17 @@ impl<'a> PageIndexEvaluator<'a> {
return self.select_all_rows();
};
+ // Declared byte width of a FIXED_LEN_BYTE_ARRAY column, used to detect
+ // truncated page-index bounds when decoding decimals. `None` when the
+ // column metadata is absent or reports no fixed width.
+ let type_length = self
+ .row_group_metadata
+ .columns()
+ .get(parquet_column_index)
+ .map(|column| column.column_descr().type_length())
+ .filter(|&len| len > 0)
+ .map(|len| len as usize);
Review Comment:
`len as usize` is safe here (positive `i32`), but
`usize::try_from(len).ok()` keeps it lossless-by-construction. Bigger picture:
this runs for every predicate column though only the FLBA arm reads it, and it
widens the shared helper signature by a positional arg — I'd lean toward
deriving `type_length` lazily inside the arm (or passing the
`ColumnDescriptor`) so the next metadata-hungry arm doesn't add another. Not
blocking either way.
##########
crates/iceberg/src/expr/visitors/page_index_evaluator.rs:
##########
@@ -1880,6 +2051,141 @@ mod tests {
Ok(())
}
+ #[test]
+ fn eval_inequality_prunes_fixed_len_byte_array_decimal_pages() ->
Result<()> {
+ // precision 30 -> Parquet FIXED_LEN_BYTE_ARRAY page bounds.
+ let (metadata, _temp_file) = create_decimal_parquet_file(30, 2, &[100,
200, 300, 400])?;
+ let (column_index, offset_index, row_group_metadata) =
get_test_metadata(&metadata);
+ let (iceberg_schema, field_id_map) =
build_decimal_schema_and_field_map(30, 2)?;
+
+ // Pages hold 1.00, 2.00, 3.00, 4.00. `> 2.50` keeps the pages whose
+ // upper bound exceeds 2.50 (3.00 and 4.00).
+ let filter = Reference::new("col_decimal")
+ .greater_than(decimal_datum(250, 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(2048)
+ ]);
+
+ Ok(())
+ }
+
+ #[test]
+ fn eval_in_prunes_fixed_len_byte_array_decimal_pages() -> Result<()> {
+ // precision 30 -> Parquet FIXED_LEN_BYTE_ARRAY page bounds.
+ let (metadata, _temp_file) = create_decimal_parquet_file(30, 2, &[100,
200, 300, 400])?;
+ let (column_index, offset_index, row_group_metadata) =
get_test_metadata(&metadata);
+ let (iceberg_schema, field_id_map) =
build_decimal_schema_and_field_map(30, 2)?;
+
+ // Pages hold 1.00, 2.00, 3.00, 4.00. IN (2.00, 4.00) keeps only the
+ // pages whose single value is one of the literals.
+ let filter = Reference::new("col_decimal")
+ .is_in([decimal_datum(200, 2, 30)?, decimal_datum(400, 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(1024),
+ RowSelector::select(1024),
+ RowSelector::skip(1024),
+ RowSelector::select(1024),
+ ]);
+
+ Ok(())
+ }
+
+ #[test]
+ fn eval_inequality_prunes_negative_fixed_len_byte_array_decimal_pages() ->
Result<()> {
+ // precision 30 -> Parquet FIXED_LEN_BYTE_ARRAY page bounds, spanning
+ // negative values. Negative decimals have a 0xff high byte in two's
+ // complement, so this only prunes correctly if the bounds are compared
+ // numerically rather than by raw byte order.
+ let (metadata, _temp_file) = create_decimal_parquet_file(30, 2,
&[-400, -200, 100, 300])?;
+ let (column_index, offset_index, row_group_metadata) =
get_test_metadata(&metadata);
+ let (iceberg_schema, field_id_map) =
build_decimal_schema_and_field_map(30, 2)?;
+
+ // Pages hold -4.00, -2.00, 1.00, 3.00. `>= -2.00` keeps the pages
whose
+ // upper bound is at least -2.00, including page 1 whose bound equals
it.
+ let filter = Reference::new("col_decimal")
+ .greater_than_or_equal_to(decimal_datum(-200, 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(1024),
+ RowSelector::select(3072)
+ ]);
+
+ Ok(())
+ }
+
+ #[test]
+ fn eval_skips_pruning_for_truncated_fixed_len_byte_array_decimal_bound()
-> Result<()> {
Review Comment:
The new tests all land on precision 30 (13-byte bounds), so the width
extremes aren't covered. Two worth adding: a precision-38 column (16-byte, hits
the `type_length == 16` boundary and the full-width negative extreme), and a
narrow-width-widened-precision case — a column physically stored below the
field precision's minimum width (e.g. a 9-byte FLBA read as decimal(30,2)) —
which is where the sign extension in `i128_from_be_bytes` actually earns its
keep. Not blocking, but they'd pin the decode down across the width range.
##########
crates/iceberg/src/expr/visitors/page_index_evaluator.rs:
##########
@@ -1880,6 +2051,141 @@ mod tests {
Ok(())
}
+ #[test]
+ fn eval_inequality_prunes_fixed_len_byte_array_decimal_pages() ->
Result<()> {
Review Comment:
All four new tests use non-null data, so the `None`-bound / null-count path
on this arm (the `idx.null_count(i)` + `None, None` into the predicate) isn't
exercised. I'd add one test with a null-only page — an `IS NULL`, or a `> x`
that should keep the null page — so a bad refactor of the None handling here
gets caught.
--
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]
