laskoviymishka commented on code in PR #3247:
URL: https://github.com/apache/iceberg-rust/pull/3247#discussion_r4075510016
##########
crates/iceberg/src/expr/visitors/page_index_evaluator.rs:
##########
@@ -339,46 +339,62 @@ 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()),
- )
- }),
- PageNullCount::from_row_and_null_counts(row_count,
idx.null_count(i)),
- )
- })
- .collect(),
- ColumnIndexMetaData::FIXED_LEN_BYTE_ARRAY(_) => {
- return Err(Error::new(
- ErrorKind::FeatureUnsupported,
- "unsupported 'FIXED_LEN_BYTE_ARRAY' index type in
column_index",
- ));
+ 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) {
+ return Ok(None);
+ }
+
+ idx.min_values_iter()
+ .zip(idx.max_values_iter())
+ .enumerate()
+ .zip(row_counts.iter())
+ .map(|((i, (min, max)), &row_count)| {
+ predicate(
+ 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()?,
+ PageNullCount::from_row_and_null_counts(row_count,
idx.null_count(i)),
+ )
+ })
+ .collect()
}
- ColumnIndexMetaData::INT96(_) => {
- return Err(Error::new(
- ErrorKind::FeatureUnsupported,
- "unsupported 'INT96' index type in column_index",
- ));
+ // Column index types we can't interpret: skip page pruning rather
+ // than abort the scan. Row-group filtering and the Arrow row
filter
+ // still apply the predicate.
+ ColumnIndexMetaData::FIXED_LEN_BYTE_ARRAY(_) |
ColumnIndexMetaData::INT96(_) => {
+ return Ok(None);
Review Comment:
While we're here — both new skip branches return `Ok(None)` silently, so a
table with fixed/uuid/decimal(P>18) or INT96 columns permanently loses
page-index pruning on every filtered scan with no signal to an operator. The
absent-column-index path in `row_filter.rs` already emits a `tracing::debug!`
for exactly this kind of skip.
A `debug!`/`trace!` here noting the column-index type and field id would
keep it diagnosable. Not blocking.
##########
crates/iceberg/src/expr/visitors/page_index_evaluator.rs:
##########
@@ -339,46 +339,62 @@ 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()),
- )
- }),
- PageNullCount::from_row_and_null_counts(row_count,
idx.null_count(i)),
- )
- })
- .collect(),
- ColumnIndexMetaData::FIXED_LEN_BYTE_ARRAY(_) => {
- return Err(Error::new(
- ErrorKind::FeatureUnsupported,
- "unsupported 'FIXED_LEN_BYTE_ARRAY' index type in
column_index",
- ));
+ 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) {
+ return Ok(None);
+ }
+
+ idx.min_values_iter()
+ .zip(idx.max_values_iter())
+ .enumerate()
+ .zip(row_counts.iter())
+ .map(|((i, (min, max)), &row_count)| {
+ predicate(
+ 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()?,
+ PageNullCount::from_row_and_null_counts(row_count,
idx.null_count(i)),
+ )
+ })
+ .collect()
}
- ColumnIndexMetaData::INT96(_) => {
- return Err(Error::new(
- ErrorKind::FeatureUnsupported,
- "unsupported 'INT96' index type in column_index",
- ));
+ // Column index types we can't interpret: skip page pruning rather
+ // than abort the scan. Row-group filtering and the Arrow row
filter
+ // still apply the predicate.
+ ColumnIndexMetaData::FIXED_LEN_BYTE_ARRAY(_) |
ColumnIndexMetaData::INT96(_) => {
+ return Ok(None);
}
};
Ok(Some(result?))
}
+ /// Converts a `BYTE_ARRAY` page bound into a [`Datum`] according to the
+ /// field's primitive type. Parquet stores Iceberg `string` and `binary`
+ /// bounds as `BYTE_ARRAY`.
+ fn byte_array_bound_to_datum(field_type: &PrimitiveType, bytes: &[u8]) ->
Result<Datum> {
+ match field_type {
+ PrimitiveType::String => {
+ let value = std::str::from_utf8(bytes).map_err(|err| {
+ Error::new(ErrorKind::DataInvalid, "Invalid UTF-8 in
string page bound")
+ .with_source(err)
+ })?;
+ Ok(Datum::string(value))
+ }
+ PrimitiveType::Binary => Ok(Datum::binary(bytes.to_vec())),
+ // Unreachable: callers gate BYTE_ARRAY decoding to string and
binary.
Review Comment:
Small thing on this comment — the `_` arm is reachable.
`byte_array_bound_to_datum` is a plain associated fn with no gating, and the
test module calls it directly (that's how
`byte_array_bound_errors_on_invalid_utf8_string` works). Keeping the `Err`
fallback is right, but "Unreachable" could mislead someone into treating it as
dead code. I'd reword to something like "Defensive: the production caller only
invokes this for String/Binary."
##########
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:
This asserts on `byte_array_bound_to_datum` directly, so it locks in the
helper's error kind but never exercises what `eval` actually does with an
invalid-UTF-8 String bound — which is where the abort behavior above lives.
I'd add a test that builds a `col_string` BYTE_ARRAY column index with a
non-UTF-8 bound and asserts on `eval`'s return. As it stands, if we fix the
abort to skip, this test passes unchanged — so it isn't guarding the behavior
that matters.
##########
crates/iceberg/src/expr/visitors/page_index_evaluator.rs:
##########
@@ -339,46 +339,62 @@ 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()),
- )
- }),
- PageNullCount::from_row_and_null_counts(row_count,
idx.null_count(i)),
- )
- })
- .collect(),
- ColumnIndexMetaData::FIXED_LEN_BYTE_ARRAY(_) => {
- return Err(Error::new(
- ErrorKind::FeatureUnsupported,
- "unsupported 'FIXED_LEN_BYTE_ARRAY' index type in
column_index",
- ));
+ 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) {
+ return Ok(None);
+ }
+
+ idx.min_values_iter()
+ .zip(idx.max_values_iter())
+ .enumerate()
+ .zip(row_counts.iter())
+ .map(|((i, (min, max)), &row_count)| {
+ predicate(
+ min.map(|val|
Self::byte_array_bound_to_datum(field_type, val))
+ .transpose()?,
Review Comment:
This is the one path in the arm that still aborts the whole scan instead of
degrading.
When `field_type` is String and a page bound isn't valid UTF-8,
`byte_array_bound_to_datum` returns `DataInvalid`, and `.transpose()?`
propagates it out through `collect()` → `eval` → the pipeline, failing the
entire file read — not just page pruning for this column. That's the one case
where we added real decoding, and it's the one case that doesn't fall back to
`Ok(None)` like the BYTE_ARRAY-decimal / FIXED_LEN / INT96 skips right above
it. A writer that truncates a min/max stat mid-UTF-8-sequence is a real class
of bug, and it'd take down a read on an otherwise spec-conformant String column.
I'd restructure the arm so a decode error on either bound short-circuits to
`Ok(None)` for the column rather than erroring through `collect()`. Once it
degrades like the rest, happy to approve.
--
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]
