From 1c013de2a3b13c30998edc5e26b7c48d014a89f5 Mon Sep 17 00:00:00 2001 From: Xander Date: Sat, 2 May 2026 11:32:23 +0100 Subject: [PATCH 01/25] feat: bloom filter pushdown --- crates/iceberg/src/arrow/reader/mod.rs | 18 + crates/iceberg/src/arrow/reader/pipeline.rs | 83 ++ crates/iceberg/src/arrow/reader/row_filter.rs | 171 ++++ .../expr/visitors/bloom_filter_evaluator.rs | 865 ++++++++++++++++++ crates/iceberg/src/expr/visitors/mod.rs | 1 + crates/iceberg/src/scan/mod.rs | 22 +- .../iceberg/src/spec/values/decimal_utils.rs | 92 ++ 7 files changed, 1251 insertions(+), 1 deletion(-) create mode 100644 crates/iceberg/src/expr/visitors/bloom_filter_evaluator.rs diff --git a/crates/iceberg/src/arrow/reader/mod.rs b/crates/iceberg/src/arrow/reader/mod.rs index 495283f47f..c9fdecbfd3 100644 --- a/crates/iceberg/src/arrow/reader/mod.rs +++ b/crates/iceberg/src/arrow/reader/mod.rs @@ -57,6 +57,7 @@ pub struct ArrowReaderBuilder { concurrency_limit_data_files: usize, row_group_filtering_enabled: bool, row_selection_enabled: bool, + bloom_filter_enabled: bool, parquet_read_options: ParquetReadOptions, runtime: Runtime, } @@ -72,6 +73,7 @@ impl ArrowReaderBuilder { concurrency_limit_data_files: num_cpus, row_group_filtering_enabled: true, row_selection_enabled: false, + bloom_filter_enabled: false, parquet_read_options: ParquetReadOptions::builder().build(), runtime, } @@ -102,6 +104,20 @@ impl ArrowReaderBuilder { self } + /// Determines whether to enable bloom filter-based row group filtering. + /// + /// When enabled, if a read is performed with an equality or IN predicate, + /// the bloom filter for relevant columns in each row group is read and + /// checked. Row groups where the bloom filter proves the value is absent + /// are skipped entirely. + /// + /// Defaults to disabled, as reading bloom filters requires additional I/O + /// per column per row group. + pub fn with_bloom_filter_enabled(mut self, bloom_filter_enabled: bool) -> Self { + self.bloom_filter_enabled = bloom_filter_enabled; + self + } + /// Provide a hint as to the number of bytes to prefetch for parsing the Parquet metadata /// /// This hint can help reduce the number of fetch requests. For more details see the @@ -141,6 +157,7 @@ impl ArrowReaderBuilder { concurrency_limit_data_files: self.concurrency_limit_data_files, row_group_filtering_enabled: self.row_group_filtering_enabled, row_selection_enabled: self.row_selection_enabled, + bloom_filter_enabled: self.bloom_filter_enabled, parquet_read_options: self.parquet_read_options, } } @@ -158,5 +175,6 @@ pub struct ArrowReader { row_group_filtering_enabled: bool, row_selection_enabled: bool, + bloom_filter_enabled: bool, parquet_read_options: ParquetReadOptions, } diff --git a/crates/iceberg/src/arrow/reader/pipeline.rs b/crates/iceberg/src/arrow/reader/pipeline.rs index 5354c72043..609143d00f 100644 --- a/crates/iceberg/src/arrow/reader/pipeline.rs +++ b/crates/iceberg/src/arrow/reader/pipeline.rs @@ -44,6 +44,9 @@ use crate::arrow::record_batch_transformer::RecordBatchTransformerBuilder; use crate::arrow::scan_metrics::{CountingFileRead, ScanMetrics, ScanResult}; use crate::encryption::StandardKeyMetadata; use crate::error::Result; +use crate::expr::visitors::bloom_filter_evaluator::{ + BloomFilterEvaluator, collect_bloom_filter_field_ids, +}; use crate::io::{FileIO, FileMetadata, FileRead}; use crate::metadata_columns::{ RESERVED_COL_NAME_LAST_UPDATED_SEQUENCE_NUMBER, RESERVED_COL_NAME_POS, @@ -70,6 +73,7 @@ impl ArrowReader { .with_scan_metrics(scan_metrics.clone()), row_group_filtering_enabled: self.row_group_filtering_enabled, row_selection_enabled: self.row_selection_enabled, + bloom_filter_enabled: self.bloom_filter_enabled, parquet_read_options: self.parquet_read_options, scan_metrics: scan_metrics.clone(), }; @@ -123,6 +127,7 @@ struct FileScanTaskReader { delete_file_loader: CachingDeleteFileLoader, row_group_filtering_enabled: bool, row_selection_enabled: bool, + bloom_filter_enabled: bool, parquet_read_options: ParquetReadOptions, scan_metrics: ScanMetrics, } @@ -632,6 +637,24 @@ impl FileScanTaskReader { }; } + if self.bloom_filter_enabled { + let candidate_rgs = selected_row_group_indices.clone().unwrap_or_else(|| { + (0..record_batch_stream_builder.metadata().num_row_groups()).collect() + }); + + let bloom_filtered = Self::filter_row_groups_by_bloom_filter( + &predicate, + &mut record_batch_stream_builder, + &candidate_rgs, + &field_id_map, + ) + .await?; + + if bloom_filtered.len() < candidate_rgs.len() { + selected_row_group_indices = Some(bloom_filtered); + } + } + if self.row_selection_enabled { row_selection = ArrowReader::get_row_selection_for_filter_predicate( &predicate, @@ -692,6 +715,66 @@ impl FileScanTaskReader { Ok(Box::pin(record_batch_stream) as ArrowRecordBatchStream) } + + /// Reads bloom filters for relevant columns and evaluates the predicate + /// against them to filter out row groups that definitely don't match. + async fn filter_row_groups_by_bloom_filter( + predicate: &crate::expr::BoundPredicate, + builder: &mut ParquetRecordBatchStreamBuilder, + candidate_row_groups: &[usize], + field_id_map: &HashMap, + ) -> Result> { + // Only collect field IDs from eq/in predicates — the only types + // bloom filters can help with. Skip columns not in the parquet schema. + let bloom_filter_field_ids: Vec = collect_bloom_filter_field_ids(predicate)? + .into_iter() + .filter(|id| field_id_map.contains_key(id)) + .collect(); + + if bloom_filter_field_ids.is_empty() { + return Ok(candidate_row_groups.to_vec()); + } + + let mut result = Vec::with_capacity(candidate_row_groups.len()); + + for &rg_idx in candidate_row_groups { + let mut bloom_filters: HashMap = + HashMap::new(); + + for &field_id in &bloom_filter_field_ids { + let col_idx = field_id_map[&field_id]; + let col_meta = builder.metadata().row_group(rg_idx).column(col_idx); + + // Only attempt to load if this column chunk actually has a bloom filter + if col_meta.bloom_filter_offset().is_none() { + continue; + } + + let physical_type = col_meta.column_type(); + + match builder + .get_row_group_column_bloom_filter(rg_idx, col_idx) + .await + { + Ok(Some(sbbf)) => { + bloom_filters.insert(field_id, (sbbf, physical_type)); + } + Ok(None) => {} + Err(_) => { + // If we can't read the bloom filter, conservatively include the row group + } + } + } + + match BloomFilterEvaluator::eval(predicate, &bloom_filters) { + Ok(true) => result.push(rg_idx), + Ok(false) => { /* Row group pruned by bloom filter */ } + Err(_) => result.push(rg_idx), // On error, conservatively include + } + } + + Ok(result) + } } impl ArrowReader { diff --git a/crates/iceberg/src/arrow/reader/row_filter.rs b/crates/iceberg/src/arrow/reader/row_filter.rs index 389231c461..faa5038490 100644 --- a/crates/iceberg/src/arrow/reader/row_filter.rs +++ b/crates/iceberg/src/arrow/reader/row_filter.rs @@ -1280,4 +1280,175 @@ mod tests { "positional deletes must be applied correctly even when page indexes are absent" ); } + + /// Tests that bloom filter pushdown correctly prunes row groups. + #[tokio::test] + async fn test_bloom_filter_pushdown_prunes_row_groups() { + let schema = Arc::new( + Schema::builder() + .with_schema_id(1) + .with_fields(vec![ + NestedField::required(1, "id", Type::Primitive(PrimitiveType::Int)).into(), + ]) + .build() + .unwrap(), + ); + + let arrow_schema = Arc::new(ArrowSchema::new(vec![ + Field::new("id", DataType::Int32, false).with_metadata(HashMap::from([( + PARQUET_FIELD_ID_META_KEY.to_string(), + "1".to_string(), + )])), + ])); + + let tmp_dir = TempDir::new().unwrap(); + let file_path = format!("{}/bloom_test.parquet", tmp_dir.path().to_str().unwrap()); + + // Write a Parquet file with 3 row groups, each containing distinct values, + // with bloom filters enabled. + // Row group 0: ids 0..100 + // Row group 1: ids 100..200 + // Row group 2: ids 200..300 + let props = WriterProperties::builder() + .set_compression(Compression::SNAPPY) + .set_max_row_group_row_count(Some(100)) + .set_bloom_filter_enabled(true) + .build(); + + let file = File::create(&file_path).unwrap(); + let mut writer = ArrowWriter::try_new(file, arrow_schema.clone(), Some(props)).unwrap(); + + for batch_start in [0, 100, 200] { + let batch = RecordBatch::try_new(arrow_schema.clone(), vec![Arc::new( + Int32Array::from((batch_start..batch_start + 100).collect::>()), + )]) + .unwrap(); + writer.write(&batch).unwrap(); + } + writer.close().unwrap(); + + let file_io = FileIO::new_with_fs(); + + // Query for id = 150, which is only in row group 1. + // With bloom filter pushdown, row groups 0 and 2 should be pruned. + let predicate = Reference::new("id").equal_to(Datum::int(150)); + + let reader = ArrowReaderBuilder::new(file_io.clone(), Runtime::current()) + .with_bloom_filter_enabled(true) + .build(); + + let task = FileScanTask::builder() + .with_file_size_in_bytes(std::fs::metadata(&file_path).unwrap().len()) + .with_start(0) + .with_length(0) + .with_data_file_path(file_path.clone()) + .with_data_file_format(DataFileFormat::Parquet) + .with_schema(schema.clone()) + .with_project_field_ids(vec![1]) + .with_predicate(Some(predicate.bind(schema.clone(), true).unwrap())) + .with_case_sensitive(false) + .build() + .unwrap(); + + let tasks = Box::pin(futures::stream::iter(vec![Ok(task)])) as FileScanTaskStream; + + let result = reader + .read(tasks) + .unwrap() + .stream() + .try_collect::>() + .await + .unwrap(); + + // Only row group 1 (ids 100..200) should be read. The row filter + // then further filters to just id=150. + let total_rows: usize = result.iter().map(|b| b.num_rows()).sum(); + assert_eq!(total_rows, 1, "Should find exactly one row matching id=150"); + + let id_col = result[0] + .column(0) + .as_primitive::(); + assert_eq!(id_col.value(0), 150); + } + + /// Tests that bloom filter pushdown skips all row groups when value is absent. + #[tokio::test] + async fn test_bloom_filter_pushdown_value_absent() { + let schema = Arc::new( + Schema::builder() + .with_schema_id(1) + .with_fields(vec![ + NestedField::required(1, "id", Type::Primitive(PrimitiveType::Int)).into(), + ]) + .build() + .unwrap(), + ); + + let arrow_schema = Arc::new(ArrowSchema::new(vec![ + Field::new("id", DataType::Int32, false).with_metadata(HashMap::from([( + PARQUET_FIELD_ID_META_KEY.to_string(), + "1".to_string(), + )])), + ])); + + let tmp_dir = TempDir::new().unwrap(); + let file_path = format!("{}/bloom_absent.parquet", tmp_dir.path().to_str().unwrap()); + + let props = WriterProperties::builder() + .set_compression(Compression::SNAPPY) + .set_max_row_group_row_count(Some(100)) + .set_bloom_filter_enabled(true) + .build(); + + let file = File::create(&file_path).unwrap(); + let mut writer = ArrowWriter::try_new(file, arrow_schema.clone(), Some(props)).unwrap(); + + for batch_start in [0, 100, 200] { + let batch = RecordBatch::try_new(arrow_schema.clone(), vec![Arc::new( + Int32Array::from((batch_start..batch_start + 100).collect::>()), + )]) + .unwrap(); + writer.write(&batch).unwrap(); + } + writer.close().unwrap(); + + let file_io = FileIO::new_with_fs(); + + // Query for id = 999, which doesn't exist in any row group. + // All row groups should be pruned by bloom filter. + let predicate = Reference::new("id").equal_to(Datum::int(999)); + + let reader = ArrowReaderBuilder::new(file_io, Runtime::current()) + .with_bloom_filter_enabled(true) + .build(); + + let task = FileScanTask::builder() + .with_file_size_in_bytes(std::fs::metadata(&file_path).unwrap().len()) + .with_start(0) + .with_length(0) + .with_data_file_path(file_path.clone()) + .with_data_file_format(DataFileFormat::Parquet) + .with_schema(schema.clone()) + .with_project_field_ids(vec![1]) + .with_predicate(Some(predicate.bind(schema, true).unwrap())) + .with_case_sensitive(false) + .build() + .unwrap(); + + let tasks = Box::pin(futures::stream::iter(vec![Ok(task)])) as FileScanTaskStream; + + let result = reader + .read(tasks) + .unwrap() + .stream() + .try_collect::>() + .await + .unwrap(); + + let total_rows: usize = result.iter().map(|b| b.num_rows()).sum(); + assert_eq!( + total_rows, 0, + "Should find zero rows when value is absent from all bloom filters" + ); + } } diff --git a/crates/iceberg/src/expr/visitors/bloom_filter_evaluator.rs b/crates/iceberg/src/expr/visitors/bloom_filter_evaluator.rs new file mode 100644 index 0000000000..db9c2464df --- /dev/null +++ b/crates/iceberg/src/expr/visitors/bloom_filter_evaluator.rs @@ -0,0 +1,865 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +//! Evaluates predicates against Parquet bloom filters to determine whether +//! a row group can be skipped. + +use std::collections::{HashMap, HashSet}; + +use fnv::FnvHashSet; +use parquet::basic::Type as PhysicalType; +use parquet::bloom_filter::Sbbf; +use parquet::data_type::ByteArray; + +use crate::Result; +use crate::expr::visitors::bound_predicate_visitor::{BoundPredicateVisitor, visit}; +use crate::expr::{BoundPredicate, BoundReference}; +use crate::spec::decimal_utils::decimal_to_fixed_length_bytes; +use crate::spec::{Datum, PrimitiveLiteral}; + +const ROW_GROUP_MIGHT_MATCH: Result = Ok(true); +const ROW_GROUP_CANT_MATCH: Result = Ok(false); + +pub(crate) struct BloomFilterEvaluator<'a> { + /// Maps Iceberg field_id -> (bloom filter, Parquet physical type) for this row group + bloom_filters: &'a HashMap, +} + +impl<'a> BloomFilterEvaluator<'a> { + /// Evaluate the predicate against the provided bloom filters. + /// Returns `false` if the row group definitely does not match, + /// `true` if it might match. + pub(crate) fn eval( + filter: &BoundPredicate, + bloom_filters: &HashMap, + ) -> Result { + if bloom_filters.is_empty() { + return ROW_GROUP_MIGHT_MATCH; + } + + let mut evaluator = BloomFilterEvaluator { bloom_filters }; + visit(&mut evaluator, filter) + } + + fn check_datum(&self, reference: &BoundReference, datum: &Datum) -> bool { + let field_id = reference.field().id; + let Some((sbbf, physical_type)) = self.bloom_filters.get(&field_id) else { + // No bloom filter for this column — conservatively might match + return true; + }; + + check_in_bloom_filter(sbbf, datum, *physical_type) + } +} + +/// Collects field IDs that appear in `eq` or `in` predicates — the only +/// predicate types that benefit from bloom filter checks. +pub(crate) fn collect_bloom_filter_field_ids( + predicate: &BoundPredicate, +) -> Result> { + let mut visitor = EqualityFieldIdCollector { + field_ids: HashSet::new(), + }; + visit(&mut visitor, predicate)?; + Ok(visitor.field_ids) +} + +struct EqualityFieldIdCollector { + field_ids: HashSet, +} + +impl BoundPredicateVisitor for EqualityFieldIdCollector { + type T = (); + + fn always_true(&mut self) -> Result<()> { + Ok(()) + } + + fn always_false(&mut self) -> Result<()> { + Ok(()) + } + + fn and(&mut self, _lhs: (), _rhs: ()) -> Result<()> { + Ok(()) + } + + fn or(&mut self, _lhs: (), _rhs: ()) -> Result<()> { + Ok(()) + } + + fn not(&mut self, _inner: ()) -> Result<()> { + Ok(()) + } + + fn is_null(&mut self, _r: &BoundReference, _p: &BoundPredicate) -> Result<()> { + Ok(()) + } + + fn not_null(&mut self, _r: &BoundReference, _p: &BoundPredicate) -> Result<()> { + Ok(()) + } + + fn is_nan(&mut self, _r: &BoundReference, _p: &BoundPredicate) -> Result<()> { + Ok(()) + } + + fn not_nan(&mut self, _r: &BoundReference, _p: &BoundPredicate) -> Result<()> { + Ok(()) + } + + fn less_than(&mut self, _r: &BoundReference, _l: &Datum, _p: &BoundPredicate) -> Result<()> { + Ok(()) + } + + fn less_than_or_eq( + &mut self, + _r: &BoundReference, + _l: &Datum, + _p: &BoundPredicate, + ) -> Result<()> { + Ok(()) + } + + fn greater_than( + &mut self, + _r: &BoundReference, + _l: &Datum, + _p: &BoundPredicate, + ) -> Result<()> { + Ok(()) + } + + fn greater_than_or_eq( + &mut self, + _r: &BoundReference, + _l: &Datum, + _p: &BoundPredicate, + ) -> Result<()> { + Ok(()) + } + + fn eq(&mut self, r: &BoundReference, _l: &Datum, _p: &BoundPredicate) -> Result<()> { + self.field_ids.insert(r.field().id); + Ok(()) + } + + fn not_eq(&mut self, _r: &BoundReference, _l: &Datum, _p: &BoundPredicate) -> Result<()> { + Ok(()) + } + + fn starts_with( + &mut self, + _r: &BoundReference, + _l: &Datum, + _p: &BoundPredicate, + ) -> Result<()> { + Ok(()) + } + + fn not_starts_with( + &mut self, + _r: &BoundReference, + _l: &Datum, + _p: &BoundPredicate, + ) -> Result<()> { + Ok(()) + } + + fn r#in( + &mut self, + r: &BoundReference, + _literals: &FnvHashSet, + _p: &BoundPredicate, + ) -> Result<()> { + self.field_ids.insert(r.field().id); + Ok(()) + } + + fn not_in( + &mut self, + _r: &BoundReference, + _literals: &FnvHashSet, + _p: &BoundPredicate, + ) -> Result<()> { + Ok(()) + } +} + +/// Check whether a datum value might be present in the bloom filter. +/// +/// The value must be checked using the same physical encoding the Parquet +/// writer used when inserting into the bloom filter. We use the actual +/// physical type from the column metadata (not inferred from precision) +/// to ensure correctness regardless of which writer produced the file. +/// +/// The encoding for each physical type is defined by the Parquet column +/// writer in `parquet::column::writer::encoder`: +/// +/// `bloom_filter.insert(value)` where `value` is the physical type `T::T` +/// +/// The arrow writer's conversion from logical to physical is in +/// `parquet::arrow::arrow_writer`: +/// +/// - **INT32**: `|v: i128| v as i32` +/// - **INT64**: `|v: i128| v as i64` +/// - **FIXED_LEN_BYTE_ARRAY**: `i128.to_be_bytes()[(16-size)..]` +fn check_in_bloom_filter(sbbf: &Sbbf, datum: &Datum, physical_type: PhysicalType) -> bool { + match datum.literal() { + PrimitiveLiteral::Boolean(v) => sbbf.check(v), + PrimitiveLiteral::Int(v) => sbbf.check(v), + PrimitiveLiteral::Long(v) => sbbf.check(v), + PrimitiveLiteral::Float(v) => sbbf.check(&v.0), + PrimitiveLiteral::Double(v) => sbbf.check(&v.0), + PrimitiveLiteral::String(v) => sbbf.check(v.as_str()), + PrimitiveLiteral::Binary(v) => sbbf.check(v.as_slice()), + PrimitiveLiteral::Int128(v) => { + // Decimal: dispatch based on the actual Parquet physical type + // from the file, not inferred from precision. + match physical_type { + PhysicalType::INT32 => sbbf.check(&(*v as i32)), + PhysicalType::INT64 => sbbf.check(&(*v as i64)), + PhysicalType::FIXED_LEN_BYTE_ARRAY => { + // to_be_bytes() truncated to the column's fixed length. + // We use the Iceberg type's precision to determine the + // byte length, which must match the file's fixed length. + let crate::spec::PrimitiveType::Decimal { precision, .. } = + datum.data_type() + else { + return true; + }; + let bytes = decimal_to_fixed_length_bytes(*v, *precision); + sbbf.check(&ByteArray::from(bytes)) + } + _ => true, // Unexpected physical type — conservatively might match + } + } + PrimitiveLiteral::UInt128(v) => { + // UUID: stored as FIXED_LEN_BYTE_ARRAY(16), big-endian + let bytes = v.to_be_bytes(); + sbbf.check(&ByteArray::from(bytes.to_vec())) + } + PrimitiveLiteral::AboveMax | PrimitiveLiteral::BelowMin => true, + } +} + +impl<'a> BoundPredicateVisitor for BloomFilterEvaluator<'a> { + type T = bool; + + fn always_true(&mut self) -> Result { + ROW_GROUP_MIGHT_MATCH + } + + fn always_false(&mut self) -> Result { + ROW_GROUP_CANT_MATCH + } + + fn and(&mut self, lhs: Self::T, rhs: Self::T) -> Result { + Ok(lhs && rhs) + } + + fn or(&mut self, lhs: Self::T, rhs: Self::T) -> Result { + Ok(lhs || rhs) + } + + fn not(&mut self, _inner: Self::T) -> Result { + // Bloom filters are not invertible — we cannot prove presence, + // so NOT of any result must conservatively return "might match". + ROW_GROUP_MIGHT_MATCH + } + + fn is_null( + &mut self, + _reference: &BoundReference, + _predicate: &BoundPredicate, + ) -> Result { + ROW_GROUP_MIGHT_MATCH + } + + fn not_null( + &mut self, + _reference: &BoundReference, + _predicate: &BoundPredicate, + ) -> Result { + ROW_GROUP_MIGHT_MATCH + } + + fn is_nan( + &mut self, + _reference: &BoundReference, + _predicate: &BoundPredicate, + ) -> Result { + ROW_GROUP_MIGHT_MATCH + } + + fn not_nan( + &mut self, + _reference: &BoundReference, + _predicate: &BoundPredicate, + ) -> Result { + ROW_GROUP_MIGHT_MATCH + } + + fn less_than( + &mut self, + _reference: &BoundReference, + _literal: &Datum, + _predicate: &BoundPredicate, + ) -> Result { + ROW_GROUP_MIGHT_MATCH + } + + fn less_than_or_eq( + &mut self, + _reference: &BoundReference, + _literal: &Datum, + _predicate: &BoundPredicate, + ) -> Result { + ROW_GROUP_MIGHT_MATCH + } + + fn greater_than( + &mut self, + _reference: &BoundReference, + _literal: &Datum, + _predicate: &BoundPredicate, + ) -> Result { + ROW_GROUP_MIGHT_MATCH + } + + fn greater_than_or_eq( + &mut self, + _reference: &BoundReference, + _literal: &Datum, + _predicate: &BoundPredicate, + ) -> Result { + ROW_GROUP_MIGHT_MATCH + } + + fn eq( + &mut self, + reference: &BoundReference, + literal: &Datum, + _predicate: &BoundPredicate, + ) -> Result { + if self.check_datum(reference, literal) { + ROW_GROUP_MIGHT_MATCH + } else { + ROW_GROUP_CANT_MATCH + } + } + + fn not_eq( + &mut self, + _reference: &BoundReference, + _literal: &Datum, + _predicate: &BoundPredicate, + ) -> Result { + ROW_GROUP_MIGHT_MATCH + } + + fn starts_with( + &mut self, + _reference: &BoundReference, + _literal: &Datum, + _predicate: &BoundPredicate, + ) -> Result { + ROW_GROUP_MIGHT_MATCH + } + + fn not_starts_with( + &mut self, + _reference: &BoundReference, + _literal: &Datum, + _predicate: &BoundPredicate, + ) -> Result { + ROW_GROUP_MIGHT_MATCH + } + + fn r#in( + &mut self, + reference: &BoundReference, + literals: &FnvHashSet, + _predicate: &BoundPredicate, + ) -> Result { + let field_id = reference.field().id; + let Some((sbbf, physical_type)) = self.bloom_filters.get(&field_id) else { + return ROW_GROUP_MIGHT_MATCH; + }; + + // If ANY literal might be present, the row group might match + for literal in literals { + if check_in_bloom_filter(sbbf, literal, *physical_type) { + return ROW_GROUP_MIGHT_MATCH; + } + } + + // All literals are definitely absent + ROW_GROUP_CANT_MATCH + } + + fn not_in( + &mut self, + _reference: &BoundReference, + _literals: &FnvHashSet, + _predicate: &BoundPredicate, + ) -> Result { + ROW_GROUP_MIGHT_MATCH + } +} + +#[cfg(test)] +mod tests { + use std::collections::HashMap; + use std::ops::Not; + + use parquet::basic::Type as PhysicalType; + use parquet::bloom_filter::Sbbf; + use parquet::data_type::ByteArray; + + use super::BloomFilterEvaluator; + use crate::expr::{Bind, Reference}; + use crate::spec::decimal_utils::decimal_to_fixed_length_bytes; + use crate::spec::{Datum, NestedField, PrimitiveType, Schema, Type}; + + fn create_test_schema() -> Schema { + Schema::builder() + .with_schema_id(1) + .with_fields(vec![ + NestedField::required(1, "id", Type::Primitive(PrimitiveType::Int)).into(), + NestedField::optional(2, "name", Type::Primitive(PrimitiveType::String)).into(), + ]) + .build() + .unwrap() + } + + fn create_bloom_filter_with_values_i32(values: &[i32]) -> Sbbf { + let mut sbbf = Sbbf::new_with_ndv_fpp(values.len() as u64, 0.01).unwrap(); + for v in values { + sbbf.insert(v); + } + sbbf + } + + fn create_bloom_filter_with_values_str(values: &[&str]) -> Sbbf { + let mut sbbf = Sbbf::new_with_ndv_fpp(values.len() as u64, 0.01).unwrap(); + for v in values { + sbbf.insert(*v); + } + sbbf + } + + #[test] + fn test_eq_value_present() { + let schema = create_test_schema(); + let bloom_filters = HashMap::from([(1, (create_bloom_filter_with_values_i32(&[1, 2, 3]), PhysicalType::INT32))]); + + let predicate = Reference::new("id") + .equal_to(Datum::int(2)) + .bind(schema.into(), true) + .unwrap(); + + let result = BloomFilterEvaluator::eval(&predicate, &bloom_filters).unwrap(); + assert!(result, "Row group should might-match when value is present"); + } + + #[test] + fn test_eq_value_absent() { + let schema = create_test_schema(); + let bloom_filters = HashMap::from([(1, (create_bloom_filter_with_values_i32(&[1, 2, 3]), PhysicalType::INT32))]); + + let predicate = Reference::new("id") + .equal_to(Datum::int(999)) + .bind(schema.into(), true) + .unwrap(); + + let result = BloomFilterEvaluator::eval(&predicate, &bloom_filters).unwrap(); + assert!( + !result, + "Row group should not match when value is absent from bloom filter" + ); + } + + #[test] + fn test_eq_no_bloom_filter_for_column() { + let schema = create_test_schema(); + let bloom_filters = HashMap::new(); // No bloom filters + + let predicate = Reference::new("id") + .equal_to(Datum::int(1)) + .bind(schema.into(), true) + .unwrap(); + + let result = BloomFilterEvaluator::eval(&predicate, &bloom_filters).unwrap(); + assert!( + result, + "Row group should might-match when no bloom filter available" + ); + } + + #[test] + fn test_in_all_absent() { + let schema = create_test_schema(); + let bloom_filters = HashMap::from([(1, (create_bloom_filter_with_values_i32(&[1, 2, 3]), PhysicalType::INT32))]); + + let predicate = Reference::new("id") + .is_in([Datum::int(100), Datum::int(200), Datum::int(300)]) + .bind(schema.into(), true) + .unwrap(); + + let result = BloomFilterEvaluator::eval(&predicate, &bloom_filters).unwrap(); + assert!( + !result, + "Row group should not match when all IN values are absent" + ); + } + + #[test] + fn test_in_some_present() { + let schema = create_test_schema(); + let bloom_filters = HashMap::from([(1, (create_bloom_filter_with_values_i32(&[1, 2, 3]), PhysicalType::INT32))]); + + let predicate = Reference::new("id") + .is_in([Datum::int(2), Datum::int(200)]) + .bind(schema.into(), true) + .unwrap(); + + let result = BloomFilterEvaluator::eval(&predicate, &bloom_filters).unwrap(); + assert!( + result, + "Row group should might-match when at least one IN value is present" + ); + } + + #[test] + fn test_and_one_absent() { + let schema = create_test_schema(); + let bloom_filters = HashMap::from([ + (1, (create_bloom_filter_with_values_i32(&[1, 2, 3]), PhysicalType::INT32)), + (2, (create_bloom_filter_with_values_str(&["alice", "bob"]), PhysicalType::BYTE_ARRAY)), + ]); + + // id = 999 AND name = 'alice' + // id=999 is absent, so AND should be false + let predicate = Reference::new("id") + .equal_to(Datum::int(999)) + .and(Reference::new("name").equal_to(Datum::string("alice"))) + .bind(schema.into(), true) + .unwrap(); + + let result = BloomFilterEvaluator::eval(&predicate, &bloom_filters).unwrap(); + assert!( + !result, + "AND should be false when one operand is definitely absent" + ); + } + + #[test] + fn test_or_one_present() { + let schema = create_test_schema(); + let bloom_filters = HashMap::from([(1, (create_bloom_filter_with_values_i32(&[1, 2, 3]), PhysicalType::INT32))]); + + // id = 999 OR id = 2 + // id=2 is present, so OR should be true + let predicate = Reference::new("id") + .equal_to(Datum::int(999)) + .or(Reference::new("id").equal_to(Datum::int(2))) + .bind(schema.into(), true) + .unwrap(); + + let result = BloomFilterEvaluator::eval(&predicate, &bloom_filters).unwrap(); + assert!(result, "OR should be true when one operand might match"); + } + + #[test] + fn test_not_always_might_match() { + let schema = create_test_schema(); + let bloom_filters = HashMap::from([(1, (create_bloom_filter_with_values_i32(&[1, 2, 3]), PhysicalType::INT32))]); + + // NOT(id = 999) — even though 999 is absent, NOT should still return true + let predicate = Reference::new("id") + .equal_to(Datum::int(999)) + .not() + .bind(schema.into(), true) + .unwrap(); + + let result = BloomFilterEvaluator::eval(&predicate, &bloom_filters).unwrap(); + assert!(result, "NOT should always return might-match"); + } + + #[test] + fn test_range_predicates_always_might_match() { + let schema = create_test_schema(); + let bloom_filters = HashMap::from([(1, (create_bloom_filter_with_values_i32(&[1, 2, 3]), PhysicalType::INT32))]); + + let predicate = Reference::new("id") + .less_than(Datum::int(0)) + .bind(schema.into(), true) + .unwrap(); + + let result = BloomFilterEvaluator::eval(&predicate, &bloom_filters).unwrap(); + assert!(result, "Range predicates should always return might-match"); + } + + #[test] + fn test_string_eq_present() { + let schema = create_test_schema(); + let bloom_filters = + HashMap::from([(2, (create_bloom_filter_with_values_str(&["alice", "bob"]), PhysicalType::BYTE_ARRAY))]); + + let predicate = Reference::new("name") + .equal_to(Datum::string("alice")) + .bind(schema.into(), true) + .unwrap(); + + let result = BloomFilterEvaluator::eval(&predicate, &bloom_filters).unwrap(); + assert!(result, "Should might-match when string is in bloom filter"); + } + + #[test] + fn test_string_eq_absent() { + let schema = create_test_schema(); + let bloom_filters = + HashMap::from([(2, (create_bloom_filter_with_values_str(&["alice", "bob"]), PhysicalType::BYTE_ARRAY))]); + + let predicate = Reference::new("name") + .equal_to(Datum::string("charlie")) + .bind(schema.into(), true) + .unwrap(); + + let result = BloomFilterEvaluator::eval(&predicate, &bloom_filters).unwrap(); + assert!( + !result, + "Should not match when string is absent from bloom filter" + ); + } + + // --- Decimal tests --- + + fn create_decimal_schema(precision: u32, scale: u32) -> Schema { + Schema::builder() + .with_schema_id(1) + .with_fields(vec![NestedField::required( + 1, + "amount", + Type::Primitive(PrimitiveType::Decimal { precision, scale }), + ) + .into()]) + .build() + .unwrap() + } + + /// Decimal with precision <= 9 is stored as INT32 in Parquet. + /// The bloom filter contains i32 values (the unscaled mantissa). + #[test] + fn test_decimal_int32_present() { + let schema = create_decimal_schema(9, 2); + + // Parquet stores decimal(9,2) as INT32 with unscaled value + // Value "123.45" has mantissa 12345 + let mut sbbf = Sbbf::new_with_ndv_fpp(10, 0.01).unwrap(); + sbbf.insert(&12345_i32); + sbbf.insert(&67890_i32); + + let bloom_filters = HashMap::from([(1, (sbbf, PhysicalType::INT32))]); + + let predicate = Reference::new("amount") + .equal_to(Datum::decimal_with_precision( + crate::spec::decimal_utils::decimal_from_i128_with_scale(12345, 2), + 9, + ).unwrap()) + .bind(schema.into(), true) + .unwrap(); + + let result = BloomFilterEvaluator::eval(&predicate, &bloom_filters).unwrap(); + assert!(result, "Decimal INT32 value present should might-match"); + } + + #[test] + fn test_decimal_int32_absent() { + let schema = create_decimal_schema(9, 2); + + let mut sbbf = Sbbf::new_with_ndv_fpp(10, 0.01).unwrap(); + sbbf.insert(&12345_i32); + sbbf.insert(&67890_i32); + + let bloom_filters = HashMap::from([(1, (sbbf, PhysicalType::INT32))]); + + // Value "999.99" has mantissa 99999, not in the bloom filter + let predicate = Reference::new("amount") + .equal_to(Datum::decimal_with_precision( + crate::spec::decimal_utils::decimal_from_i128_with_scale(99999, 2), + 9, + ).unwrap()) + .bind(schema.into(), true) + .unwrap(); + + let result = BloomFilterEvaluator::eval(&predicate, &bloom_filters).unwrap(); + assert!(!result, "Decimal INT32 value absent should not match"); + } + + /// Decimal with precision 10-18 is stored as INT64 in Parquet. + #[test] + fn test_decimal_int64_present() { + let schema = create_decimal_schema(15, 2); + + // "1234567890123.45" has mantissa 123456789012345 + let mantissa: i64 = 123456789012345; + let mut sbbf = Sbbf::new_with_ndv_fpp(10, 0.01).unwrap(); + sbbf.insert(&mantissa); + + let bloom_filters = HashMap::from([(1, (sbbf, PhysicalType::INT64))]); + + let predicate = Reference::new("amount") + .equal_to(Datum::decimal_with_precision( + crate::spec::decimal_utils::decimal_from_i128_with_scale(mantissa as i128, 2), + 15, + ).unwrap()) + .bind(schema.into(), true) + .unwrap(); + + let result = BloomFilterEvaluator::eval(&predicate, &bloom_filters).unwrap(); + assert!(result, "Decimal INT64 value present should might-match"); + } + + #[test] + fn test_decimal_int64_absent() { + let schema = create_decimal_schema(15, 2); + + let mut sbbf = Sbbf::new_with_ndv_fpp(10, 0.01).unwrap(); + sbbf.insert(&123456789012345_i64); + + let bloom_filters = HashMap::from([(1, (sbbf, PhysicalType::INT64))]); + + let predicate = Reference::new("amount") + .equal_to(Datum::decimal_with_precision( + crate::spec::decimal_utils::decimal_from_i128_with_scale(999999999999999, 2), + 15, + ).unwrap()) + .bind(schema.into(), true) + .unwrap(); + + let result = BloomFilterEvaluator::eval(&predicate, &bloom_filters).unwrap(); + assert!(!result, "Decimal INT64 value absent should not match"); + } + + /// Decimal with precision 19+ is stored as FIXED_LEN_BYTE_ARRAY in Parquet. + #[test] + fn test_decimal_fixed_bytes_present() { + let schema = create_decimal_schema(25, 2); + + // Large mantissa that requires FIXED_LEN_BYTE_ARRAY + let mantissa: i128 = 12345678901234567890; + let bytes = decimal_to_fixed_length_bytes(mantissa, 25); + + let mut sbbf = Sbbf::new_with_ndv_fpp(10, 0.01).unwrap(); + sbbf.insert(&ByteArray::from(bytes)); + + let bloom_filters = HashMap::from([(1, (sbbf, PhysicalType::FIXED_LEN_BYTE_ARRAY))]); + + let predicate = Reference::new("amount") + .equal_to(Datum::decimal_with_precision( + crate::spec::decimal_utils::decimal_from_i128_with_scale(mantissa, 2), + 25, + ).unwrap()) + .bind(schema.into(), true) + .unwrap(); + + let result = BloomFilterEvaluator::eval(&predicate, &bloom_filters).unwrap(); + assert!( + result, + "Decimal FIXED_LEN_BYTE_ARRAY value present should might-match" + ); + } + + #[test] + fn test_decimal_fixed_bytes_absent() { + let schema = create_decimal_schema(25, 2); + + let mantissa: i128 = 12345678901234567890; + let bytes = decimal_to_fixed_length_bytes(mantissa, 25); + + let mut sbbf = Sbbf::new_with_ndv_fpp(10, 0.01).unwrap(); + sbbf.insert(&ByteArray::from(bytes)); + + let bloom_filters = HashMap::from([(1, (sbbf, PhysicalType::FIXED_LEN_BYTE_ARRAY))]); + + // Different value not in the bloom filter + let predicate = Reference::new("amount") + .equal_to(Datum::decimal_with_precision( + crate::spec::decimal_utils::decimal_from_i128_with_scale(99999999999999999999, 2), + 25, + ).unwrap()) + .bind(schema.into(), true) + .unwrap(); + + let result = BloomFilterEvaluator::eval(&predicate, &bloom_filters).unwrap(); + assert!( + !result, + "Decimal FIXED_LEN_BYTE_ARRAY value absent should not match" + ); + } + + /// Negative decimal values should also work correctly. + #[test] + fn test_decimal_negative_int32() { + let schema = create_decimal_schema(9, 2); + + // "-123.45" has mantissa -12345 + let mut sbbf = Sbbf::new_with_ndv_fpp(10, 0.01).unwrap(); + sbbf.insert(&(-12345_i32)); + + let bloom_filters = HashMap::from([(1, (sbbf, PhysicalType::INT32))]); + + let predicate = Reference::new("amount") + .equal_to(Datum::decimal_with_precision( + crate::spec::decimal_utils::decimal_from_i128_with_scale(-12345, 2), + 9, + ).unwrap()) + .bind(schema.into(), true) + .unwrap(); + + let result = BloomFilterEvaluator::eval(&predicate, &bloom_filters).unwrap(); + assert!(result, "Negative decimal INT32 present should might-match"); + } + + #[test] + fn test_decimal_negative_fixed_bytes() { + let schema = create_decimal_schema(25, 2); + + let mantissa: i128 = -12345678901234567890; + let bytes = decimal_to_fixed_length_bytes(mantissa, 25); + + let mut sbbf = Sbbf::new_with_ndv_fpp(10, 0.01).unwrap(); + sbbf.insert(&ByteArray::from(bytes)); + + let bloom_filters = HashMap::from([(1, (sbbf, PhysicalType::FIXED_LEN_BYTE_ARRAY))]); + + let predicate = Reference::new("amount") + .equal_to(Datum::decimal_with_precision( + crate::spec::decimal_utils::decimal_from_i128_with_scale(mantissa, 2), + 25, + ).unwrap()) + .bind(schema.into(), true) + .unwrap(); + + let result = BloomFilterEvaluator::eval(&predicate, &bloom_filters).unwrap(); + assert!( + result, + "Negative decimal FIXED_LEN_BYTE_ARRAY present should might-match" + ); + } +} diff --git a/crates/iceberg/src/expr/visitors/mod.rs b/crates/iceberg/src/expr/visitors/mod.rs index 38332ea6a0..c5aca6838d 100644 --- a/crates/iceberg/src/expr/visitors/mod.rs +++ b/crates/iceberg/src/expr/visitors/mod.rs @@ -15,6 +15,7 @@ // specific language governing permissions and limitations // under the License. +pub(crate) mod bloom_filter_evaluator; pub(crate) mod bound_predicate_visitor; pub(crate) mod expression_evaluator; pub(crate) mod inclusive_metrics_evaluator; diff --git a/crates/iceberg/src/scan/mod.rs b/crates/iceberg/src/scan/mod.rs index db70806473..a02fe1dedf 100644 --- a/crates/iceberg/src/scan/mod.rs +++ b/crates/iceberg/src/scan/mod.rs @@ -141,6 +141,7 @@ pub struct TableScanBuilder<'a> { concurrency_limit_manifest_files: usize, row_group_filtering_enabled: bool, row_selection_enabled: bool, + bloom_filter_enabled: bool, } impl<'a> TableScanBuilder<'a> { @@ -159,6 +160,7 @@ impl<'a> TableScanBuilder<'a> { concurrency_limit_manifest_files: num_cpus, row_group_filtering_enabled: true, row_selection_enabled: false, + bloom_filter_enabled: false, } } @@ -265,6 +267,20 @@ impl<'a> TableScanBuilder<'a> { self } + /// Determines whether to enable bloom filter-based row group filtering. + /// + /// When enabled, if a read is performed with an equality or IN predicate, + /// the bloom filter for relevant columns in each row group is read and + /// checked. Row groups where the bloom filter proves the value is absent + /// are skipped entirely. + /// + /// Defaults to disabled, as reading bloom filters requires additional I/O + /// per column per row group. + pub fn with_bloom_filter_enabled(mut self, bloom_filter_enabled: bool) -> Self { + self.bloom_filter_enabled = bloom_filter_enabled; + self + } + /// Build the table scan. pub fn build(self) -> Result { let snapshot = match self.snapshot_id { @@ -291,6 +307,7 @@ impl<'a> TableScanBuilder<'a> { concurrency_limit_manifest_files: self.concurrency_limit_manifest_files, row_group_filtering_enabled: self.row_group_filtering_enabled, row_selection_enabled: self.row_selection_enabled, + bloom_filter_enabled: self.bloom_filter_enabled, runtime: self.table.runtime().clone(), }); }; @@ -337,6 +354,7 @@ impl<'a> TableScanBuilder<'a> { concurrency_limit_manifest_files: self.concurrency_limit_manifest_files, row_group_filtering_enabled: self.row_group_filtering_enabled, row_selection_enabled: self.row_selection_enabled, + bloom_filter_enabled: self.bloom_filter_enabled, runtime: self.table.runtime().clone(), }) } @@ -366,6 +384,7 @@ pub struct TableScan { row_group_filtering_enabled: bool, row_selection_enabled: bool, + bloom_filter_enabled: bool, runtime: Runtime, } @@ -498,7 +517,8 @@ impl TableScan { ArrowReaderBuilder::new(self.file_io.clone(), self.runtime.clone()) .with_data_file_concurrency_limit(self.concurrency_limit_data_files) .with_row_group_filtering_enabled(self.row_group_filtering_enabled) - .with_row_selection_enabled(self.row_selection_enabled); + .with_row_selection_enabled(self.row_selection_enabled) + .with_bloom_filter_enabled(self.bloom_filter_enabled); if let Some(batch_size) = self.batch_size { arrow_reader_builder = arrow_reader_builder.with_batch_size(batch_size); diff --git a/crates/iceberg/src/spec/values/decimal_utils.rs b/crates/iceberg/src/spec/values/decimal_utils.rs index 97225113cb..d3496c2091 100644 --- a/crates/iceberg/src/spec/values/decimal_utils.rs +++ b/crates/iceberg/src/spec/values/decimal_utils.rs @@ -196,6 +196,41 @@ pub fn i128_to_be_bytes_min(value: i128) -> Vec { bytes[start..].to_vec() } +/// Encode an i128 decimal value as a fixed-length big-endian byte array, +/// matching how Parquet stores `FIXED_LEN_BYTE_ARRAY` decimals. +/// +/// The result is sign-extended or trimmed to exactly the number of bytes +/// required for the given precision, matching the Java implementation in +/// `DecimalUtil.toReusedFixLengthBytes`. +/// +pub fn decimal_to_fixed_length_bytes(value: i128, precision: u32) -> Vec { + let required_len = parquet_decimal_byte_length(precision); + let be_bytes = value.to_be_bytes(); // 16 bytes, big-endian, two's complement + + if required_len >= 16 { + // Sign-extend to the required length + let fill_byte = if value < 0 { 0xFF } else { 0x00 }; + let mut buf = vec![fill_byte; required_len]; + let offset = required_len - 16; + buf[offset..].copy_from_slice(&be_bytes); + buf + } else { + // Trim leading bytes (value fits in fewer bytes) + let offset = 16 - required_len; + be_bytes[offset..].to_vec() + } +} + +/// Returns the number of bytes required to store a decimal with the given +/// precision as a Parquet `FIXED_LEN_BYTE_ARRAY`. +/// +/// Mirrors `parquet::arrow::schema::decimal_length_from_precision` which is +/// not publicly accessible outside the parquet crate without the `experimental` +/// feature flag. +fn parquet_decimal_byte_length(precision: u32) -> usize { + (((10.0_f64.powi(precision as i32) + 1.0).log2() + 1.0) / 8.0).ceil() as usize +} + #[cfg(test)] mod tests { use super::*; @@ -356,4 +391,61 @@ mod tests { ); } } + + #[test] + fn test_parquet_decimal_byte_length() { + // INT32 range (precision 1-9) should need <= 4 bytes + assert!(parquet_decimal_byte_length(1) <= 4); + assert!(parquet_decimal_byte_length(9) <= 4); + // INT64 range (precision 10-18) should need <= 8 bytes + assert!(parquet_decimal_byte_length(10) <= 8); + assert!(parquet_decimal_byte_length(18) <= 8); + // FIXED_LEN_BYTE_ARRAY range (precision 19+) + assert_eq!(parquet_decimal_byte_length(19), 9); + assert_eq!(parquet_decimal_byte_length(38), 16); + } + + #[test] + fn test_decimal_to_parquet_fixed_bytes_positive() { + // 12345 with precision 20 (requires 9 bytes) + let bytes = decimal_to_fixed_length_bytes(12345, 20); + assert_eq!(bytes.len(), parquet_decimal_byte_length(20)); + // Should be big-endian, zero-padded on the left + assert_eq!(bytes[bytes.len() - 2], 0x30); // 12345 = 0x3039 + assert_eq!(bytes[bytes.len() - 1], 0x39); + // Leading bytes should be 0x00 (positive) + assert!(bytes[..bytes.len() - 2].iter().all(|&b| b == 0x00)); + } + + #[test] + fn test_decimal_to_parquet_fixed_bytes_negative() { + // -1 with precision 20 + let bytes = decimal_to_fixed_length_bytes(-1, 20); + assert_eq!(bytes.len(), parquet_decimal_byte_length(20)); + // All bytes should be 0xFF (-1 in two's complement) + assert!(bytes.iter().all(|&b| b == 0xFF)); + } + + #[test] + fn test_decimal_to_parquet_fixed_bytes_round_trip() { + // Verify that encoding then decoding via i128_from_be_bytes gives back + // the original value + for (value, precision) in [ + (0i128, 20), + (1, 20), + (-1, 20), + (12345, 20), + (-12345, 20), + (i64::MAX as i128, 20), + (i64::MIN as i128, 20), + ] { + let bytes = decimal_to_fixed_length_bytes(value, precision); + let decoded = i128_from_be_bytes(&bytes); + assert_eq!( + decoded, + Some(value), + "Round trip failed for value={value}, precision={precision}" + ); + } + } } From 0671bf1627658e3ba44aa5279ec30ddf5a607379 Mon Sep 17 00:00:00 2001 From: Xander Date: Sat, 2 May 2026 14:02:57 +0100 Subject: [PATCH 02/25] test --- .../expr/visitors/bloom_filter_evaluator.rs | 36 +++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/crates/iceberg/src/expr/visitors/bloom_filter_evaluator.rs b/crates/iceberg/src/expr/visitors/bloom_filter_evaluator.rs index db9c2464df..a33c5cc085 100644 --- a/crates/iceberg/src/expr/visitors/bloom_filter_evaluator.rs +++ b/crates/iceberg/src/expr/visitors/bloom_filter_evaluator.rs @@ -862,4 +862,40 @@ mod tests { "Negative decimal FIXED_LEN_BYTE_ARRAY present should might-match" ); } + + /// A Parquet writer other than arrow-rs (e.g. Spark, Java Parquet) might + /// choose FIXED_LEN_BYTE_ARRAY for a low-precision decimal rather than INT32. + /// The evaluator must use the physical type from the file metadata, not assume + /// a mapping based on precision. + #[test] + fn test_decimal_low_precision_stored_as_fixed_len_byte_array() { + let schema = create_decimal_schema(5, 2); + + // Simulate a file where decimal(5,2) was stored as FIXED_LEN_BYTE_ARRAY + let mantissa: i128 = 12345; + let bytes = decimal_to_fixed_length_bytes(mantissa, 5); + + let mut sbbf = Sbbf::new_with_ndv_fpp(10, 0.01).unwrap(); + sbbf.insert(&ByteArray::from(bytes)); + + let bloom_filters = + HashMap::from([(1, (sbbf, PhysicalType::FIXED_LEN_BYTE_ARRAY))]); + + let predicate = Reference::new("amount") + .equal_to( + Datum::decimal_with_precision( + crate::spec::decimal_utils::decimal_from_i128_with_scale(mantissa, 2), + 5, + ) + .unwrap(), + ) + .bind(schema.into(), true) + .unwrap(); + + let result = BloomFilterEvaluator::eval(&predicate, &bloom_filters).unwrap(); + assert!( + result, + "Should match when physical type is FIXED_LEN_BYTE_ARRAY even for low-precision decimal" + ); + } } From 87146af190faef1c9b83de8a9d03e7d0893c1a5f Mon Sep 17 00:00:00 2001 From: Xander Date: Sat, 2 May 2026 14:17:07 +0100 Subject: [PATCH 03/25] avoid copy --- crates/iceberg/src/arrow/reader/pipeline.rs | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/crates/iceberg/src/arrow/reader/pipeline.rs b/crates/iceberg/src/arrow/reader/pipeline.rs index 609143d00f..abac6fb119 100644 --- a/crates/iceberg/src/arrow/reader/pipeline.rs +++ b/crates/iceberg/src/arrow/reader/pipeline.rs @@ -638,14 +638,20 @@ impl FileScanTaskReader { } if self.bloom_filter_enabled { - let candidate_rgs = selected_row_group_indices.clone().unwrap_or_else(|| { - (0..record_batch_stream_builder.metadata().num_row_groups()).collect() - }); + let all_rgs; + let candidate_rgs = match &selected_row_group_indices { + Some(indices) => indices.as_slice(), + None => { + all_rgs = (0..record_batch_stream_builder.metadata().num_row_groups()) + .collect::>(); + &all_rgs + } + }; let bloom_filtered = Self::filter_row_groups_by_bloom_filter( &predicate, &mut record_batch_stream_builder, - &candidate_rgs, + candidate_rgs, &field_id_map, ) .await?; From 73c4fddb387a408cd5b3f0135f85767ef6c6a89f Mon Sep 17 00:00:00 2001 From: Xander Date: Sat, 2 May 2026 14:24:25 +0100 Subject: [PATCH 04/25] new name --- crates/iceberg/src/expr/visitors/bloom_filter_evaluator.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/crates/iceberg/src/expr/visitors/bloom_filter_evaluator.rs b/crates/iceberg/src/expr/visitors/bloom_filter_evaluator.rs index a33c5cc085..7ba5aae185 100644 --- a/crates/iceberg/src/expr/visitors/bloom_filter_evaluator.rs +++ b/crates/iceberg/src/expr/visitors/bloom_filter_evaluator.rs @@ -71,18 +71,18 @@ impl<'a> BloomFilterEvaluator<'a> { pub(crate) fn collect_bloom_filter_field_ids( predicate: &BoundPredicate, ) -> Result> { - let mut visitor = EqualityFieldIdCollector { + let mut visitor = BloomFilterFieldIdCollector { field_ids: HashSet::new(), }; visit(&mut visitor, predicate)?; Ok(visitor.field_ids) } -struct EqualityFieldIdCollector { +struct BloomFilterFieldIdCollector { field_ids: HashSet, } -impl BoundPredicateVisitor for EqualityFieldIdCollector { +impl BoundPredicateVisitor for BloomFilterFieldIdCollector { type T = (); fn always_true(&mut self) -> Result<()> { From 15595710635610eabbcd24b24ef68d4510446cda Mon Sep 17 00:00:00 2001 From: Xander Date: Sat, 2 May 2026 14:24:32 +0100 Subject: [PATCH 05/25] fmt --- crates/iceberg/src/arrow/reader/pipeline.rs | 6 +- .../expr/visitors/bloom_filter_evaluator.rs | 219 ++++++++++++------ .../iceberg/src/spec/values/decimal_utils.rs | 1 - 3 files changed, 153 insertions(+), 73 deletions(-) diff --git a/crates/iceberg/src/arrow/reader/pipeline.rs b/crates/iceberg/src/arrow/reader/pipeline.rs index abac6fb119..7949556dd4 100644 --- a/crates/iceberg/src/arrow/reader/pipeline.rs +++ b/crates/iceberg/src/arrow/reader/pipeline.rs @@ -744,8 +744,10 @@ impl FileScanTaskReader { let mut result = Vec::with_capacity(candidate_row_groups.len()); for &rg_idx in candidate_row_groups { - let mut bloom_filters: HashMap = - HashMap::new(); + let mut bloom_filters: HashMap< + i32, + (parquet::bloom_filter::Sbbf, parquet::basic::Type), + > = HashMap::new(); for &field_id in &bloom_filter_field_ids { let col_idx = field_id_map[&field_id]; diff --git a/crates/iceberg/src/expr/visitors/bloom_filter_evaluator.rs b/crates/iceberg/src/expr/visitors/bloom_filter_evaluator.rs index 7ba5aae185..ddb9719bab 100644 --- a/crates/iceberg/src/expr/visitors/bloom_filter_evaluator.rs +++ b/crates/iceberg/src/expr/visitors/bloom_filter_evaluator.rs @@ -68,9 +68,7 @@ impl<'a> BloomFilterEvaluator<'a> { /// Collects field IDs that appear in `eq` or `in` predicates — the only /// predicate types that benefit from bloom filter checks. -pub(crate) fn collect_bloom_filter_field_ids( - predicate: &BoundPredicate, -) -> Result> { +pub(crate) fn collect_bloom_filter_field_ids(predicate: &BoundPredicate) -> Result> { let mut visitor = BloomFilterFieldIdCollector { field_ids: HashSet::new(), }; @@ -134,12 +132,7 @@ impl BoundPredicateVisitor for BloomFilterFieldIdCollector { Ok(()) } - fn greater_than( - &mut self, - _r: &BoundReference, - _l: &Datum, - _p: &BoundPredicate, - ) -> Result<()> { + fn greater_than(&mut self, _r: &BoundReference, _l: &Datum, _p: &BoundPredicate) -> Result<()> { Ok(()) } @@ -161,12 +154,7 @@ impl BoundPredicateVisitor for BloomFilterFieldIdCollector { Ok(()) } - fn starts_with( - &mut self, - _r: &BoundReference, - _l: &Datum, - _p: &BoundPredicate, - ) -> Result<()> { + fn starts_with(&mut self, _r: &BoundReference, _l: &Datum, _p: &BoundPredicate) -> Result<()> { Ok(()) } @@ -236,8 +224,7 @@ fn check_in_bloom_filter(sbbf: &Sbbf, datum: &Datum, physical_type: PhysicalType // to_be_bytes() truncated to the column's fixed length. // We use the Iceberg type's precision to determine the // byte length, which must match the file's fixed length. - let crate::spec::PrimitiveType::Decimal { precision, .. } = - datum.data_type() + let crate::spec::PrimitiveType::Decimal { precision, .. } = datum.data_type() else { return true; }; @@ -465,7 +452,13 @@ mod tests { #[test] fn test_eq_value_present() { let schema = create_test_schema(); - let bloom_filters = HashMap::from([(1, (create_bloom_filter_with_values_i32(&[1, 2, 3]), PhysicalType::INT32))]); + let bloom_filters = HashMap::from([( + 1, + ( + create_bloom_filter_with_values_i32(&[1, 2, 3]), + PhysicalType::INT32, + ), + )]); let predicate = Reference::new("id") .equal_to(Datum::int(2)) @@ -479,7 +472,13 @@ mod tests { #[test] fn test_eq_value_absent() { let schema = create_test_schema(); - let bloom_filters = HashMap::from([(1, (create_bloom_filter_with_values_i32(&[1, 2, 3]), PhysicalType::INT32))]); + let bloom_filters = HashMap::from([( + 1, + ( + create_bloom_filter_with_values_i32(&[1, 2, 3]), + PhysicalType::INT32, + ), + )]); let predicate = Reference::new("id") .equal_to(Datum::int(999)) @@ -513,7 +512,13 @@ mod tests { #[test] fn test_in_all_absent() { let schema = create_test_schema(); - let bloom_filters = HashMap::from([(1, (create_bloom_filter_with_values_i32(&[1, 2, 3]), PhysicalType::INT32))]); + let bloom_filters = HashMap::from([( + 1, + ( + create_bloom_filter_with_values_i32(&[1, 2, 3]), + PhysicalType::INT32, + ), + )]); let predicate = Reference::new("id") .is_in([Datum::int(100), Datum::int(200), Datum::int(300)]) @@ -530,7 +535,13 @@ mod tests { #[test] fn test_in_some_present() { let schema = create_test_schema(); - let bloom_filters = HashMap::from([(1, (create_bloom_filter_with_values_i32(&[1, 2, 3]), PhysicalType::INT32))]); + let bloom_filters = HashMap::from([( + 1, + ( + create_bloom_filter_with_values_i32(&[1, 2, 3]), + PhysicalType::INT32, + ), + )]); let predicate = Reference::new("id") .is_in([Datum::int(2), Datum::int(200)]) @@ -548,8 +559,20 @@ mod tests { fn test_and_one_absent() { let schema = create_test_schema(); let bloom_filters = HashMap::from([ - (1, (create_bloom_filter_with_values_i32(&[1, 2, 3]), PhysicalType::INT32)), - (2, (create_bloom_filter_with_values_str(&["alice", "bob"]), PhysicalType::BYTE_ARRAY)), + ( + 1, + ( + create_bloom_filter_with_values_i32(&[1, 2, 3]), + PhysicalType::INT32, + ), + ), + ( + 2, + ( + create_bloom_filter_with_values_str(&["alice", "bob"]), + PhysicalType::BYTE_ARRAY, + ), + ), ]); // id = 999 AND name = 'alice' @@ -570,7 +593,13 @@ mod tests { #[test] fn test_or_one_present() { let schema = create_test_schema(); - let bloom_filters = HashMap::from([(1, (create_bloom_filter_with_values_i32(&[1, 2, 3]), PhysicalType::INT32))]); + let bloom_filters = HashMap::from([( + 1, + ( + create_bloom_filter_with_values_i32(&[1, 2, 3]), + PhysicalType::INT32, + ), + )]); // id = 999 OR id = 2 // id=2 is present, so OR should be true @@ -587,7 +616,13 @@ mod tests { #[test] fn test_not_always_might_match() { let schema = create_test_schema(); - let bloom_filters = HashMap::from([(1, (create_bloom_filter_with_values_i32(&[1, 2, 3]), PhysicalType::INT32))]); + let bloom_filters = HashMap::from([( + 1, + ( + create_bloom_filter_with_values_i32(&[1, 2, 3]), + PhysicalType::INT32, + ), + )]); // NOT(id = 999) — even though 999 is absent, NOT should still return true let predicate = Reference::new("id") @@ -603,7 +638,13 @@ mod tests { #[test] fn test_range_predicates_always_might_match() { let schema = create_test_schema(); - let bloom_filters = HashMap::from([(1, (create_bloom_filter_with_values_i32(&[1, 2, 3]), PhysicalType::INT32))]); + let bloom_filters = HashMap::from([( + 1, + ( + create_bloom_filter_with_values_i32(&[1, 2, 3]), + PhysicalType::INT32, + ), + )]); let predicate = Reference::new("id") .less_than(Datum::int(0)) @@ -617,8 +658,13 @@ mod tests { #[test] fn test_string_eq_present() { let schema = create_test_schema(); - let bloom_filters = - HashMap::from([(2, (create_bloom_filter_with_values_str(&["alice", "bob"]), PhysicalType::BYTE_ARRAY))]); + let bloom_filters = HashMap::from([( + 2, + ( + create_bloom_filter_with_values_str(&["alice", "bob"]), + PhysicalType::BYTE_ARRAY, + ), + )]); let predicate = Reference::new("name") .equal_to(Datum::string("alice")) @@ -632,8 +678,13 @@ mod tests { #[test] fn test_string_eq_absent() { let schema = create_test_schema(); - let bloom_filters = - HashMap::from([(2, (create_bloom_filter_with_values_str(&["alice", "bob"]), PhysicalType::BYTE_ARRAY))]); + let bloom_filters = HashMap::from([( + 2, + ( + create_bloom_filter_with_values_str(&["alice", "bob"]), + PhysicalType::BYTE_ARRAY, + ), + )]); let predicate = Reference::new("name") .equal_to(Datum::string("charlie")) @@ -652,12 +703,14 @@ mod tests { fn create_decimal_schema(precision: u32, scale: u32) -> Schema { Schema::builder() .with_schema_id(1) - .with_fields(vec![NestedField::required( - 1, - "amount", - Type::Primitive(PrimitiveType::Decimal { precision, scale }), - ) - .into()]) + .with_fields(vec![ + NestedField::required( + 1, + "amount", + Type::Primitive(PrimitiveType::Decimal { precision, scale }), + ) + .into(), + ]) .build() .unwrap() } @@ -677,10 +730,13 @@ mod tests { let bloom_filters = HashMap::from([(1, (sbbf, PhysicalType::INT32))]); let predicate = Reference::new("amount") - .equal_to(Datum::decimal_with_precision( - crate::spec::decimal_utils::decimal_from_i128_with_scale(12345, 2), - 9, - ).unwrap()) + .equal_to( + Datum::decimal_with_precision( + crate::spec::decimal_utils::decimal_from_i128_with_scale(12345, 2), + 9, + ) + .unwrap(), + ) .bind(schema.into(), true) .unwrap(); @@ -700,10 +756,13 @@ mod tests { // Value "999.99" has mantissa 99999, not in the bloom filter let predicate = Reference::new("amount") - .equal_to(Datum::decimal_with_precision( - crate::spec::decimal_utils::decimal_from_i128_with_scale(99999, 2), - 9, - ).unwrap()) + .equal_to( + Datum::decimal_with_precision( + crate::spec::decimal_utils::decimal_from_i128_with_scale(99999, 2), + 9, + ) + .unwrap(), + ) .bind(schema.into(), true) .unwrap(); @@ -724,10 +783,13 @@ mod tests { let bloom_filters = HashMap::from([(1, (sbbf, PhysicalType::INT64))]); let predicate = Reference::new("amount") - .equal_to(Datum::decimal_with_precision( - crate::spec::decimal_utils::decimal_from_i128_with_scale(mantissa as i128, 2), - 15, - ).unwrap()) + .equal_to( + Datum::decimal_with_precision( + crate::spec::decimal_utils::decimal_from_i128_with_scale(mantissa as i128, 2), + 15, + ) + .unwrap(), + ) .bind(schema.into(), true) .unwrap(); @@ -745,10 +807,13 @@ mod tests { let bloom_filters = HashMap::from([(1, (sbbf, PhysicalType::INT64))]); let predicate = Reference::new("amount") - .equal_to(Datum::decimal_with_precision( - crate::spec::decimal_utils::decimal_from_i128_with_scale(999999999999999, 2), - 15, - ).unwrap()) + .equal_to( + Datum::decimal_with_precision( + crate::spec::decimal_utils::decimal_from_i128_with_scale(999999999999999, 2), + 15, + ) + .unwrap(), + ) .bind(schema.into(), true) .unwrap(); @@ -771,10 +836,13 @@ mod tests { let bloom_filters = HashMap::from([(1, (sbbf, PhysicalType::FIXED_LEN_BYTE_ARRAY))]); let predicate = Reference::new("amount") - .equal_to(Datum::decimal_with_precision( - crate::spec::decimal_utils::decimal_from_i128_with_scale(mantissa, 2), - 25, - ).unwrap()) + .equal_to( + Datum::decimal_with_precision( + crate::spec::decimal_utils::decimal_from_i128_with_scale(mantissa, 2), + 25, + ) + .unwrap(), + ) .bind(schema.into(), true) .unwrap(); @@ -799,10 +867,16 @@ mod tests { // Different value not in the bloom filter let predicate = Reference::new("amount") - .equal_to(Datum::decimal_with_precision( - crate::spec::decimal_utils::decimal_from_i128_with_scale(99999999999999999999, 2), - 25, - ).unwrap()) + .equal_to( + Datum::decimal_with_precision( + crate::spec::decimal_utils::decimal_from_i128_with_scale( + 99999999999999999999, + 2, + ), + 25, + ) + .unwrap(), + ) .bind(schema.into(), true) .unwrap(); @@ -825,10 +899,13 @@ mod tests { let bloom_filters = HashMap::from([(1, (sbbf, PhysicalType::INT32))]); let predicate = Reference::new("amount") - .equal_to(Datum::decimal_with_precision( - crate::spec::decimal_utils::decimal_from_i128_with_scale(-12345, 2), - 9, - ).unwrap()) + .equal_to( + Datum::decimal_with_precision( + crate::spec::decimal_utils::decimal_from_i128_with_scale(-12345, 2), + 9, + ) + .unwrap(), + ) .bind(schema.into(), true) .unwrap(); @@ -849,10 +926,13 @@ mod tests { let bloom_filters = HashMap::from([(1, (sbbf, PhysicalType::FIXED_LEN_BYTE_ARRAY))]); let predicate = Reference::new("amount") - .equal_to(Datum::decimal_with_precision( - crate::spec::decimal_utils::decimal_from_i128_with_scale(mantissa, 2), - 25, - ).unwrap()) + .equal_to( + Datum::decimal_with_precision( + crate::spec::decimal_utils::decimal_from_i128_with_scale(mantissa, 2), + 25, + ) + .unwrap(), + ) .bind(schema.into(), true) .unwrap(); @@ -878,8 +958,7 @@ mod tests { let mut sbbf = Sbbf::new_with_ndv_fpp(10, 0.01).unwrap(); sbbf.insert(&ByteArray::from(bytes)); - let bloom_filters = - HashMap::from([(1, (sbbf, PhysicalType::FIXED_LEN_BYTE_ARRAY))]); + let bloom_filters = HashMap::from([(1, (sbbf, PhysicalType::FIXED_LEN_BYTE_ARRAY))]); let predicate = Reference::new("amount") .equal_to( diff --git a/crates/iceberg/src/spec/values/decimal_utils.rs b/crates/iceberg/src/spec/values/decimal_utils.rs index d3496c2091..ae962cee05 100644 --- a/crates/iceberg/src/spec/values/decimal_utils.rs +++ b/crates/iceberg/src/spec/values/decimal_utils.rs @@ -202,7 +202,6 @@ pub fn i128_to_be_bytes_min(value: i128) -> Vec { /// The result is sign-extended or trimmed to exactly the number of bytes /// required for the given precision, matching the Java implementation in /// `DecimalUtil.toReusedFixLengthBytes`. -/// pub fn decimal_to_fixed_length_bytes(value: i128, precision: u32) -> Vec { let required_len = parquet_decimal_byte_length(precision); let be_bytes = value.to_be_bytes(); // 16 bytes, big-endian, two's complement From 58ad7761436bd6ab278317909afdbe465828f1cb Mon Sep 17 00:00:00 2001 From: Xander Date: Sat, 2 May 2026 14:25:52 +0100 Subject: [PATCH 06/25] doc string --- .../src/expr/visitors/bloom_filter_evaluator.rs | 16 ++-------------- 1 file changed, 2 insertions(+), 14 deletions(-) diff --git a/crates/iceberg/src/expr/visitors/bloom_filter_evaluator.rs b/crates/iceberg/src/expr/visitors/bloom_filter_evaluator.rs index ddb9719bab..7b94ec1690 100644 --- a/crates/iceberg/src/expr/visitors/bloom_filter_evaluator.rs +++ b/crates/iceberg/src/expr/visitors/bloom_filter_evaluator.rs @@ -191,20 +191,8 @@ impl BoundPredicateVisitor for BloomFilterFieldIdCollector { /// /// The value must be checked using the same physical encoding the Parquet /// writer used when inserting into the bloom filter. We use the actual -/// physical type from the column metadata (not inferred from precision) -/// to ensure correctness regardless of which writer produced the file. -/// -/// The encoding for each physical type is defined by the Parquet column -/// writer in `parquet::column::writer::encoder`: -/// -/// `bloom_filter.insert(value)` where `value` is the physical type `T::T` -/// -/// The arrow writer's conversion from logical to physical is in -/// `parquet::arrow::arrow_writer`: -/// -/// - **INT32**: `|v: i128| v as i32` -/// - **INT64**: `|v: i128| v as i64` -/// - **FIXED_LEN_BYTE_ARRAY**: `i128.to_be_bytes()[(16-size)..]` +/// physical type from the column metadata to ensure correctness regardless +/// of which writer produced the file. fn check_in_bloom_filter(sbbf: &Sbbf, datum: &Datum, physical_type: PhysicalType) -> bool { match datum.literal() { PrimitiveLiteral::Boolean(v) => sbbf.check(v), From e23e73327c2cef6c35c0af795ed543756a3ecbbe Mon Sep 17 00:00:00 2001 From: Xander Date: Wed, 9 Sep 2026 09:30:58 +0100 Subject: [PATCH 07/25] handle type widening --- .../expr/visitors/bloom_filter_evaluator.rs | 193 +++++++++++++++++- 1 file changed, 189 insertions(+), 4 deletions(-) diff --git a/crates/iceberg/src/expr/visitors/bloom_filter_evaluator.rs b/crates/iceberg/src/expr/visitors/bloom_filter_evaluator.rs index 7b94ec1690..7f4136e047 100644 --- a/crates/iceberg/src/expr/visitors/bloom_filter_evaluator.rs +++ b/crates/iceberg/src/expr/visitors/bloom_filter_evaluator.rs @@ -196,10 +196,42 @@ impl BoundPredicateVisitor for BloomFilterFieldIdCollector { fn check_in_bloom_filter(sbbf: &Sbbf, datum: &Datum, physical_type: PhysicalType) -> bool { match datum.literal() { PrimitiveLiteral::Boolean(v) => sbbf.check(v), - PrimitiveLiteral::Int(v) => sbbf.check(v), - PrimitiveLiteral::Long(v) => sbbf.check(v), - PrimitiveLiteral::Float(v) => sbbf.check(&v.0), - PrimitiveLiteral::Double(v) => sbbf.check(&v.0), + // A promoted column (int -> long, float -> double) keeps its original + // physical width in files written before the promotion, and the writer + // hashed that width, so probe at the file's width rather than the + // predicate's. + PrimitiveLiteral::Int(v) => match physical_type { + PhysicalType::INT32 => sbbf.check(v), + PhysicalType::INT64 => sbbf.check(&i64::from(*v)), + _ => true, + }, + PrimitiveLiteral::Long(v) => match physical_type { + PhysicalType::INT64 => sbbf.check(v), + PhysicalType::INT32 => match i32::try_from(*v) { + Ok(narrowed) => sbbf.check(&narrowed), + // Out of range for the column, so it cannot be present. + Err(_) => true, + }, + _ => true, + }, + PrimitiveLiteral::Float(v) => match physical_type { + PhysicalType::FLOAT => sbbf.check(&v.0), + PhysicalType::DOUBLE => sbbf.check(&f64::from(v.0)), + _ => true, + }, + PrimitiveLiteral::Double(v) => match physical_type { + PhysicalType::DOUBLE => sbbf.check(&v.0), + PhysicalType::FLOAT => { + let narrowed = v.0 as f32; + // Only an exactly representable value can equal a widened f32. + if f64::from(narrowed) == v.0 { + sbbf.check(&narrowed) + } else { + true + } + } + _ => true, + }, PrimitiveLiteral::String(v) => sbbf.check(v.as_str()), PrimitiveLiteral::Binary(v) => sbbf.check(v.as_slice()), PrimitiveLiteral::Int128(v) => { @@ -965,4 +997,157 @@ mod tests { "Should match when physical type is FIXED_LEN_BYTE_ARRAY even for low-precision decimal" ); } + + fn single_field_schema(name: &str, ty: PrimitiveType) -> Schema { + Schema::builder() + .with_schema_id(1) + .with_fields(vec![ + NestedField::required(1, name, Type::Primitive(ty)).into(), + ]) + .build() + .unwrap() + } + + /// After an `int` -> `long` promotion the predicate carries a `long`, but files + /// written before the promotion store the column as `INT32` and hashed it at + /// that width. Probing at the predicate's width made every lookup miss and + /// silently pruned row groups holding matching rows. + #[test] + fn test_promoted_int_to_long_present_is_not_pruned() { + let schema = single_field_schema("id", PrimitiveType::Long); + let bloom_filters = HashMap::from([( + 1, + ( + create_bloom_filter_with_values_i32(&[100, 150, 199]), + PhysicalType::INT32, + ), + )]); + + let predicate = Reference::new("id") + .equal_to(Datum::long(150)) + .bind(schema.into(), true) + .unwrap(); + + let result = BloomFilterEvaluator::eval(&predicate, &bloom_filters).unwrap(); + assert!( + result, + "long predicate against an INT32 column must find the promoted value" + ); + } + + #[test] + fn test_promoted_int_to_long_absent_still_prunes() { + let schema = single_field_schema("id", PrimitiveType::Long); + let bloom_filters = HashMap::from([( + 1, + ( + create_bloom_filter_with_values_i32(&[100, 150, 199]), + PhysicalType::INT32, + ), + )]); + + let predicate = Reference::new("id") + .equal_to(Datum::long(4242)) + .bind(schema.into(), true) + .unwrap(); + + let result = BloomFilterEvaluator::eval(&predicate, &bloom_filters).unwrap(); + assert!(!result, "genuinely absent value must still prune"); + } + + /// A `long` predicate outside `i32` range cannot be present in an `INT32` + /// column; stay conservative rather than probe a truncated value. + #[test] + fn test_long_out_of_int32_range_does_not_prune() { + let schema = single_field_schema("id", PrimitiveType::Long); + let bloom_filters = HashMap::from([( + 1, + ( + create_bloom_filter_with_values_i32(&[1, 2, 3]), + PhysicalType::INT32, + ), + )]); + + let predicate = Reference::new("id") + .equal_to(Datum::long(i64::from(i32::MAX) + 1)) + .bind(schema.into(), true) + .unwrap(); + + let result = BloomFilterEvaluator::eval(&predicate, &bloom_filters).unwrap(); + assert!(result, "out-of-range long must not prune"); + } + + fn create_bloom_filter_with_values_f32(values: &[f32]) -> Sbbf { + let mut sbbf = Sbbf::new_with_ndv_fpp(values.len() as u64, 0.01).unwrap(); + for v in values { + sbbf.insert(v); + } + sbbf + } + + /// The `float` -> `double` counterpart of the `int` -> `long` promotion above. + #[test] + fn test_promoted_float_to_double_present_is_not_pruned() { + let schema = single_field_schema("val", PrimitiveType::Double); + let bloom_filters = HashMap::from([( + 1, + ( + create_bloom_filter_with_values_f32(&[1.5, 2.25, 4.0]), + PhysicalType::FLOAT, + ), + )]); + + let predicate = Reference::new("val") + .equal_to(Datum::double(1.5)) + .bind(schema.into(), true) + .unwrap(); + + let result = BloomFilterEvaluator::eval(&predicate, &bloom_filters).unwrap(); + assert!( + result, + "double predicate against a FLOAT column must find the promoted value" + ); + } + + #[test] + fn test_promoted_float_to_double_absent_still_prunes() { + let schema = single_field_schema("val", PrimitiveType::Double); + let bloom_filters = HashMap::from([( + 1, + ( + create_bloom_filter_with_values_f32(&[1.5, 2.25, 4.0]), + PhysicalType::FLOAT, + ), + )]); + + let predicate = Reference::new("val") + .equal_to(Datum::double(9.75)) + .bind(schema.into(), true) + .unwrap(); + + let result = BloomFilterEvaluator::eval(&predicate, &bloom_filters).unwrap(); + assert!(!result, "genuinely absent value must still prune"); + } + + /// A double with no exact `f32` representation cannot equal any widened + /// `f32` in the column, so probing it would be meaningless; stay conservative. + #[test] + fn test_double_not_representable_as_f32_does_not_prune() { + let schema = single_field_schema("val", PrimitiveType::Double); + let bloom_filters = HashMap::from([( + 1, + ( + create_bloom_filter_with_values_f32(&[1.5, 2.25]), + PhysicalType::FLOAT, + ), + )]); + + let predicate = Reference::new("val") + .equal_to(Datum::double(0.1)) + .bind(schema.into(), true) + .unwrap(); + + let result = BloomFilterEvaluator::eval(&predicate, &bloom_filters).unwrap(); + assert!(result, "non-representable double must not prune"); + } } From 29072aafe092d6ba0b1db22be3f7e5e73abe9113 Mon Sep 17 00:00:00 2001 From: Xander Date: Wed, 9 Sep 2026 09:59:42 +0100 Subject: [PATCH 08/25] widen decimals --- crates/iceberg/src/arrow/reader/pipeline.rs | 13 +- .../expr/visitors/bloom_filter_evaluator.rs | 274 ++++++++++++++---- .../iceberg/src/spec/values/decimal_utils.rs | 151 +++++----- 3 files changed, 310 insertions(+), 128 deletions(-) diff --git a/crates/iceberg/src/arrow/reader/pipeline.rs b/crates/iceberg/src/arrow/reader/pipeline.rs index 7949556dd4..7047b59425 100644 --- a/crates/iceberg/src/arrow/reader/pipeline.rs +++ b/crates/iceberg/src/arrow/reader/pipeline.rs @@ -45,7 +45,7 @@ use crate::arrow::scan_metrics::{CountingFileRead, ScanMetrics, ScanResult}; use crate::encryption::StandardKeyMetadata; use crate::error::Result; use crate::expr::visitors::bloom_filter_evaluator::{ - BloomFilterEvaluator, collect_bloom_filter_field_ids, + BloomFilterEvaluator, ColumnBloomFilter, collect_bloom_filter_field_ids, }; use crate::io::{FileIO, FileMetadata, FileRead}; use crate::metadata_columns::{ @@ -744,10 +744,7 @@ impl FileScanTaskReader { let mut result = Vec::with_capacity(candidate_row_groups.len()); for &rg_idx in candidate_row_groups { - let mut bloom_filters: HashMap< - i32, - (parquet::bloom_filter::Sbbf, parquet::basic::Type), - > = HashMap::new(); + let mut bloom_filters: HashMap = HashMap::new(); for &field_id in &bloom_filter_field_ids { let col_idx = field_id_map[&field_id]; @@ -759,13 +756,17 @@ impl FileScanTaskReader { } let physical_type = col_meta.column_type(); + let type_length = col_meta.column_descr().type_length(); match builder .get_row_group_column_bloom_filter(rg_idx, col_idx) .await { Ok(Some(sbbf)) => { - bloom_filters.insert(field_id, (sbbf, physical_type)); + bloom_filters.insert( + field_id, + ColumnBloomFilter::new(sbbf, physical_type, type_length), + ); } Ok(None) => {} Err(_) => { diff --git a/crates/iceberg/src/expr/visitors/bloom_filter_evaluator.rs b/crates/iceberg/src/expr/visitors/bloom_filter_evaluator.rs index 7f4136e047..562b3c6405 100644 --- a/crates/iceberg/src/expr/visitors/bloom_filter_evaluator.rs +++ b/crates/iceberg/src/expr/visitors/bloom_filter_evaluator.rs @@ -28,15 +28,36 @@ use parquet::data_type::ByteArray; use crate::Result; use crate::expr::visitors::bound_predicate_visitor::{BoundPredicateVisitor, visit}; use crate::expr::{BoundPredicate, BoundReference}; -use crate::spec::decimal_utils::decimal_to_fixed_length_bytes; +use crate::spec::decimal_utils::decimal_to_fixed_length_bytes_exact; use crate::spec::{Datum, PrimitiveLiteral}; const ROW_GROUP_MIGHT_MATCH: Result = Ok(true); const ROW_GROUP_CANT_MATCH: Result = Ok(false); +/// A column's bloom filter for one row group, together with the file's physical +/// encoding of that column. A probe must be encoded the way the writer encoded +/// the values it inserted, so the encoding travels with the filter. +pub(crate) struct ColumnBloomFilter { + sbbf: Sbbf, + physical_type: PhysicalType, + /// `type_length` from the file's column descriptor. Only meaningful for + /// `FIXED_LEN_BYTE_ARRAY`. + type_length: i32, +} + +impl ColumnBloomFilter { + pub(crate) fn new(sbbf: Sbbf, physical_type: PhysicalType, type_length: i32) -> Self { + Self { + sbbf, + physical_type, + type_length, + } + } +} + pub(crate) struct BloomFilterEvaluator<'a> { - /// Maps Iceberg field_id -> (bloom filter, Parquet physical type) for this row group - bloom_filters: &'a HashMap, + /// Maps Iceberg field_id -> bloom filter for this row group + bloom_filters: &'a HashMap, } impl<'a> BloomFilterEvaluator<'a> { @@ -45,7 +66,7 @@ impl<'a> BloomFilterEvaluator<'a> { /// `true` if it might match. pub(crate) fn eval( filter: &BoundPredicate, - bloom_filters: &HashMap, + bloom_filters: &HashMap, ) -> Result { if bloom_filters.is_empty() { return ROW_GROUP_MIGHT_MATCH; @@ -57,12 +78,12 @@ impl<'a> BloomFilterEvaluator<'a> { fn check_datum(&self, reference: &BoundReference, datum: &Datum) -> bool { let field_id = reference.field().id; - let Some((sbbf, physical_type)) = self.bloom_filters.get(&field_id) else { + let Some(column) = self.bloom_filters.get(&field_id) else { // No bloom filter for this column — conservatively might match return true; }; - check_in_bloom_filter(sbbf, datum, *physical_type) + check_in_bloom_filter(column, datum) } } @@ -193,7 +214,14 @@ impl BoundPredicateVisitor for BloomFilterFieldIdCollector { /// writer used when inserting into the bloom filter. We use the actual /// physical type from the column metadata to ensure correctness regardless /// of which writer produced the file. -fn check_in_bloom_filter(sbbf: &Sbbf, datum: &Datum, physical_type: PhysicalType) -> bool { +fn check_in_bloom_filter(column: &ColumnBloomFilter, datum: &Datum) -> bool { + let ColumnBloomFilter { + sbbf, + physical_type, + type_length, + } = column; + let physical_type = *physical_type; + match datum.literal() { PrimitiveLiteral::Boolean(v) => sbbf.check(v), // A promoted column (int -> long, float -> double) keeps its original @@ -241,15 +269,18 @@ fn check_in_bloom_filter(sbbf: &Sbbf, datum: &Datum, physical_type: PhysicalType PhysicalType::INT32 => sbbf.check(&(*v as i32)), PhysicalType::INT64 => sbbf.check(&(*v as i64)), PhysicalType::FIXED_LEN_BYTE_ARRAY => { - // to_be_bytes() truncated to the column's fixed length. - // We use the Iceberg type's precision to determine the - // byte length, which must match the file's fixed length. - let crate::spec::PrimitiveType::Decimal { precision, .. } = datum.data_type() - else { - return true; - }; - let bytes = decimal_to_fixed_length_bytes(*v, *precision); - sbbf.check(&ByteArray::from(bytes)) + // Encode to the file's declared length, not one derived from + // the Iceberg precision: a widened precision would change the + // length and miss every entry the writer inserted. + match usize::try_from(*type_length) + .ok() + .and_then(|len| decimal_to_fixed_length_bytes_exact(*v, len)) + { + Some(bytes) => sbbf.check(&ByteArray::from(bytes)), + // Unusable length, or a value too large for the column to + // hold — conservatively might match. + None => true, + } } _ => true, // Unexpected physical type — conservatively might match } @@ -403,13 +434,13 @@ impl<'a> BoundPredicateVisitor for BloomFilterEvaluator<'a> { _predicate: &BoundPredicate, ) -> Result { let field_id = reference.field().id; - let Some((sbbf, physical_type)) = self.bloom_filters.get(&field_id) else { + let Some(column) = self.bloom_filters.get(&field_id) else { return ROW_GROUP_MIGHT_MATCH; }; // If ANY literal might be present, the row group might match for literal in literals { - if check_in_bloom_filter(sbbf, literal, *physical_type) { + if check_in_bloom_filter(column, literal) { return ROW_GROUP_MIGHT_MATCH; } } @@ -437,9 +468,9 @@ mod tests { use parquet::bloom_filter::Sbbf; use parquet::data_type::ByteArray; - use super::BloomFilterEvaluator; - use crate::expr::{Bind, Reference}; - use crate::spec::decimal_utils::decimal_to_fixed_length_bytes; + use super::{BloomFilterEvaluator, ColumnBloomFilter}; + use crate::expr::{Bind, BoundPredicate, Reference}; + use crate::spec::decimal_utils::decimal_to_fixed_length_bytes_exact; use crate::spec::{Datum, NestedField, PrimitiveType, Schema, Type}; fn create_test_schema() -> Schema { @@ -474,9 +505,10 @@ mod tests { let schema = create_test_schema(); let bloom_filters = HashMap::from([( 1, - ( + ColumnBloomFilter::new( create_bloom_filter_with_values_i32(&[1, 2, 3]), PhysicalType::INT32, + 0, ), )]); @@ -494,9 +526,10 @@ mod tests { let schema = create_test_schema(); let bloom_filters = HashMap::from([( 1, - ( + ColumnBloomFilter::new( create_bloom_filter_with_values_i32(&[1, 2, 3]), PhysicalType::INT32, + 0, ), )]); @@ -534,9 +567,10 @@ mod tests { let schema = create_test_schema(); let bloom_filters = HashMap::from([( 1, - ( + ColumnBloomFilter::new( create_bloom_filter_with_values_i32(&[1, 2, 3]), PhysicalType::INT32, + 0, ), )]); @@ -557,9 +591,10 @@ mod tests { let schema = create_test_schema(); let bloom_filters = HashMap::from([( 1, - ( + ColumnBloomFilter::new( create_bloom_filter_with_values_i32(&[1, 2, 3]), PhysicalType::INT32, + 0, ), )]); @@ -581,16 +616,18 @@ mod tests { let bloom_filters = HashMap::from([ ( 1, - ( + ColumnBloomFilter::new( create_bloom_filter_with_values_i32(&[1, 2, 3]), PhysicalType::INT32, + 0, ), ), ( 2, - ( + ColumnBloomFilter::new( create_bloom_filter_with_values_str(&["alice", "bob"]), PhysicalType::BYTE_ARRAY, + 0, ), ), ]); @@ -615,9 +652,10 @@ mod tests { let schema = create_test_schema(); let bloom_filters = HashMap::from([( 1, - ( + ColumnBloomFilter::new( create_bloom_filter_with_values_i32(&[1, 2, 3]), PhysicalType::INT32, + 0, ), )]); @@ -638,9 +676,10 @@ mod tests { let schema = create_test_schema(); let bloom_filters = HashMap::from([( 1, - ( + ColumnBloomFilter::new( create_bloom_filter_with_values_i32(&[1, 2, 3]), PhysicalType::INT32, + 0, ), )]); @@ -660,9 +699,10 @@ mod tests { let schema = create_test_schema(); let bloom_filters = HashMap::from([( 1, - ( + ColumnBloomFilter::new( create_bloom_filter_with_values_i32(&[1, 2, 3]), PhysicalType::INT32, + 0, ), )]); @@ -680,9 +720,10 @@ mod tests { let schema = create_test_schema(); let bloom_filters = HashMap::from([( 2, - ( + ColumnBloomFilter::new( create_bloom_filter_with_values_str(&["alice", "bob"]), PhysicalType::BYTE_ARRAY, + 0, ), )]); @@ -700,9 +741,10 @@ mod tests { let schema = create_test_schema(); let bloom_filters = HashMap::from([( 2, - ( + ColumnBloomFilter::new( create_bloom_filter_with_values_str(&["alice", "bob"]), PhysicalType::BYTE_ARRAY, + 0, ), )]); @@ -747,7 +789,8 @@ mod tests { sbbf.insert(&12345_i32); sbbf.insert(&67890_i32); - let bloom_filters = HashMap::from([(1, (sbbf, PhysicalType::INT32))]); + let bloom_filters = + HashMap::from([(1, ColumnBloomFilter::new(sbbf, PhysicalType::INT32, 0))]); let predicate = Reference::new("amount") .equal_to( @@ -772,7 +815,8 @@ mod tests { sbbf.insert(&12345_i32); sbbf.insert(&67890_i32); - let bloom_filters = HashMap::from([(1, (sbbf, PhysicalType::INT32))]); + let bloom_filters = + HashMap::from([(1, ColumnBloomFilter::new(sbbf, PhysicalType::INT32, 0))]); // Value "999.99" has mantissa 99999, not in the bloom filter let predicate = Reference::new("amount") @@ -800,7 +844,8 @@ mod tests { let mut sbbf = Sbbf::new_with_ndv_fpp(10, 0.01).unwrap(); sbbf.insert(&mantissa); - let bloom_filters = HashMap::from([(1, (sbbf, PhysicalType::INT64))]); + let bloom_filters = + HashMap::from([(1, ColumnBloomFilter::new(sbbf, PhysicalType::INT64, 0))]); let predicate = Reference::new("amount") .equal_to( @@ -824,7 +869,8 @@ mod tests { let mut sbbf = Sbbf::new_with_ndv_fpp(10, 0.01).unwrap(); sbbf.insert(&123456789012345_i64); - let bloom_filters = HashMap::from([(1, (sbbf, PhysicalType::INT64))]); + let bloom_filters = + HashMap::from([(1, ColumnBloomFilter::new(sbbf, PhysicalType::INT64, 0))]); let predicate = Reference::new("amount") .equal_to( @@ -848,12 +894,16 @@ mod tests { // Large mantissa that requires FIXED_LEN_BYTE_ARRAY let mantissa: i128 = 12345678901234567890; - let bytes = decimal_to_fixed_length_bytes(mantissa, 25); + let bytes = decimal_to_fixed_length_bytes_exact(mantissa, 11).unwrap(); + let type_length = bytes.len() as i32; let mut sbbf = Sbbf::new_with_ndv_fpp(10, 0.01).unwrap(); sbbf.insert(&ByteArray::from(bytes)); - let bloom_filters = HashMap::from([(1, (sbbf, PhysicalType::FIXED_LEN_BYTE_ARRAY))]); + let bloom_filters = HashMap::from([( + 1, + ColumnBloomFilter::new(sbbf, PhysicalType::FIXED_LEN_BYTE_ARRAY, type_length), + )]); let predicate = Reference::new("amount") .equal_to( @@ -878,12 +928,16 @@ mod tests { let schema = create_decimal_schema(25, 2); let mantissa: i128 = 12345678901234567890; - let bytes = decimal_to_fixed_length_bytes(mantissa, 25); + let bytes = decimal_to_fixed_length_bytes_exact(mantissa, 11).unwrap(); + let type_length = bytes.len() as i32; let mut sbbf = Sbbf::new_with_ndv_fpp(10, 0.01).unwrap(); sbbf.insert(&ByteArray::from(bytes)); - let bloom_filters = HashMap::from([(1, (sbbf, PhysicalType::FIXED_LEN_BYTE_ARRAY))]); + let bloom_filters = HashMap::from([( + 1, + ColumnBloomFilter::new(sbbf, PhysicalType::FIXED_LEN_BYTE_ARRAY, type_length), + )]); // Different value not in the bloom filter let predicate = Reference::new("amount") @@ -916,7 +970,8 @@ mod tests { let mut sbbf = Sbbf::new_with_ndv_fpp(10, 0.01).unwrap(); sbbf.insert(&(-12345_i32)); - let bloom_filters = HashMap::from([(1, (sbbf, PhysicalType::INT32))]); + let bloom_filters = + HashMap::from([(1, ColumnBloomFilter::new(sbbf, PhysicalType::INT32, 0))]); let predicate = Reference::new("amount") .equal_to( @@ -938,12 +993,16 @@ mod tests { let schema = create_decimal_schema(25, 2); let mantissa: i128 = -12345678901234567890; - let bytes = decimal_to_fixed_length_bytes(mantissa, 25); + let bytes = decimal_to_fixed_length_bytes_exact(mantissa, 11).unwrap(); + let type_length = bytes.len() as i32; let mut sbbf = Sbbf::new_with_ndv_fpp(10, 0.01).unwrap(); sbbf.insert(&ByteArray::from(bytes)); - let bloom_filters = HashMap::from([(1, (sbbf, PhysicalType::FIXED_LEN_BYTE_ARRAY))]); + let bloom_filters = HashMap::from([( + 1, + ColumnBloomFilter::new(sbbf, PhysicalType::FIXED_LEN_BYTE_ARRAY, type_length), + )]); let predicate = Reference::new("amount") .equal_to( @@ -973,12 +1032,16 @@ mod tests { // Simulate a file where decimal(5,2) was stored as FIXED_LEN_BYTE_ARRAY let mantissa: i128 = 12345; - let bytes = decimal_to_fixed_length_bytes(mantissa, 5); + let bytes = decimal_to_fixed_length_bytes_exact(mantissa, 3).unwrap(); + let type_length = bytes.len() as i32; let mut sbbf = Sbbf::new_with_ndv_fpp(10, 0.01).unwrap(); sbbf.insert(&ByteArray::from(bytes)); - let bloom_filters = HashMap::from([(1, (sbbf, PhysicalType::FIXED_LEN_BYTE_ARRAY))]); + let bloom_filters = HashMap::from([( + 1, + ColumnBloomFilter::new(sbbf, PhysicalType::FIXED_LEN_BYTE_ARRAY, type_length), + )]); let predicate = Reference::new("amount") .equal_to( @@ -998,6 +1061,109 @@ mod tests { ); } + /// Builds the (schema, bloom filter) pair for a `decimal` column that the file + /// stored at `file_type_length` bytes while the table schema now declares + /// `schema_precision`, as a precision-widening evolution produces. + fn widened_decimal_case( + mantissa: i128, + file_type_length: usize, + schema_precision: u32, + ) -> (Schema, HashMap) { + let bytes = decimal_to_fixed_length_bytes_exact(mantissa, file_type_length).unwrap(); + let mut sbbf = Sbbf::new_with_ndv_fpp(10, 0.01).unwrap(); + sbbf.insert(&ByteArray::from(bytes)); + + let filters = HashMap::from([( + 1, + ColumnBloomFilter::new( + sbbf, + PhysicalType::FIXED_LEN_BYTE_ARRAY, + file_type_length as i32, + ), + )]); + + (create_decimal_schema(schema_precision, 2), filters) + } + + fn decimal_eq_predicate(schema: Schema, mantissa: i128, precision: u32) -> BoundPredicate { + Reference::new("amount") + .equal_to( + Datum::decimal_with_precision( + crate::spec::decimal_utils::decimal_from_i128_with_scale(mantissa, 2), + precision, + ) + .unwrap(), + ) + .bind(schema.into(), true) + .unwrap() + } + + /// Widening a decimal's precision does not rewrite existing files, so the + /// column keeps its original `type_length`. Deriving the probe length from the + /// widened precision changed the encoding and missed every entry the writer + /// inserted, silently pruning row groups holding matching rows. + #[test] + fn test_decimal_widened_precision_present_is_not_pruned() { + let mantissa: i128 = 12345678901234567890; + // File written as decimal(20,2) -> 9 bytes; schema since widened to 38. + let (schema, filters) = widened_decimal_case(mantissa, 9, 38); + let predicate = decimal_eq_predicate(schema, mantissa, 38); + + let result = BloomFilterEvaluator::eval(&predicate, &filters).unwrap(); + assert!( + result, + "widened decimal precision must still find the value at the file's length" + ); + } + + #[test] + fn test_decimal_widened_precision_absent_still_prunes() { + let mantissa: i128 = 12345678901234567890; + let (schema, filters) = widened_decimal_case(mantissa, 9, 38); + let predicate = decimal_eq_predicate(schema, 999999999999999999, 38); + + let result = BloomFilterEvaluator::eval(&predicate, &filters).unwrap(); + assert!(!result, "genuinely absent value must still prune"); + } + + /// A value too wide for the column's `type_length` cannot be present, but + /// truncating it would probe an unrelated slot, so stay conservative. + #[test] + fn test_decimal_value_wider_than_column_does_not_prune() { + let (schema, filters) = widened_decimal_case(1234, 2, 38); + let predicate = decimal_eq_predicate(schema, i64::MAX as i128, 38); + + let result = BloomFilterEvaluator::eval(&predicate, &filters).unwrap(); + assert!(result, "value too wide for the column must not prune"); + } + + /// A zero or oversized `type_length` is unusable; never prune on it. + #[test] + fn test_decimal_unusable_type_length_does_not_prune() { + let mantissa: i128 = 1234; + let bytes = decimal_to_fixed_length_bytes_exact(mantissa, 4).unwrap(); + let mut sbbf = Sbbf::new_with_ndv_fpp(10, 0.01).unwrap(); + sbbf.insert(&ByteArray::from(bytes)); + + for bogus_length in [0, -1, 17] { + let filters = HashMap::from([( + 1, + ColumnBloomFilter::new( + Sbbf::new_with_ndv_fpp(10, 0.01).unwrap(), + PhysicalType::FIXED_LEN_BYTE_ARRAY, + bogus_length, + ), + )]); + let predicate = decimal_eq_predicate(create_decimal_schema(38, 2), mantissa, 38); + + let result = BloomFilterEvaluator::eval(&predicate, &filters).unwrap(); + assert!( + result, + "type_length {bogus_length} is unusable and must not prune" + ); + } + } + fn single_field_schema(name: &str, ty: PrimitiveType) -> Schema { Schema::builder() .with_schema_id(1) @@ -1017,9 +1183,10 @@ mod tests { let schema = single_field_schema("id", PrimitiveType::Long); let bloom_filters = HashMap::from([( 1, - ( + ColumnBloomFilter::new( create_bloom_filter_with_values_i32(&[100, 150, 199]), PhysicalType::INT32, + 0, ), )]); @@ -1040,9 +1207,10 @@ mod tests { let schema = single_field_schema("id", PrimitiveType::Long); let bloom_filters = HashMap::from([( 1, - ( + ColumnBloomFilter::new( create_bloom_filter_with_values_i32(&[100, 150, 199]), PhysicalType::INT32, + 0, ), )]); @@ -1062,9 +1230,10 @@ mod tests { let schema = single_field_schema("id", PrimitiveType::Long); let bloom_filters = HashMap::from([( 1, - ( + ColumnBloomFilter::new( create_bloom_filter_with_values_i32(&[1, 2, 3]), PhysicalType::INT32, + 0, ), )]); @@ -1091,9 +1260,10 @@ mod tests { let schema = single_field_schema("val", PrimitiveType::Double); let bloom_filters = HashMap::from([( 1, - ( + ColumnBloomFilter::new( create_bloom_filter_with_values_f32(&[1.5, 2.25, 4.0]), PhysicalType::FLOAT, + 0, ), )]); @@ -1114,9 +1284,10 @@ mod tests { let schema = single_field_schema("val", PrimitiveType::Double); let bloom_filters = HashMap::from([( 1, - ( + ColumnBloomFilter::new( create_bloom_filter_with_values_f32(&[1.5, 2.25, 4.0]), PhysicalType::FLOAT, + 0, ), )]); @@ -1136,9 +1307,10 @@ mod tests { let schema = single_field_schema("val", PrimitiveType::Double); let bloom_filters = HashMap::from([( 1, - ( + ColumnBloomFilter::new( create_bloom_filter_with_values_f32(&[1.5, 2.25]), PhysicalType::FLOAT, + 0, ), )]); diff --git a/crates/iceberg/src/spec/values/decimal_utils.rs b/crates/iceberg/src/spec/values/decimal_utils.rs index ae962cee05..92c94e7350 100644 --- a/crates/iceberg/src/spec/values/decimal_utils.rs +++ b/crates/iceberg/src/spec/values/decimal_utils.rs @@ -196,38 +196,31 @@ pub fn i128_to_be_bytes_min(value: i128) -> Vec { bytes[start..].to_vec() } -/// Encode an i128 decimal value as a fixed-length big-endian byte array, -/// matching how Parquet stores `FIXED_LEN_BYTE_ARRAY` decimals. +/// Encode an i128 as exactly `len` big-endian two's complement bytes, matching a +/// Parquet `FIXED_LEN_BYTE_ARRAY` column of that declared `type_length`. /// -/// The result is sign-extended or trimmed to exactly the number of bytes -/// required for the given precision, matching the Java implementation in -/// `DecimalUtil.toReusedFixLengthBytes`. -pub fn decimal_to_fixed_length_bytes(value: i128, precision: u32) -> Vec { - let required_len = parquet_decimal_byte_length(precision); - let be_bytes = value.to_be_bytes(); // 16 bytes, big-endian, two's complement - - if required_len >= 16 { - // Sign-extend to the required length - let fill_byte = if value < 0 { 0xFF } else { 0x00 }; - let mut buf = vec![fill_byte; required_len]; - let offset = required_len - 16; - buf[offset..].copy_from_slice(&be_bytes); - buf - } else { - // Trim leading bytes (value fits in fewer bytes) - let offset = 16 - required_len; - be_bytes[offset..].to_vec() +/// Returns `None` if `value` does not fit in `len` bytes, since a truncated +/// encoding would represent a different number. +pub fn decimal_to_fixed_length_bytes_exact(value: i128, len: usize) -> Option> { + if len == 0 || len > 16 { + return None; } -} -/// Returns the number of bytes required to store a decimal with the given -/// precision as a Parquet `FIXED_LEN_BYTE_ARRAY`. -/// -/// Mirrors `parquet::arrow::schema::decimal_length_from_precision` which is -/// not publicly accessible outside the parquet crate without the `experimental` -/// feature flag. -fn parquet_decimal_byte_length(precision: u32) -> usize { - (((10.0_f64.powi(precision as i32) + 1.0).log2() + 1.0) / 8.0).ceil() as usize + let be_bytes = value.to_be_bytes(); + let offset = 16 - len; + let sign_byte = if value < 0 { 0xFF } else { 0x00 }; + + // The bytes being dropped must be pure sign extension. + if be_bytes[..offset].iter().any(|&b| b != sign_byte) { + return None; + } + + // The retained leading byte must carry the value's sign. + if (be_bytes[offset] & 0x80 != 0) != (value < 0) { + return None; + } + + Some(be_bytes[offset..].to_vec()) } #[cfg(test)] @@ -392,59 +385,75 @@ mod tests { } #[test] - fn test_parquet_decimal_byte_length() { - // INT32 range (precision 1-9) should need <= 4 bytes - assert!(parquet_decimal_byte_length(1) <= 4); - assert!(parquet_decimal_byte_length(9) <= 4); - // INT64 range (precision 10-18) should need <= 8 bytes - assert!(parquet_decimal_byte_length(10) <= 8); - assert!(parquet_decimal_byte_length(18) <= 8); - // FIXED_LEN_BYTE_ARRAY range (precision 19+) - assert_eq!(parquet_decimal_byte_length(19), 9); - assert_eq!(parquet_decimal_byte_length(38), 16); + fn test_decimal_to_fixed_length_bytes_exact_positive() { + let bytes = decimal_to_fixed_length_bytes_exact(12345, 9).unwrap(); + assert_eq!(bytes.len(), 9); + // Big-endian, zero-padded on the left: 12345 = 0x3039 + assert_eq!(bytes[7], 0x30); + assert_eq!(bytes[8], 0x39); + assert!(bytes[..7].iter().all(|&b| b == 0x00)); } #[test] - fn test_decimal_to_parquet_fixed_bytes_positive() { - // 12345 with precision 20 (requires 9 bytes) - let bytes = decimal_to_fixed_length_bytes(12345, 20); - assert_eq!(bytes.len(), parquet_decimal_byte_length(20)); - // Should be big-endian, zero-padded on the left - assert_eq!(bytes[bytes.len() - 2], 0x30); // 12345 = 0x3039 - assert_eq!(bytes[bytes.len() - 1], 0x39); - // Leading bytes should be 0x00 (positive) - assert!(bytes[..bytes.len() - 2].iter().all(|&b| b == 0x00)); - } - - #[test] - fn test_decimal_to_parquet_fixed_bytes_negative() { - // -1 with precision 20 - let bytes = decimal_to_fixed_length_bytes(-1, 20); - assert_eq!(bytes.len(), parquet_decimal_byte_length(20)); - // All bytes should be 0xFF (-1 in two's complement) + fn test_decimal_to_fixed_length_bytes_exact_negative() { + let bytes = decimal_to_fixed_length_bytes_exact(-1, 9).unwrap(); + assert_eq!(bytes.len(), 9); assert!(bytes.iter().all(|&b| b == 0xFF)); } #[test] - fn test_decimal_to_parquet_fixed_bytes_round_trip() { - // Verify that encoding then decoding via i128_from_be_bytes gives back - // the original value - for (value, precision) in [ - (0i128, 20), - (1, 20), - (-1, 20), - (12345, 20), - (-12345, 20), - (i64::MAX as i128, 20), - (i64::MIN as i128, 20), + fn test_decimal_to_fixed_length_bytes_exact_round_trip() { + for value in [ + 0i128, + 1, + -1, + 12345, + -12345, + i64::MAX as i128, + i64::MIN as i128, ] { - let bytes = decimal_to_fixed_length_bytes(value, precision); - let decoded = i128_from_be_bytes(&bytes); + let bytes = decimal_to_fixed_length_bytes_exact(value, 16).unwrap(); + assert_eq!(bytes.len(), 16); assert_eq!( - decoded, + i128_from_be_bytes(&bytes), Some(value), - "Round trip failed for value={value}, precision={precision}" + "Round trip failed for value={value}" ); } } + + /// The length must be honoured exactly: a value needing more bytes cannot be + /// truncated into the column's width, because the truncation would encode a + /// different number and probe the wrong bloom filter slot. + #[test] + fn test_decimal_to_fixed_length_bytes_exact_rejects_overflow() { + // 200 needs a leading zero byte to stay positive in two's complement. + assert_eq!(decimal_to_fixed_length_bytes_exact(200, 1), None); + assert_eq!( + decimal_to_fixed_length_bytes_exact(200, 2), + Some(vec![0x00, 0xC8]) + ); + + assert_eq!(decimal_to_fixed_length_bytes_exact(-200, 1), None); + assert_eq!( + decimal_to_fixed_length_bytes_exact(-200, 2), + Some(vec![0xFF, 0x38]) + ); + + // Exactly representable boundaries. + assert_eq!( + decimal_to_fixed_length_bytes_exact(127, 1), + Some(vec![0x7F]) + ); + assert_eq!(decimal_to_fixed_length_bytes_exact(128, 1), None); + assert_eq!( + decimal_to_fixed_length_bytes_exact(-128, 1), + Some(vec![0x80]) + ); + assert_eq!(decimal_to_fixed_length_bytes_exact(-129, 1), None); + + assert_eq!(decimal_to_fixed_length_bytes_exact(i128::MAX, 15), None); + assert_eq!(decimal_to_fixed_length_bytes_exact(0, 0), None); + assert_eq!(decimal_to_fixed_length_bytes_exact(0, 17), None); + } } From 1c4a8c1a5bd3b022606e92a8e4168b84de9c5e07 Mon Sep 17 00:00:00 2001 From: Xander Date: Thu, 10 Sep 2026 08:51:30 +0100 Subject: [PATCH 09/25] pub crate --- crates/iceberg/public-api.txt | 2 ++ crates/iceberg/src/spec/values/decimal_utils.rs | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/crates/iceberg/public-api.txt b/crates/iceberg/public-api.txt index ca55804966..6992fa993e 100644 --- a/crates/iceberg/public-api.txt +++ b/crates/iceberg/public-api.txt @@ -72,6 +72,7 @@ impl iceberg::arrow::ArrowReaderBuilder pub fn iceberg::arrow::ArrowReaderBuilder::build(self) -> iceberg::arrow::ArrowReader pub fn iceberg::arrow::ArrowReaderBuilder::new(file_io: iceberg::io::FileIO, runtime: iceberg::Runtime) -> Self pub fn iceberg::arrow::ArrowReaderBuilder::with_batch_size(self, batch_size: usize) -> Self +pub fn iceberg::arrow::ArrowReaderBuilder::with_bloom_filter_enabled(self, bloom_filter_enabled: bool) -> Self pub fn iceberg::arrow::ArrowReaderBuilder::with_data_file_concurrency_limit(self, val: usize) -> Self pub fn iceberg::arrow::ArrowReaderBuilder::with_metadata_size_hint(self, metadata_size_hint: usize) -> Self pub fn iceberg::arrow::ArrowReaderBuilder::with_range_coalesce_bytes(self, range_coalesce_bytes: u64) -> Self @@ -1361,6 +1362,7 @@ pub fn iceberg::scan::TableScanBuilder<'a>::select_all(self) -> Self pub fn iceberg::scan::TableScanBuilder<'a>::select_empty(self) -> Self pub fn iceberg::scan::TableScanBuilder<'a>::snapshot_id(self, snapshot_id: i64) -> Self pub fn iceberg::scan::TableScanBuilder<'a>::with_batch_size(self, batch_size: core::option::Option) -> Self +pub fn iceberg::scan::TableScanBuilder<'a>::with_bloom_filter_enabled(self, bloom_filter_enabled: bool) -> Self pub fn iceberg::scan::TableScanBuilder<'a>::with_case_sensitive(self, case_sensitive: bool) -> Self pub fn iceberg::scan::TableScanBuilder<'a>::with_concurrency_limit(self, limit: usize) -> Self pub fn iceberg::scan::TableScanBuilder<'a>::with_data_file_concurrency_limit(self, limit: usize) -> Self diff --git a/crates/iceberg/src/spec/values/decimal_utils.rs b/crates/iceberg/src/spec/values/decimal_utils.rs index 92c94e7350..3d09d6f25e 100644 --- a/crates/iceberg/src/spec/values/decimal_utils.rs +++ b/crates/iceberg/src/spec/values/decimal_utils.rs @@ -201,7 +201,7 @@ pub fn i128_to_be_bytes_min(value: i128) -> Vec { /// /// Returns `None` if `value` does not fit in `len` bytes, since a truncated /// encoding would represent a different number. -pub fn decimal_to_fixed_length_bytes_exact(value: i128, len: usize) -> Option> { +pub(crate) fn decimal_to_fixed_length_bytes_exact(value: i128, len: usize) -> Option> { if len == 0 || len > 16 { return None; } From 5e4821382ef88ded1f224a91d8abc649f3b1e9a1 Mon Sep 17 00:00:00 2001 From: Xander Date: Thu, 10 Sep 2026 08:58:02 +0100 Subject: [PATCH 10/25] full import --- crates/iceberg/src/arrow/reader/pipeline.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/crates/iceberg/src/arrow/reader/pipeline.rs b/crates/iceberg/src/arrow/reader/pipeline.rs index 7047b59425..02123a7e1b 100644 --- a/crates/iceberg/src/arrow/reader/pipeline.rs +++ b/crates/iceberg/src/arrow/reader/pipeline.rs @@ -44,6 +44,7 @@ use crate::arrow::record_batch_transformer::RecordBatchTransformerBuilder; use crate::arrow::scan_metrics::{CountingFileRead, ScanMetrics, ScanResult}; use crate::encryption::StandardKeyMetadata; use crate::error::Result; +use crate::expr::BoundPredicate; use crate::expr::visitors::bloom_filter_evaluator::{ BloomFilterEvaluator, ColumnBloomFilter, collect_bloom_filter_field_ids, }; @@ -725,7 +726,7 @@ impl FileScanTaskReader { /// Reads bloom filters for relevant columns and evaluates the predicate /// against them to filter out row groups that definitely don't match. async fn filter_row_groups_by_bloom_filter( - predicate: &crate::expr::BoundPredicate, + predicate: &BoundPredicate, builder: &mut ParquetRecordBatchStreamBuilder, candidate_row_groups: &[usize], field_id_map: &HashMap, From 310b37447eabd4a016dd8abc61817192257476cd Mon Sep 17 00:00:00 2001 From: Xander Date: Thu, 10 Sep 2026 08:59:39 +0100 Subject: [PATCH 11/25] lifetime --- crates/iceberg/src/expr/visitors/bloom_filter_evaluator.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/iceberg/src/expr/visitors/bloom_filter_evaluator.rs b/crates/iceberg/src/expr/visitors/bloom_filter_evaluator.rs index 562b3c6405..4a78beaf9d 100644 --- a/crates/iceberg/src/expr/visitors/bloom_filter_evaluator.rs +++ b/crates/iceberg/src/expr/visitors/bloom_filter_evaluator.rs @@ -60,7 +60,7 @@ pub(crate) struct BloomFilterEvaluator<'a> { bloom_filters: &'a HashMap, } -impl<'a> BloomFilterEvaluator<'a> { +impl BloomFilterEvaluator<'_> { /// Evaluate the predicate against the provided bloom filters. /// Returns `false` if the row group definitely does not match, /// `true` if it might match. @@ -294,7 +294,7 @@ fn check_in_bloom_filter(column: &ColumnBloomFilter, datum: &Datum) -> bool { } } -impl<'a> BoundPredicateVisitor for BloomFilterEvaluator<'a> { +impl BoundPredicateVisitor for BloomFilterEvaluator<'_> { type T = bool; fn always_true(&mut self) -> Result { From 32302b7b8698a090d7074c6a4cd801ad5a8b0b09 Mon Sep 17 00:00:00 2001 From: Xander Date: Thu, 10 Sep 2026 09:13:15 +0100 Subject: [PATCH 12/25] log --- crates/iceberg/src/arrow/reader/pipeline.rs | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/crates/iceberg/src/arrow/reader/pipeline.rs b/crates/iceberg/src/arrow/reader/pipeline.rs index 02123a7e1b..7ec9bf4d55 100644 --- a/crates/iceberg/src/arrow/reader/pipeline.rs +++ b/crates/iceberg/src/arrow/reader/pipeline.rs @@ -770,8 +770,12 @@ impl FileScanTaskReader { ); } Ok(None) => {} - Err(_) => { - // If we can't read the bloom filter, conservatively include the row group + Err(e) => { + // Left absent from the map, so the evaluator treats the column + // as might-match and the row group survives. + tracing::debug!( + "Bloom filter for field {field_id} in row group {rg_idx} could not be read: {e}" + ); } } } @@ -779,7 +783,12 @@ impl FileScanTaskReader { match BloomFilterEvaluator::eval(predicate, &bloom_filters) { Ok(true) => result.push(rg_idx), Ok(false) => { /* Row group pruned by bloom filter */ } - Err(_) => result.push(rg_idx), // On error, conservatively include + Err(e) => { + tracing::debug!( + "Bloom filter evaluation failed for row group {rg_idx}, including it: {e}" + ); + result.push(rg_idx); + } } } From eed3e03e9c5e511c7b604f62cf0651543f677f3c Mon Sep 17 00:00:00 2001 From: Xander Date: Thu, 10 Sep 2026 09:18:27 +0100 Subject: [PATCH 13/25] add not supported comment --- .../iceberg/src/expr/visitors/bloom_filter_evaluator.rs | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/crates/iceberg/src/expr/visitors/bloom_filter_evaluator.rs b/crates/iceberg/src/expr/visitors/bloom_filter_evaluator.rs index 4a78beaf9d..3d569e8092 100644 --- a/crates/iceberg/src/expr/visitors/bloom_filter_evaluator.rs +++ b/crates/iceberg/src/expr/visitors/bloom_filter_evaluator.rs @@ -282,7 +282,13 @@ fn check_in_bloom_filter(column: &ColumnBloomFilter, datum: &Datum) -> bool { None => true, } } - _ => true, // Unexpected physical type — conservatively might match + // Known gap: BYTE_ARRAY decimals are valid in Parquet (though not in + // Iceberg's Appendix A) and some older Spark writers emit them. Not + // probed because BYTE_ARRAY carries no `type_length` and sign-extension + // padding is writer-dependent, so a single-length probe would miss + // padded entries and prune row groups that do hold the value. A sound + // version ORs a check over every length from minimal..=16. + _ => true, // Conservatively might match } } PrimitiveLiteral::UInt128(v) => { From 6c67826a11c6032709555b7d769fb36e82156b90 Mon Sep 17 00:00:00 2001 From: Xander Date: Thu, 10 Sep 2026 09:22:44 +0100 Subject: [PATCH 14/25] make it more safe --- .../expr/visitors/bloom_filter_evaluator.rs | 58 ++++++++++++++++++- 1 file changed, 55 insertions(+), 3 deletions(-) diff --git a/crates/iceberg/src/expr/visitors/bloom_filter_evaluator.rs b/crates/iceberg/src/expr/visitors/bloom_filter_evaluator.rs index 3d569e8092..84b69e50e5 100644 --- a/crates/iceberg/src/expr/visitors/bloom_filter_evaluator.rs +++ b/crates/iceberg/src/expr/visitors/bloom_filter_evaluator.rs @@ -237,7 +237,8 @@ fn check_in_bloom_filter(column: &ColumnBloomFilter, datum: &Datum) -> bool { PhysicalType::INT64 => sbbf.check(v), PhysicalType::INT32 => match i32::try_from(*v) { Ok(narrowed) => sbbf.check(&narrowed), - // Out of range for the column, so it cannot be present. + // Too wide for an INT32 column to hold, so there is nothing + // meaningful to probe — keep the row group. Err(_) => true, }, _ => true, @@ -266,8 +267,16 @@ fn check_in_bloom_filter(column: &ColumnBloomFilter, datum: &Datum) -> bool { // Decimal: dispatch based on the actual Parquet physical type // from the file, not inferred from precision. match physical_type { - PhysicalType::INT32 => sbbf.check(&(*v as i32)), - PhysicalType::INT64 => sbbf.check(&(*v as i64)), + // Narrow only when the mantissa round-trips; a truncated copy would + // hash to an unrelated slot. + PhysicalType::INT32 => match i32::try_from(*v) { + Ok(narrowed) => sbbf.check(&narrowed), + Err(_) => true, + }, + PhysicalType::INT64 => match i64::try_from(*v) { + Ok(narrowed) => sbbf.check(&narrowed), + Err(_) => true, + }, PhysicalType::FIXED_LEN_BYTE_ARRAY => { // Encode to the file's declared length, not one derived from // the Iceberg precision: a widened precision would change the @@ -1143,6 +1152,49 @@ mod tests { assert!(result, "value too wide for the column must not prune"); } + /// The `INT32` / `INT64` counterparts of the case above: after a precision + /// widening the bound literal can carry a mantissa wider than the file's + /// physical column, and a truncated probe would hash an unrelated slot. + #[test] + fn test_decimal_wider_than_int32_column_does_not_prune() { + let mut sbbf = Sbbf::new_with_ndv_fpp(10, 0.01).unwrap(); + sbbf.insert(&12345_i32); + let filters = HashMap::from([(1, ColumnBloomFilter::new(sbbf, PhysicalType::INT32, 0))]); + + // File written as decimal(9,2) -> INT32; schema since widened to decimal(38,2). + let predicate = decimal_eq_predicate(create_decimal_schema(38, 2), i64::MAX as i128, 38); + + let result = BloomFilterEvaluator::eval(&predicate, &filters).unwrap(); + assert!(result, "mantissa too wide for an INT32 column must not prune"); + } + + #[test] + fn test_decimal_wider_than_int64_column_does_not_prune() { + let mut sbbf = Sbbf::new_with_ndv_fpp(10, 0.01).unwrap(); + sbbf.insert(&12345_i64); + let filters = HashMap::from([(1, ColumnBloomFilter::new(sbbf, PhysicalType::INT64, 0))]); + + let predicate = + decimal_eq_predicate(create_decimal_schema(38, 2), i128::from(i64::MAX) + 1, 38); + + let result = BloomFilterEvaluator::eval(&predicate, &filters).unwrap(); + assert!(result, "mantissa too wide for an INT64 column must not prune"); + } + + /// A mantissa that does fit must still prune when genuinely absent, so the + /// `try_from` guard above does not disable pruning outright. + #[test] + fn test_decimal_within_int64_column_still_prunes() { + let mut sbbf = Sbbf::new_with_ndv_fpp(10, 0.01).unwrap(); + sbbf.insert(&12345_i64); + let filters = HashMap::from([(1, ColumnBloomFilter::new(sbbf, PhysicalType::INT64, 0))]); + + let predicate = decimal_eq_predicate(create_decimal_schema(38, 2), 999_999, 38); + + let result = BloomFilterEvaluator::eval(&predicate, &filters).unwrap(); + assert!(!result, "absent in-range mantissa must still prune"); + } + /// A zero or oversized `type_length` is unusable; never prune on it. #[test] fn test_decimal_unusable_type_length_does_not_prune() { From db5daf868a7647b71a68e6b0d9611f4dd0eb82f5 Mon Sep 17 00:00:00 2001 From: Xander Date: Thu, 10 Sep 2026 09:24:59 +0100 Subject: [PATCH 15/25] Add tests --- .../src/expr/visitors/bloom_filter_evaluator.rs | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/crates/iceberg/src/expr/visitors/bloom_filter_evaluator.rs b/crates/iceberg/src/expr/visitors/bloom_filter_evaluator.rs index 84b69e50e5..d997b810c2 100644 --- a/crates/iceberg/src/expr/visitors/bloom_filter_evaluator.rs +++ b/crates/iceberg/src/expr/visitors/bloom_filter_evaluator.rs @@ -269,14 +269,8 @@ fn check_in_bloom_filter(column: &ColumnBloomFilter, datum: &Datum) -> bool { match physical_type { // Narrow only when the mantissa round-trips; a truncated copy would // hash to an unrelated slot. - PhysicalType::INT32 => match i32::try_from(*v) { - Ok(narrowed) => sbbf.check(&narrowed), - Err(_) => true, - }, - PhysicalType::INT64 => match i64::try_from(*v) { - Ok(narrowed) => sbbf.check(&narrowed), - Err(_) => true, - }, + PhysicalType::INT32 => sbbf.check(&(*v as i32)), + PhysicalType::INT64 => sbbf.check(&(*v as i64)), PhysicalType::FIXED_LEN_BYTE_ARRAY => { // Encode to the file's declared length, not one derived from // the Iceberg precision: a widened precision would change the From a1c6b927d4067660c8303ad31120b96d8e50db89 Mon Sep 17 00:00:00 2001 From: Xander Date: Thu, 10 Sep 2026 09:25:55 +0100 Subject: [PATCH 16/25] fix --- .../src/expr/visitors/bloom_filter_evaluator.rs | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/crates/iceberg/src/expr/visitors/bloom_filter_evaluator.rs b/crates/iceberg/src/expr/visitors/bloom_filter_evaluator.rs index d997b810c2..84b69e50e5 100644 --- a/crates/iceberg/src/expr/visitors/bloom_filter_evaluator.rs +++ b/crates/iceberg/src/expr/visitors/bloom_filter_evaluator.rs @@ -269,8 +269,14 @@ fn check_in_bloom_filter(column: &ColumnBloomFilter, datum: &Datum) -> bool { match physical_type { // Narrow only when the mantissa round-trips; a truncated copy would // hash to an unrelated slot. - PhysicalType::INT32 => sbbf.check(&(*v as i32)), - PhysicalType::INT64 => sbbf.check(&(*v as i64)), + PhysicalType::INT32 => match i32::try_from(*v) { + Ok(narrowed) => sbbf.check(&narrowed), + Err(_) => true, + }, + PhysicalType::INT64 => match i64::try_from(*v) { + Ok(narrowed) => sbbf.check(&narrowed), + Err(_) => true, + }, PhysicalType::FIXED_LEN_BYTE_ARRAY => { // Encode to the file's declared length, not one derived from // the Iceberg precision: a widened precision would change the From 3d80060c337ee75200dad5c4aebf880eace9f4db Mon Sep 17 00:00:00 2001 From: Xander Date: Thu, 10 Sep 2026 09:26:21 +0100 Subject: [PATCH 17/25] fmt --- .../src/expr/visitors/bloom_filter_evaluator.rs | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/crates/iceberg/src/expr/visitors/bloom_filter_evaluator.rs b/crates/iceberg/src/expr/visitors/bloom_filter_evaluator.rs index 84b69e50e5..5c2a7b936b 100644 --- a/crates/iceberg/src/expr/visitors/bloom_filter_evaluator.rs +++ b/crates/iceberg/src/expr/visitors/bloom_filter_evaluator.rs @@ -1165,7 +1165,10 @@ mod tests { let predicate = decimal_eq_predicate(create_decimal_schema(38, 2), i64::MAX as i128, 38); let result = BloomFilterEvaluator::eval(&predicate, &filters).unwrap(); - assert!(result, "mantissa too wide for an INT32 column must not prune"); + assert!( + result, + "mantissa too wide for an INT32 column must not prune" + ); } #[test] @@ -1178,7 +1181,10 @@ mod tests { decimal_eq_predicate(create_decimal_schema(38, 2), i128::from(i64::MAX) + 1, 38); let result = BloomFilterEvaluator::eval(&predicate, &filters).unwrap(); - assert!(result, "mantissa too wide for an INT64 column must not prune"); + assert!( + result, + "mantissa too wide for an INT64 column must not prune" + ); } /// A mantissa that does fit must still prune when genuinely absent, so the From 8adcd728fd96968fc716b3c7eab0fd715ea57054 Mon Sep 17 00:00:00 2001 From: Xander Date: Thu, 10 Sep 2026 11:07:32 +0100 Subject: [PATCH 18/25] don't read not predicates --- .../expr/visitors/bloom_filter_evaluator.rs | 197 +++++++++++++----- 1 file changed, 147 insertions(+), 50 deletions(-) diff --git a/crates/iceberg/src/expr/visitors/bloom_filter_evaluator.rs b/crates/iceberg/src/expr/visitors/bloom_filter_evaluator.rs index 5c2a7b936b..65cfa3b2dd 100644 --- a/crates/iceberg/src/expr/visitors/bloom_filter_evaluator.rs +++ b/crates/iceberg/src/expr/visitors/bloom_filter_evaluator.rs @@ -89,59 +89,66 @@ impl BloomFilterEvaluator<'_> { /// Collects field IDs that appear in `eq` or `in` predicates — the only /// predicate types that benefit from bloom filter checks. +/// +/// Each node returns the field IDs its subtree contributes, mirroring +/// [`BloomFilterEvaluator`]'s structure so that a field is collected only where +/// the evaluator can act on it. In particular `not` discards its subtree: the +/// evaluator's `not` returns might-match regardless, so a filter fetched for a +/// field under a `NOT` could never prune and would be pure wasted I/O. pub(crate) fn collect_bloom_filter_field_ids(predicate: &BoundPredicate) -> Result> { - let mut visitor = BloomFilterFieldIdCollector { - field_ids: HashSet::new(), - }; - visit(&mut visitor, predicate)?; - Ok(visitor.field_ids) + visit(&mut BloomFilterFieldIdCollector, predicate) } -struct BloomFilterFieldIdCollector { - field_ids: HashSet, -} +struct BloomFilterFieldIdCollector; impl BoundPredicateVisitor for BloomFilterFieldIdCollector { - type T = (); + type T = HashSet; - fn always_true(&mut self) -> Result<()> { - Ok(()) + fn always_true(&mut self) -> Result { + Ok(HashSet::new()) } - fn always_false(&mut self) -> Result<()> { - Ok(()) + fn always_false(&mut self) -> Result { + Ok(HashSet::new()) } - fn and(&mut self, _lhs: (), _rhs: ()) -> Result<()> { - Ok(()) + fn and(&mut self, mut lhs: Self::T, rhs: Self::T) -> Result { + lhs.extend(rhs); + Ok(lhs) } - fn or(&mut self, _lhs: (), _rhs: ()) -> Result<()> { - Ok(()) + fn or(&mut self, mut lhs: Self::T, rhs: Self::T) -> Result { + lhs.extend(rhs); + Ok(lhs) } - fn not(&mut self, _inner: ()) -> Result<()> { - Ok(()) + fn not(&mut self, _inner: Self::T) -> Result { + Ok(HashSet::new()) } - fn is_null(&mut self, _r: &BoundReference, _p: &BoundPredicate) -> Result<()> { - Ok(()) + fn is_null(&mut self, _r: &BoundReference, _p: &BoundPredicate) -> Result { + Ok(HashSet::new()) } - fn not_null(&mut self, _r: &BoundReference, _p: &BoundPredicate) -> Result<()> { - Ok(()) + fn not_null(&mut self, _r: &BoundReference, _p: &BoundPredicate) -> Result { + Ok(HashSet::new()) } - fn is_nan(&mut self, _r: &BoundReference, _p: &BoundPredicate) -> Result<()> { - Ok(()) + fn is_nan(&mut self, _r: &BoundReference, _p: &BoundPredicate) -> Result { + Ok(HashSet::new()) } - fn not_nan(&mut self, _r: &BoundReference, _p: &BoundPredicate) -> Result<()> { - Ok(()) + fn not_nan(&mut self, _r: &BoundReference, _p: &BoundPredicate) -> Result { + Ok(HashSet::new()) } - fn less_than(&mut self, _r: &BoundReference, _l: &Datum, _p: &BoundPredicate) -> Result<()> { - Ok(()) + fn less_than( + &mut self, + _r: &BoundReference, + _l: &Datum, + _p: &BoundPredicate, + ) -> Result { + Ok(HashSet::new()) } fn less_than_or_eq( @@ -149,12 +156,17 @@ impl BoundPredicateVisitor for BloomFilterFieldIdCollector { _r: &BoundReference, _l: &Datum, _p: &BoundPredicate, - ) -> Result<()> { - Ok(()) + ) -> Result { + Ok(HashSet::new()) } - fn greater_than(&mut self, _r: &BoundReference, _l: &Datum, _p: &BoundPredicate) -> Result<()> { - Ok(()) + fn greater_than( + &mut self, + _r: &BoundReference, + _l: &Datum, + _p: &BoundPredicate, + ) -> Result { + Ok(HashSet::new()) } fn greater_than_or_eq( @@ -162,21 +174,25 @@ impl BoundPredicateVisitor for BloomFilterFieldIdCollector { _r: &BoundReference, _l: &Datum, _p: &BoundPredicate, - ) -> Result<()> { - Ok(()) + ) -> Result { + Ok(HashSet::new()) } - fn eq(&mut self, r: &BoundReference, _l: &Datum, _p: &BoundPredicate) -> Result<()> { - self.field_ids.insert(r.field().id); - Ok(()) + fn eq(&mut self, r: &BoundReference, _l: &Datum, _p: &BoundPredicate) -> Result { + Ok(HashSet::from([r.field().id])) } - fn not_eq(&mut self, _r: &BoundReference, _l: &Datum, _p: &BoundPredicate) -> Result<()> { - Ok(()) + fn not_eq(&mut self, _r: &BoundReference, _l: &Datum, _p: &BoundPredicate) -> Result { + Ok(HashSet::new()) } - fn starts_with(&mut self, _r: &BoundReference, _l: &Datum, _p: &BoundPredicate) -> Result<()> { - Ok(()) + fn starts_with( + &mut self, + _r: &BoundReference, + _l: &Datum, + _p: &BoundPredicate, + ) -> Result { + Ok(HashSet::new()) } fn not_starts_with( @@ -184,8 +200,8 @@ impl BoundPredicateVisitor for BloomFilterFieldIdCollector { _r: &BoundReference, _l: &Datum, _p: &BoundPredicate, - ) -> Result<()> { - Ok(()) + ) -> Result { + Ok(HashSet::new()) } fn r#in( @@ -193,9 +209,8 @@ impl BoundPredicateVisitor for BloomFilterFieldIdCollector { r: &BoundReference, _literals: &FnvHashSet, _p: &BoundPredicate, - ) -> Result<()> { - self.field_ids.insert(r.field().id); - Ok(()) + ) -> Result { + Ok(HashSet::from([r.field().id])) } fn not_in( @@ -203,8 +218,8 @@ impl BoundPredicateVisitor for BloomFilterFieldIdCollector { _r: &BoundReference, _literals: &FnvHashSet, _p: &BoundPredicate, - ) -> Result<()> { - Ok(()) + ) -> Result { + Ok(HashSet::new()) } } @@ -483,7 +498,7 @@ mod tests { use parquet::bloom_filter::Sbbf; use parquet::data_type::ByteArray; - use super::{BloomFilterEvaluator, ColumnBloomFilter}; + use super::{BloomFilterEvaluator, ColumnBloomFilter, collect_bloom_filter_field_ids}; use crate::expr::{Bind, BoundPredicate, Reference}; use crate::spec::decimal_utils::decimal_to_fixed_length_bytes_exact; use crate::spec::{Datum, NestedField, PrimitiveType, Schema, Type}; @@ -709,6 +724,88 @@ mod tests { assert!(result, "NOT should always return might-match"); } + // --- Field ID collection --- + // + // The collector decides which bloom filters get fetched, one round trip per + // column per row group, so anything it reports that the evaluator cannot act + // on is wasted I/O. + + fn collected_ids(predicate: BoundPredicate) -> Vec { + let mut ids: Vec = collect_bloom_filter_field_ids(&predicate) + .unwrap() + .into_iter() + .collect(); + ids.sort_unstable(); + ids + } + + #[test] + fn test_collects_eq_and_in_field_ids() { + let schema = create_test_schema(); + let predicate = Reference::new("id") + .equal_to(Datum::int(1)) + .and(Reference::new("name").is_in([Datum::string("alice")])) + .bind(schema.into(), true) + .unwrap(); + + assert_eq!(collected_ids(predicate), vec![1, 2]); + } + + /// `not` discards its subtree, so `eq`/`in` beneath a `NOT` are not collected. + /// The evaluator's `not` returns might-match regardless, so fetching those + /// filters could never prune. + #[test] + fn test_does_not_collect_eq_under_not() { + let schema = create_test_schema(); + let predicate = Reference::new("id") + .equal_to(Datum::int(1)) + .not() + .bind(schema.into(), true) + .unwrap(); + + assert!(collected_ids(predicate).is_empty()); + } + + #[test] + fn test_does_not_collect_in_under_not() { + let schema = create_test_schema(); + let predicate = Reference::new("id") + .is_in([Datum::int(1), Datum::int(2)]) + .not() + .bind(schema.into(), true) + .unwrap(); + + assert!(collected_ids(predicate).is_empty()); + } + + /// A `NOT` must not suppress collection for its siblings. + #[test] + fn test_collects_sibling_of_not() { + let schema = create_test_schema(); + let predicate = Reference::new("id") + .equal_to(Datum::int(1)) + .not() + .and(Reference::new("name").equal_to(Datum::string("alice"))) + .bind(schema.into(), true) + .unwrap(); + + assert_eq!(collected_ids(predicate), vec![2]); + } + + /// Range and `not_eq` predicates cannot be probed, so they contribute nothing. + #[test] + fn test_does_not_collect_unprobeable_operators() { + let schema = create_test_schema(); + let predicate = Reference::new("id") + .less_than(Datum::int(1)) + .and(Reference::new("id").not_equal_to(Datum::int(2))) + .and(Reference::new("name").is_not_null()) + .bind(schema.into(), true) + .unwrap(); + + assert!(collected_ids(predicate).is_empty()); + } + #[test] fn test_range_predicates_always_might_match() { let schema = create_test_schema(); From cfbf233fd8dadad447db1af16fb33be552c57a92 Mon Sep 17 00:00:00 2001 From: Xander Date: Thu, 10 Sep 2026 12:21:27 +0100 Subject: [PATCH 19/25] tests --- crates/iceberg/src/arrow/reader/row_filter.rs | 540 ++++++++++++++---- 1 file changed, 414 insertions(+), 126 deletions(-) diff --git a/crates/iceberg/src/arrow/reader/row_filter.rs b/crates/iceberg/src/arrow/reader/row_filter.rs index faa5038490..14eb712f46 100644 --- a/crates/iceberg/src/arrow/reader/row_filter.rs +++ b/crates/iceberg/src/arrow/reader/row_filter.rs @@ -217,7 +217,8 @@ mod tests { use arrow_array::cast::AsArray; use arrow_array::{ - ArrayRef, Int32Array, Int64Array, LargeStringArray, RecordBatch, StringArray, + ArrayRef, Decimal128Array, Float32Array, Int32Array, Int64Array, LargeStringArray, + RecordBatch, StringArray, }; use arrow_schema::{DataType, Field, Schema as ArrowSchema}; use futures::TryStreamExt; @@ -1281,174 +1282,461 @@ mod tests { ); } - /// Tests that bloom filter pushdown correctly prunes row groups. - #[tokio::test] - async fn test_bloom_filter_pushdown_prunes_row_groups() { - let schema = Arc::new( - Schema::builder() - .with_schema_id(1) - .with_fields(vec![ - NestedField::required(1, "id", Type::Primitive(PrimitiveType::Int)).into(), - ]) - .build() - .unwrap(), - ); - - let arrow_schema = Arc::new(ArrowSchema::new(vec![ - Field::new("id", DataType::Int32, false).with_metadata(HashMap::from([( - PARQUET_FIELD_ID_META_KEY.to_string(), - "1".to_string(), - )])), - ])); - - let tmp_dir = TempDir::new().unwrap(); - let file_path = format!("{}/bloom_test.parquet", tmp_dir.path().to_str().unwrap()); + // Bloom filter pushdown: on-vs-off equivalence + // Pushdown must never change results. An encoding bug in the probe shows up as + // rows the bloom filter drops and the row filter keeps, so every case below + // reads the same file twice and compares. Fixtures spread each row group's + // values across the whole domain, so min/max statistics cannot prune and any + // reduction in bytes read is attributable to the bloom filter. + + const ROWS_PER_GROUP: i32 = 20_000; + const GROUPS: i32 = 3; + /// Gap between consecutive values in a row group, so a value can be absent from + /// the file yet still sit inside every row group's min/max range. + const STRIDE: i32 = 30; + + /// The `i`th value of row group `group`, spread across the whole domain. + fn interleaved(group: i32, i: i32) -> i32 { + i * STRIDE + group + } - // Write a Parquet file with 3 row groups, each containing distinct values, - // with bloom filters enabled. - // Row group 0: ids 0..100 - // Row group 1: ids 100..200 - // Row group 2: ids 200..300 - let props = WriterProperties::builder() - .set_compression(Compression::SNAPPY) - .set_max_row_group_row_count(Some(100)) - .set_bloom_filter_enabled(true) - .build(); + fn field_with_id(name: &str, ty: DataType, id: i32) -> Field { + Field::new(name, ty, false).with_metadata(HashMap::from([( + PARQUET_FIELD_ID_META_KEY.to_string(), + id.to_string(), + )])) + } - let file = File::create(&file_path).unwrap(); - let mut writer = ArrowWriter::try_new(file, arrow_schema.clone(), Some(props)).unwrap(); + /// Writes one row group per batch, optionally with bloom filters. + fn write_row_groups( + path: &str, + arrow_schema: Arc, + row_groups: Vec, + with_bloom_filters: bool, + ) { + let mut props = WriterProperties::builder().set_compression(Compression::UNCOMPRESSED); + if with_bloom_filters { + props = props + .set_bloom_filter_enabled(true) + .set_bloom_filter_max_ndv(ROWS_PER_GROUP as u64); + } - for batch_start in [0, 100, 200] { - let batch = RecordBatch::try_new(arrow_schema.clone(), vec![Arc::new( - Int32Array::from((batch_start..batch_start + 100).collect::>()), - )]) - .unwrap(); + let file = File::create(path).unwrap(); + let mut writer = ArrowWriter::try_new(file, arrow_schema, Some(props.build())).unwrap(); + for batch in row_groups { writer.write(&batch).unwrap(); + // Force a row group boundary so each batch is independently prunable. + writer.flush().unwrap(); } writer.close().unwrap(); + } + async fn read_once( + file_path: &str, + schema: Arc, + project_field_ids: Vec, + predicate: Predicate, + bloom_enabled: bool, + ) -> (Vec, u64) { let file_io = FileIO::new_with_fs(); - - // Query for id = 150, which is only in row group 1. - // With bloom filter pushdown, row groups 0 and 2 should be pruned. - let predicate = Reference::new("id").equal_to(Datum::int(150)); - - let reader = ArrowReaderBuilder::new(file_io.clone(), Runtime::current()) - .with_bloom_filter_enabled(true) + let reader = ArrowReaderBuilder::new(file_io, Runtime::current()) + .with_bloom_filter_enabled(bloom_enabled) + // Keep the fixed footer prefetch small so the byte measurement reflects + // row group I/O rather than a constant metadata read. + .with_metadata_size_hint(8 * 1024) .build(); let task = FileScanTask::builder() - .with_file_size_in_bytes(std::fs::metadata(&file_path).unwrap().len()) + .with_file_size_in_bytes(std::fs::metadata(file_path).unwrap().len()) .with_start(0) .with_length(0) - .with_data_file_path(file_path.clone()) + .with_data_file_path(file_path.to_string()) .with_data_file_format(DataFileFormat::Parquet) .with_schema(schema.clone()) - .with_project_field_ids(vec![1]) - .with_predicate(Some(predicate.bind(schema.clone(), true).unwrap())) + .with_project_field_ids(project_field_ids) + .with_predicate(Some(predicate.bind(schema, true).unwrap())) .with_case_sensitive(false) .build() .unwrap(); let tasks = Box::pin(futures::stream::iter(vec![Ok(task)])) as FileScanTaskStream; + let result = reader.read(tasks).unwrap(); + let metrics = result.metrics().clone(); + let batches: Vec = result.stream().try_collect().await.unwrap(); - let result = reader - .read(tasks) - .unwrap() - .stream() - .try_collect::>() - .await - .unwrap(); + (batches, metrics.bytes_read()) + } + + /// Batch boundaries differ when fewer row groups are read, so collapse to a + /// single batch before comparing. `None` means no rows at all. + fn collapse(batches: &[RecordBatch]) -> Option { + let first = batches.first()?; + Some(arrow_select::concat::concat_batches(&first.schema(), batches).unwrap()) + } - // Only row group 1 (ids 100..200) should be read. The row filter - // then further filters to just id=150. - let total_rows: usize = result.iter().map(|b| b.num_rows()).sum(); - assert_eq!(total_rows, 1, "Should find exactly one row matching id=150"); + /// Reads the file with pushdown on and off, asserts the rows are identical, and + /// returns the rows plus (bytes_on, bytes_off). + async fn assert_pushdown_agrees( + case: &str, + file_path: &str, + schema: Arc, + project_field_ids: Vec, + predicate: Predicate, + ) -> (Option, u64, u64) { + let (off, bytes_off) = read_once( + file_path, + schema.clone(), + project_field_ids.clone(), + predicate.clone(), + false, + ) + .await; + let (on, bytes_on) = read_once(file_path, schema, project_field_ids, predicate, true).await; - let id_col = result[0] - .column(0) - .as_primitive::(); - assert_eq!(id_col.value(0), 150); + let off = collapse(&off); + let on = collapse(&on); + assert_eq!( + on, off, + "{case}: bloom filter pushdown changed the rows returned" + ); + + (on, bytes_on, bytes_off) } - /// Tests that bloom filter pushdown skips all row groups when value is absent. - #[tokio::test] - async fn test_bloom_filter_pushdown_value_absent() { - let schema = Arc::new( + fn rows(batch: &Option) -> usize { + batch.as_ref().map_or(0, |b| b.num_rows()) + } + + fn value_schema(iceberg_type: Type) -> Arc { + Arc::new( Schema::builder() .with_schema_id(1) - .with_fields(vec![ - NestedField::required(1, "id", Type::Primitive(PrimitiveType::Int)).into(), - ]) + .with_fields(vec![NestedField::required(1, "v", iceberg_type).into()]) .build() .unwrap(), + ) + } + + /// Writes a single-column file, one row group per group index. + fn write_value_fixture( + path: &str, + data_type: DataType, + make_group: impl Fn(i32) -> ArrayRef, + with_bloom_filters: bool, + ) { + let arrow_schema = Arc::new(ArrowSchema::new(vec![field_with_id("v", data_type, 1)])); + let row_groups = (0..GROUPS) + .map(|g| RecordBatch::try_new(arrow_schema.clone(), vec![make_group(g)]).unwrap()) + .collect(); + write_row_groups(path, arrow_schema, row_groups, with_bloom_filters); + } + + /// The core check for one physical encoding: a value that is present must + /// survive pushdown, and one that is absent must prune every row group. The + /// first catches a probe that encodes wrongly and drops real rows; the second + /// catches a probe that silently never prunes. + async fn assert_prunes_and_agrees( + case: &str, + file_path: &str, + schema: Arc, + present: Datum, + absent: Datum, + ) { + let (on, bytes_on, bytes_off) = assert_pushdown_agrees( + &format!("{case}/present"), + file_path, + schema.clone(), + vec![1], + Reference::new("v").equal_to(present), + ) + .await; + assert_eq!(rows(&on), 1, "{case}/present: expected one matching row"); + assert!( + bytes_on < bytes_off, + "{case}/present: pushdown must skip row groups: {bytes_on} vs {bytes_off}" ); - let arrow_schema = Arc::new(ArrowSchema::new(vec![ - Field::new("id", DataType::Int32, false).with_metadata(HashMap::from([( - PARQUET_FIELD_ID_META_KEY.to_string(), - "1".to_string(), - )])), - ])); + let (on, bytes_on, bytes_off) = assert_pushdown_agrees( + &format!("{case}/absent"), + file_path, + schema, + vec![1], + Reference::new("v").equal_to(absent), + ) + .await; + assert_eq!(rows(&on), 0, "{case}/absent: expected no matching rows"); + assert!( + bytes_on * 2 < bytes_off, + "{case}/absent: pruning every row group should cut reads sharply: \ + {bytes_on} vs {bytes_off}" + ); + } - let tmp_dir = TempDir::new().unwrap(); - let file_path = format!("{}/bloom_absent.parquet", tmp_dir.path().to_str().unwrap()); + /// Value present in exactly one row group: `interleaved(1, 51)`. + const PRESENT_INT: i32 = 51 * STRIDE + 1; + /// Not congruent to any group index mod STRIDE, so absent from the file while + /// still inside every row group's min/max range. + const ABSENT_INT: i32 = 7; + + fn int_group(g: i32) -> ArrayRef { + Arc::new(Int32Array::from( + (0..ROWS_PER_GROUP) + .map(|i| interleaved(g, i)) + .collect::>(), + )) + } - let props = WriterProperties::builder() - .set_compression(Compression::SNAPPY) - .set_max_row_group_row_count(Some(100)) - .set_bloom_filter_enabled(true) - .build(); + #[tokio::test] + async fn test_bloom_pushdown_int32_eq() { + let tmp = TempDir::new().unwrap(); + let path = format!("{}/int32.parquet", tmp.path().to_str().unwrap()); + write_value_fixture(&path, DataType::Int32, int_group, true); + + assert_prunes_and_agrees( + "int32", + &path, + value_schema(Type::Primitive(PrimitiveType::Int)), + Datum::int(PRESENT_INT), + Datum::int(ABSENT_INT), + ) + .await; + } - let file = File::create(&file_path).unwrap(); - let mut writer = ArrowWriter::try_new(file, arrow_schema.clone(), Some(props)).unwrap(); + /// `IN` takes a different evaluator path from `eq`: it must keep the row group + /// when any literal might be present, and prune only when all are absent. + #[tokio::test] + async fn test_bloom_pushdown_int32_in() { + let tmp = TempDir::new().unwrap(); + let path = format!("{}/int32_in.parquet", tmp.path().to_str().unwrap()); + write_value_fixture(&path, DataType::Int32, int_group, true); + let schema = value_schema(Type::Primitive(PrimitiveType::Int)); + + // One present literal keeps its row group; the absent one must not suppress it. + let (on, bytes_on, bytes_off) = assert_pushdown_agrees( + "int32_in/mixed", + &path, + schema.clone(), + vec![1], + Reference::new("v").is_in([Datum::int(PRESENT_INT), Datum::int(ABSENT_INT)]), + ) + .await; + assert_eq!(rows(&on), 1); + assert!(bytes_on < bytes_off, "{bytes_on} vs {bytes_off}"); + + // All literals absent: every row group prunes. + let (on, bytes_on, bytes_off) = assert_pushdown_agrees( + "int32_in/all_absent", + &path, + schema, + vec![1], + Reference::new("v").is_in([Datum::int(ABSENT_INT), Datum::int(ABSENT_INT + 1)]), + ) + .await; + assert_eq!(rows(&on), 0); + assert!(bytes_on * 2 < bytes_off, "{bytes_on} vs {bytes_off}"); + } - for batch_start in [0, 100, 200] { - let batch = RecordBatch::try_new(arrow_schema.clone(), vec![Arc::new( - Int32Array::from((batch_start..batch_start + 100).collect::>()), - )]) - .unwrap(); - writer.write(&batch).unwrap(); - } - writer.close().unwrap(); + #[tokio::test] + async fn test_bloom_pushdown_string_eq() { + let tmp = TempDir::new().unwrap(); + let path = format!("{}/string.parquet", tmp.path().to_str().unwrap()); + write_value_fixture( + &path, + DataType::Utf8, + |g| { + Arc::new(StringArray::from( + (0..ROWS_PER_GROUP) + .map(|i| format!("{:016}", interleaved(g, i))) + .collect::>(), + )) + }, + true, + ); - let file_io = FileIO::new_with_fs(); + assert_prunes_and_agrees( + "string", + &path, + value_schema(Type::Primitive(PrimitiveType::String)), + Datum::string(format!("{PRESENT_INT:016}")), + Datum::string(format!("{ABSENT_INT:016}")), + ) + .await; + } - // Query for id = 999, which doesn't exist in any row group. - // All row groups should be pruned by bloom filter. - let predicate = Reference::new("id").equal_to(Datum::int(999)); + /// Decimal precision decides the Parquet physical type, and the probe has to be + /// encoded to match: precision 9 -> INT32, 15 -> INT64, 25 -> FIXED_LEN_BYTE_ARRAY. + /// Each width is a separate arm of `check_in_bloom_filter`. + async fn check_decimal_width(name: &str, precision: u32) { + const SCALE: u32 = 2; + let tmp = TempDir::new().unwrap(); + let path = format!("{}/{name}.parquet", tmp.path().to_str().unwrap()); + + write_value_fixture( + &path, + DataType::Decimal128(precision as u8, SCALE as i8), + |g| { + let values: Vec = (0..ROWS_PER_GROUP) + .map(|i| i128::from(interleaved(g, i))) + .collect(); + Arc::new( + Decimal128Array::from(values) + .with_precision_and_scale(precision as u8, SCALE as i8) + .unwrap(), + ) + }, + true, + ); - let reader = ArrowReaderBuilder::new(file_io, Runtime::current()) - .with_bloom_filter_enabled(true) - .build(); + let datum = |mantissa: i32| { + Datum::decimal_with_precision( + crate::spec::decimal_utils::decimal_from_i128_with_scale( + i128::from(mantissa), + SCALE, + ), + precision, + ) + .unwrap() + }; - let task = FileScanTask::builder() - .with_file_size_in_bytes(std::fs::metadata(&file_path).unwrap().len()) - .with_start(0) - .with_length(0) - .with_data_file_path(file_path.clone()) - .with_data_file_format(DataFileFormat::Parquet) - .with_schema(schema.clone()) - .with_project_field_ids(vec![1]) - .with_predicate(Some(predicate.bind(schema, true).unwrap())) - .with_case_sensitive(false) - .build() - .unwrap(); + assert_prunes_and_agrees( + name, + &path, + value_schema(Type::Primitive(PrimitiveType::Decimal { + precision, + scale: SCALE, + })), + datum(PRESENT_INT), + datum(ABSENT_INT), + ) + .await; + } - let tasks = Box::pin(futures::stream::iter(vec![Ok(task)])) as FileScanTaskStream; + #[tokio::test] + async fn test_bloom_pushdown_decimal_int32() { + check_decimal_width("decimal_int32", 9).await; + } - let result = reader - .read(tasks) + #[tokio::test] + async fn test_bloom_pushdown_decimal_int64() { + check_decimal_width("decimal_int64", 15).await; + } + + #[tokio::test] + async fn test_bloom_pushdown_decimal_fixed_len_byte_array() { + check_decimal_width("decimal_flba", 25).await; + } + + /// Widening a decimal's precision does not rewrite existing files, so the file + /// keeps its original physical width while the bound literal arrives at the new + /// precision. Deriving the probe encoding from the schema instead of the file + /// would miss every entry the writer inserted and prune matching row groups. + #[tokio::test] + async fn test_bloom_pushdown_decimal_widened_precision() { + const SCALE: u32 = 2; + let tmp = TempDir::new().unwrap(); + let path = format!("{}/decimal_widened.parquet", tmp.path().to_str().unwrap()); + + // File written at precision 9 -> INT32. + write_value_fixture( + &path, + DataType::Decimal128(9, SCALE as i8), + |g| { + let values: Vec = (0..ROWS_PER_GROUP) + .map(|i| i128::from(interleaved(g, i))) + .collect(); + Arc::new( + Decimal128Array::from(values) + .with_precision_and_scale(9, SCALE as i8) + .unwrap(), + ) + }, + true, + ); + + // Table schema has since widened to precision 30. + let schema = value_schema(Type::Primitive(PrimitiveType::Decimal { + precision: 30, + scale: SCALE, + })); + let datum = |mantissa: i32| { + Datum::decimal_with_precision( + crate::spec::decimal_utils::decimal_from_i128_with_scale( + i128::from(mantissa), + SCALE, + ), + 30, + ) .unwrap() - .stream() - .try_collect::>() - .await - .unwrap(); + }; - let total_rows: usize = result.iter().map(|b| b.num_rows()).sum(); - assert_eq!( - total_rows, 0, - "Should find zero rows when value is absent from all bloom filters" + assert_prunes_and_agrees( + "decimal_widened", + &path, + schema, + datum(PRESENT_INT), + datum(ABSENT_INT), + ) + .await; + } + + /// `Sbbf` hashes raw IEEE bytes, so it treats `-0.0` and `0.0` as distinct. The + /// row filter's `arrow_ord::cmp::eq` happens to agree today, but for unrelated + /// reasons, so pin that the two stay consistent. + #[tokio::test] + async fn test_bloom_pushdown_float_signed_zero() { + let tmp = TempDir::new().unwrap(); + let path = format!("{}/float_zero.parquet", tmp.path().to_str().unwrap()); + + // Row group 0 holds only -0.0; row group 1 only +0.0; row group 2 neither. + write_value_fixture( + &path, + DataType::Float32, + |g| { + let fill = match g { + 0 => -0.0f32, + 1 => 0.0f32, + _ => 7.5f32, + }; + Arc::new(Float32Array::from(vec![fill; ROWS_PER_GROUP as usize])) + }, + true, ); + + let schema = value_schema(Type::Primitive(PrimitiveType::Float)); + + for (label, probe) in [("positive_zero", 0.0f32), ("negative_zero", -0.0f32)] { + let (on, _, _) = assert_pushdown_agrees( + label, + &path, + schema.clone(), + vec![1], + Reference::new("v").equal_to(Datum::float(probe)), + ) + .await; + // Whatever the reader's notion of equality, pushdown must not change it. + assert!( + rows(&on) > 0, + "{label}: expected the baseline to match rows" + ); + } + } + + /// A file with no bloom filters at all must read identically with the option on, + /// exercising the conservative might-match path end to end. + #[tokio::test] + async fn test_bloom_pushdown_file_without_bloom_filters() { + let tmp = TempDir::new().unwrap(); + let path = format!("{}/no_filters.parquet", tmp.path().to_str().unwrap()); + write_value_fixture(&path, DataType::Int32, int_group, false); + + let (on, _, _) = assert_pushdown_agrees( + "no_filters", + &path, + value_schema(Type::Primitive(PrimitiveType::Int)), + vec![1], + Reference::new("v").equal_to(Datum::int(PRESENT_INT)), + ) + .await; + assert_eq!(rows(&on), 1); } } From 351f1dd49bdbc262cc1125d093937f5bc38c9691 Mon Sep 17 00:00:00 2001 From: Xander Date: Thu, 10 Sep 2026 12:36:11 +0100 Subject: [PATCH 20/25] remove boolean --- .../expr/visitors/bloom_filter_evaluator.rs | 81 ++++++++++++++++++- 1 file changed, 77 insertions(+), 4 deletions(-) diff --git a/crates/iceberg/src/expr/visitors/bloom_filter_evaluator.rs b/crates/iceberg/src/expr/visitors/bloom_filter_evaluator.rs index 65cfa3b2dd..618b56c680 100644 --- a/crates/iceberg/src/expr/visitors/bloom_filter_evaluator.rs +++ b/crates/iceberg/src/expr/visitors/bloom_filter_evaluator.rs @@ -29,7 +29,7 @@ use crate::Result; use crate::expr::visitors::bound_predicate_visitor::{BoundPredicateVisitor, visit}; use crate::expr::{BoundPredicate, BoundReference}; use crate::spec::decimal_utils::decimal_to_fixed_length_bytes_exact; -use crate::spec::{Datum, PrimitiveLiteral}; +use crate::spec::{Datum, PrimitiveLiteral, PrimitiveType, Type}; const ROW_GROUP_MIGHT_MATCH: Result = Ok(true); const ROW_GROUP_CANT_MATCH: Result = Ok(false); @@ -95,12 +95,29 @@ impl BloomFilterEvaluator<'_> { /// the evaluator can act on it. In particular `not` discards its subtree: the /// evaluator's `not` returns might-match regardless, so a filter fetched for a /// field under a `NOT` could never prune and would be pure wasted I/O. +/// +/// Boolean fields are skipped for the same reason: with only two possible values, +/// a column chunk's min/max statistics already determine membership exactly, so a +/// bloom filter cannot prune anything statistics did not, and reading one costs a +/// round trip to learn nothing. pub(crate) fn collect_bloom_filter_field_ids(predicate: &BoundPredicate) -> Result> { visit(&mut BloomFilterFieldIdCollector, predicate) } struct BloomFilterFieldIdCollector; +/// Field IDs worth fetching a bloom filter for: none if probing the column could +/// never prune. +fn probeable_field_id(reference: &BoundReference) -> HashSet { + if matches!( + reference.field().field_type.as_ref(), + Type::Primitive(PrimitiveType::Boolean) + ) { + return HashSet::new(); + } + HashSet::from([reference.field().id]) +} + impl BoundPredicateVisitor for BloomFilterFieldIdCollector { type T = HashSet; @@ -179,7 +196,7 @@ impl BoundPredicateVisitor for BloomFilterFieldIdCollector { } fn eq(&mut self, r: &BoundReference, _l: &Datum, _p: &BoundPredicate) -> Result { - Ok(HashSet::from([r.field().id])) + Ok(probeable_field_id(r)) } fn not_eq(&mut self, _r: &BoundReference, _l: &Datum, _p: &BoundPredicate) -> Result { @@ -210,7 +227,7 @@ impl BoundPredicateVisitor for BloomFilterFieldIdCollector { _literals: &FnvHashSet, _p: &BoundPredicate, ) -> Result { - Ok(HashSet::from([r.field().id])) + Ok(probeable_field_id(r)) } fn not_in( @@ -238,7 +255,10 @@ fn check_in_bloom_filter(column: &ColumnBloomFilter, datum: &Datum) -> bool { let physical_type = *physical_type; match datum.literal() { - PrimitiveLiteral::Boolean(v) => sbbf.check(v), + // Parquet does not define bloom filter semantics for BOOLEAN — parquet-java + // only hashes int/long/float/double/binary. arrow-rs does write them, but min/max + // statistics already decide membership for a two-valued domain, so there is nothing to gain. + PrimitiveLiteral::Boolean(_) => true, // A promoted column (int -> long, float -> double) keeps its original // physical width in files written before the promotion, and the writer // hashed that width, so probe at the file's width rather than the @@ -792,6 +812,59 @@ mod tests { assert_eq!(collected_ids(predicate), vec![2]); } + /// A boolean column's min/max statistics already determine membership exactly, + /// so fetching a bloom filter for one can never prune and is pure wasted I/O. + #[test] + fn test_does_not_collect_boolean_field_ids() { + let schema = Schema::builder() + .with_schema_id(1) + .with_fields(vec![ + NestedField::required(1, "flag", Type::Primitive(PrimitiveType::Boolean)).into(), + NestedField::required(2, "id", Type::Primitive(PrimitiveType::Int)).into(), + ]) + .build() + .unwrap(); + + let predicate = Reference::new("flag") + .equal_to(Datum::bool(true)) + .and(Reference::new("flag").is_in([Datum::bool(false)])) + .and(Reference::new("id").equal_to(Datum::int(1))) + .bind(schema.into(), true) + .unwrap(); + + assert_eq!( + collected_ids(predicate), + vec![2], + "only the non-boolean field should be collected" + ); + } + + /// Even given a filter, a boolean probe stays conservative: Parquet does not + /// define bloom filter semantics for BOOLEAN, so the encoding is not portable. + #[test] + fn test_boolean_probe_never_prunes() { + let schema = Schema::builder() + .with_schema_id(1) + .with_fields(vec![ + NestedField::required(1, "flag", Type::Primitive(PrimitiveType::Boolean)).into(), + ]) + .build() + .unwrap(); + + let mut sbbf = Sbbf::new_with_ndv_fpp(10, 0.01).unwrap(); + sbbf.insert(&true); + let filters = HashMap::from([(1, ColumnBloomFilter::new(sbbf, PhysicalType::BOOLEAN, 0))]); + + // `false` was never inserted, yet the row group must survive. + let predicate = Reference::new("flag") + .equal_to(Datum::bool(false)) + .bind(schema.into(), true) + .unwrap(); + + let result = BloomFilterEvaluator::eval(&predicate, &filters).unwrap(); + assert!(result, "boolean probes must never prune"); + } + /// Range and `not_eq` predicates cannot be probed, so they contribute nothing. #[test] fn test_does_not_collect_unprobeable_operators() { From 631461dd6df0852448c2a294291c3b10b500070b Mon Sep 17 00:00:00 2001 From: Xander Date: Thu, 10 Sep 2026 20:44:23 +0100 Subject: [PATCH 21/25] doc --- crates/iceberg/src/arrow/reader/mod.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/crates/iceberg/src/arrow/reader/mod.rs b/crates/iceberg/src/arrow/reader/mod.rs index c9fdecbfd3..9d9c8be48e 100644 --- a/crates/iceberg/src/arrow/reader/mod.rs +++ b/crates/iceberg/src/arrow/reader/mod.rs @@ -111,8 +111,9 @@ impl ArrowReaderBuilder { /// checked. Row groups where the bloom filter proves the value is absent /// are skipped entirely. /// - /// Defaults to disabled, as reading bloom filters requires additional I/O - /// per column per row group. + /// Defaults to disabled. Each bloom filter is a separate read, and they are + /// issued serially — one round trip per relevant column per row group, before + /// any data is read. TODO(#3191) pub fn with_bloom_filter_enabled(mut self, bloom_filter_enabled: bool) -> Self { self.bloom_filter_enabled = bloom_filter_enabled; self From 053ddb3b5f379d62aa47ccf38ad3adc94e693c22 Mon Sep 17 00:00:00 2001 From: Xander Date: Fri, 11 Sep 2026 09:12:41 +0100 Subject: [PATCH 22/25] update 0 test --- crates/iceberg/src/arrow/reader/row_filter.rs | 34 +++++++++++++------ 1 file changed, 24 insertions(+), 10 deletions(-) diff --git a/crates/iceberg/src/arrow/reader/row_filter.rs b/crates/iceberg/src/arrow/reader/row_filter.rs index 14eb712f46..93f27c357a 100644 --- a/crates/iceberg/src/arrow/reader/row_filter.rs +++ b/crates/iceberg/src/arrow/reader/row_filter.rs @@ -1679,30 +1679,43 @@ mod tests { .await; } - /// `Sbbf` hashes raw IEEE bytes, so it treats `-0.0` and `0.0` as distinct. The - /// row filter's `arrow_ord::cmp::eq` happens to agree today, but for unrelated - /// reasons, so pin that the two stay consistent. + /// `Sbbf` hashes raw IEEE bytes, so `-0.0` and `0.0` are distinct entries, and arrow's + /// float `is_eq` is bitwise for the same reason. That agreement is what makes it safe + /// to prune a row group holding only the opposite zero: if the row filter ever became + /// IEEE-lenient, those pruned rows would be rows it should have returned, and the + /// on/off comparison catches the divergence. A probe that canonicalized the sign + /// instead only over-keeps, costing pruning rather than correctness. + /// + /// Each row group pairs its zero with a larger filler value so its min/max range + /// spans the other zero. Statistics therefore cannot prune it and the bloom filter + /// is what discriminates, without relying on the writer widening a single-value + /// group's bounds to `[-0.0, +0.0]`. #[tokio::test] async fn test_bloom_pushdown_float_signed_zero() { + const FILLER: f32 = 5.0; let tmp = TempDir::new().unwrap(); let path = format!("{}/float_zero.parquet", tmp.path().to_str().unwrap()); - // Row group 0 holds only -0.0; row group 1 only +0.0; row group 2 neither. + // Row group 0 holds -0.0 and never +0.0; row group 1 the reverse; row group 2 + // neither zero. write_value_fixture( &path, DataType::Float32, |g| { - let fill = match g { + let zero = match g { 0 => -0.0f32, 1 => 0.0f32, _ => 7.5f32, }; - Arc::new(Float32Array::from(vec![fill; ROWS_PER_GROUP as usize])) + let mut values = vec![zero; ROWS_PER_GROUP as usize - 1]; + values.push(FILLER); + Arc::new(Float32Array::from(values)) }, true, ); let schema = value_schema(Type::Primitive(PrimitiveType::Float)); + let zero_rows = ROWS_PER_GROUP as usize - 1; for (label, probe) in [("positive_zero", 0.0f32), ("negative_zero", -0.0f32)] { let (on, _, _) = assert_pushdown_agrees( @@ -1713,10 +1726,11 @@ mod tests { Reference::new("v").equal_to(Datum::float(probe)), ) .await; - // Whatever the reader's notion of equality, pushdown must not change it. - assert!( - rows(&on) > 0, - "{label}: expected the baseline to match rows" + // Only the group holding this exact zero matches, on both paths. + assert_eq!( + rows(&on), + zero_rows, + "{label}: expected only the row group holding this zero to match" ); } } From 1a1540e47de3ca0ef2f351285bd1934eb1b0a268 Mon Sep 17 00:00:00 2001 From: Xander Date: Fri, 11 Sep 2026 14:54:31 +0100 Subject: [PATCH 23/25] UUID --- .../expr/visitors/bloom_filter_evaluator.rs | 58 ++++++++++++++++++- 1 file changed, 57 insertions(+), 1 deletion(-) diff --git a/crates/iceberg/src/expr/visitors/bloom_filter_evaluator.rs b/crates/iceberg/src/expr/visitors/bloom_filter_evaluator.rs index 618b56c680..dde2ae4676 100644 --- a/crates/iceberg/src/expr/visitors/bloom_filter_evaluator.rs +++ b/crates/iceberg/src/expr/visitors/bloom_filter_evaluator.rs @@ -338,7 +338,7 @@ fn check_in_bloom_filter(column: &ColumnBloomFilter, datum: &Datum) -> bool { PrimitiveLiteral::UInt128(v) => { // UUID: stored as FIXED_LEN_BYTE_ARRAY(16), big-endian let bytes = v.to_be_bytes(); - sbbf.check(&ByteArray::from(bytes.to_vec())) + sbbf.check(bytes.as_slice()) } PrimitiveLiteral::AboveMax | PrimitiveLiteral::BelowMin => true, } @@ -1146,6 +1146,62 @@ mod tests { ); } + fn create_uuid_schema() -> Schema { + Schema::builder() + .with_schema_id(1) + .with_fields(vec![ + NestedField::required(1, "u", Type::Primitive(PrimitiveType::Uuid)).into(), + ]) + .build() + .unwrap() + } + + /// The probe hands the 16 big-endian bytes to `check` as a slice while the writer + /// inserts them as a `ByteArray`; both hash the same bytes, so a present UUID must + /// still might-match. + #[test] + fn test_uuid_fixed_bytes_present() { + let uuid = uuid::Uuid::parse_str("f79c3e09-677c-4bbd-a479-3f349cb785e7").unwrap(); + + let mut sbbf = Sbbf::new_with_ndv_fpp(10, 0.01).unwrap(); + sbbf.insert(&ByteArray::from(uuid.as_u128().to_be_bytes().to_vec())); + + let bloom_filters = HashMap::from([( + 1, + ColumnBloomFilter::new(sbbf, PhysicalType::FIXED_LEN_BYTE_ARRAY, 16), + )]); + + let predicate = Reference::new("u") + .equal_to(Datum::uuid(uuid)) + .bind(create_uuid_schema().into(), true) + .unwrap(); + + let result = BloomFilterEvaluator::eval(&predicate, &bloom_filters).unwrap(); + assert!(result, "UUID present should might-match"); + } + + #[test] + fn test_uuid_fixed_bytes_absent() { + let present = uuid::Uuid::parse_str("f79c3e09-677c-4bbd-a479-3f349cb785e7").unwrap(); + let absent = uuid::Uuid::parse_str("00000000-0000-4000-8000-000000000001").unwrap(); + + let mut sbbf = Sbbf::new_with_ndv_fpp(10, 0.01).unwrap(); + sbbf.insert(&ByteArray::from(present.as_u128().to_be_bytes().to_vec())); + + let bloom_filters = HashMap::from([( + 1, + ColumnBloomFilter::new(sbbf, PhysicalType::FIXED_LEN_BYTE_ARRAY, 16), + )]); + + let predicate = Reference::new("u") + .equal_to(Datum::uuid(absent)) + .bind(create_uuid_schema().into(), true) + .unwrap(); + + let result = BloomFilterEvaluator::eval(&predicate, &bloom_filters).unwrap(); + assert!(!result, "UUID absent should not match"); + } + /// Negative decimal values should also work correctly. #[test] fn test_decimal_negative_int32() { From 76aca80e291c28fc643e23657509b3b070517a6e Mon Sep 17 00:00:00 2001 From: Xander Date: Fri, 11 Sep 2026 14:58:31 +0100 Subject: [PATCH 24/25] update comment --- crates/iceberg/src/expr/visitors/bloom_filter_evaluator.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/iceberg/src/expr/visitors/bloom_filter_evaluator.rs b/crates/iceberg/src/expr/visitors/bloom_filter_evaluator.rs index dde2ae4676..3dfc8ec4a9 100644 --- a/crates/iceberg/src/expr/visitors/bloom_filter_evaluator.rs +++ b/crates/iceberg/src/expr/visitors/bloom_filter_evaluator.rs @@ -272,8 +272,8 @@ fn check_in_bloom_filter(column: &ColumnBloomFilter, datum: &Datum) -> bool { PhysicalType::INT64 => sbbf.check(v), PhysicalType::INT32 => match i32::try_from(*v) { Ok(narrowed) => sbbf.check(&narrowed), - // Too wide for an INT32 column to hold, so there is nothing - // meaningful to probe — keep the row group. + // Too wide for an INT32 column to hold, so the value is provably + // absent and pruning here would be sound; kept conservative for now. Err(_) => true, }, _ => true, From 1f54c4016567561c69fbf6db0095e43dd757a80a Mon Sep 17 00:00:00 2001 From: Xander Date: Fri, 11 Sep 2026 15:11:42 +0100 Subject: [PATCH 25/25] update test doc --- crates/iceberg/src/arrow/reader/row_filter.rs | 24 +++++++++++-------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/crates/iceberg/src/arrow/reader/row_filter.rs b/crates/iceberg/src/arrow/reader/row_filter.rs index 93f27c357a..34b7391134 100644 --- a/crates/iceberg/src/arrow/reader/row_filter.rs +++ b/crates/iceberg/src/arrow/reader/row_filter.rs @@ -1679,17 +1679,21 @@ mod tests { .await; } - /// `Sbbf` hashes raw IEEE bytes, so `-0.0` and `0.0` are distinct entries, and arrow's - /// float `is_eq` is bitwise for the same reason. That agreement is what makes it safe - /// to prune a row group holding only the opposite zero: if the row filter ever became - /// IEEE-lenient, those pruned rows would be rows it should have returned, and the - /// on/off comparison catches the divergence. A probe that canonicalized the sign - /// instead only over-keeps, costing pruning rather than correctness. + /// `Sbbf` hashes raw IEEE bytes, so `-0.0` and `0.0` are distinct entries. Arrow's + /// float equality is bitwise too, for an independent reason: `ArrowNativeTypeOp::is_eq` + /// implements total-order (`total_cmp`) semantics as `to_bits() == to_bits()`, so + /// `-0.0 != 0.0` and `NaN == NaN` there. That agreement is what makes it safe to prune + /// a row group holding only the opposite zero: if `is_eq` ever became IEEE-lenient, + /// those pruned rows would be rows the row filter should have returned. A probe that + /// canonicalized the sign instead only over-keeps, costing pruning rather than + /// correctness — but it would also hide such a change from this test. /// - /// Each row group pairs its zero with a larger filler value so its min/max range - /// spans the other zero. Statistics therefore cannot prune it and the bloom filter - /// is what discriminates, without relying on the writer widening a single-value - /// group's bounds to `[-0.0, +0.0]`. + /// Each row group pairs its zero with a larger filler value so its min/max range spans + /// the other zero. Neither statistics path can prune it, so the bloom-off read really + /// does evaluate `is_eq` against `ROWS_PER_GROUP - 1` copies of the opposite zero: that + /// arm is the control pinning arrow's semantics, and IEEE-lenient equality would double + /// the row count it returns. Pairing with a filler also avoids relying on the writer + /// widening a single-value group's bounds to `[-0.0, +0.0]`. #[tokio::test] async fn test_bloom_pushdown_float_signed_zero() { const FILLER: f32 = 5.0;