laskoviymishka commented on code in PR #3247:
URL: https://github.com/apache/iceberg-rust/pull/3247#discussion_r4102917497
##########
crates/iceberg/src/expr/visitors/page_index_evaluator.rs:
##########
@@ -339,46 +341,91 @@ impl<'a> PageIndexEvaluator<'a> {
)
})
.collect(),
- ColumnIndexMetaData::BYTE_ARRAY(idx) => idx
- .min_values_iter()
- .zip(idx.max_values_iter())
- .enumerate()
- .zip(row_counts.iter())
- .map(|((i, (min, max)), &row_count)| {
- predicate(
- min.map(|val| {
- Datum::new(
- field_type.clone(),
-
PrimitiveLiteral::String(String::from_utf8(val.to_vec()).unwrap()),
- )
- }),
- max.map(|val| {
- Datum::new(
- field_type.clone(),
-
PrimitiveLiteral::String(String::from_utf8(val.to_vec()).unwrap()),
- )
- }),
+ ColumnIndexMetaData::BYTE_ARRAY(idx) => {
+ // Parquet stores Iceberg string and binary bounds as
BYTE_ARRAY.
+ // Any other field type on a BYTE_ARRAY column (e.g. a non-spec
+ // BYTE_ARRAY decimal) can't be decoded safely, so skip page
+ // pruning for it rather than risk pruning pages that match.
+ if !matches!(field_type, PrimitiveType::String |
PrimitiveType::Binary) {
+ tracing::debug!(
+ field_id,
+ %field_type,
+ "Skipping page-index pruning: BYTE_ARRAY column index
on a non-string/binary field"
+ );
+ 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
+ // mid-UTF-8-sequence) means this column's page index can't
+ // be trusted, so skip pruning for the whole column rather
+ // than abort the scan.
+ let (Ok(min), Ok(max)) = (
+ min.map(|val|
Self::byte_array_bound_to_datum(field_type, val))
+ .transpose(),
+ max.map(|val|
Self::byte_array_bound_to_datum(field_type, val))
+ .transpose(),
+ ) else {
+ tracing::debug!(
Review Comment:
The skip-site debug log I asked for last round is in, thanks — one gap keeps
it from being actionable: it drops `i` (the page index) and the decode error,
both in scope right here. As written an operator sees "undecodable BYTE_ARRAY
page bound" but can't tell which page or why without a debugger. I'd add
`page_index = i` and log the error via `.as_ref().err()` before the let-else.
##########
crates/iceberg/src/expr/visitors/page_index_evaluator.rs:
##########
@@ -896,6 +945,92 @@ mod tests {
Ok((metadata, temp_file))
}
+ /// Creates a single-column parquet file whose `col_binary` page bounds
hold
+ /// non-UTF-8 bytes, backing the binary path through `BYTE_ARRAY` column
+ /// indexes. Page 0 is `[0x01]`, page 1 is `[0xff, 0x00]`.
+ fn create_binary_parquet_file() -> Result<(Arc<ParquetMetaData>,
NamedTempFile)> {
Review Comment:
Small thing, not worth holding for: this,
`create_fixed_len_byte_array_parquet_file`, and `create_test_parquet_file`
repeat the same props/writer/write-loop/reopen/`PageIndexPolicy::Required`
boilerplate, differing only in schema and per-page array. A shared
`write_single_column_pages(schema, pages)` helper would collapse all three so a
future fix doesn't have to land in three spots. Fine as a follow-up.
--
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]
