mbutrovich commented on code in PR #3269: URL: https://github.com/apache/iceberg-rust/pull/3269#discussion_r4096182621
########## crates/iceberg/src/arrow/reader/pipeline.rs: ########## Review Comment: Could we remove the intersection instead of making it faster? Right now `filter_row_groups_by_byte_range` and `get_selected_row_group_indices` each walk every row group in the file, and then we intersect the two lists. That means `RowGroupMetricsEvaluator::eval` runs on row groups the byte range has already excluded. When a file is split into N tasks, every task evaluates the predicate against every row group in the file, which costs more than the intersection does. The bloom filter step a few lines below already avoids this. It takes the current selection as `candidate_row_groups` and only evaluates those ([`pipeline.rs` lines 638-655](https://github.com/apache/iceberg-rust/blob/cab98deda53aa1d5956de30e4bd6756123c8a74e/crates/iceberg/src/arrow/reader/pipeline.rs#L638-L655)). If `get_selected_row_group_indices` took the same `candidate_row_groups: &[usize]` argument, the caller could pass the byte range result (or `0..num_row_groups` when there is none, like the bloom filter step does) and assign the result directly to `selected_row_group_indices`. Each step then narrows the previous step's output, so the result stays in ascending order without relying on both inputs being sorted. DataFusion is built the same way. It keeps one per-row-group `ParquetAccessPlan`, applies [`prune_by_range`](https://github.com/apache/datafusion/blob/7570366fd929daf9ced744bb8397686b50565b18/datafusion/datasource-parquet/src/row_group_filter.rs#L256) first, and [`prune_by_statistics`](https://github.com/apache/datafusion/blob/7570366fd929daf9ced744bb8397686b50565b18/datafusion/datasource-parquet/src/row_group_filter.rs#L338) only evaluates the row groups that are still selected. It never intersects two index lists. The PR's own numbers also point this way for the split case. With 16 byte-range row groups against 512 predicate row groups, the two-pointer version takes 492 ns and binary search takes 194 ns. That's the shape a split task produces, since each task owns a few contiguous row groups. The candidate approach does no intersection at all. Could you also add a reader test that combines a byte range with a predicate while row group filtering is enabled? For example, write three row groups, give the task a range that owns row groups 1 and 2, and use a predicate that only matches row groups 0 and 2, then assert that only row group 2's rows come back. `test_file_splits_respect_byte_ranges` in `row_filter.rs` covers splits without a predicate, and I couldn't find a test that exercises both filters together. ########## crates/iceberg/src/arrow/reader/projection.rs: ########## @@ -436,7 +437,7 @@ pub(super) fn apply_name_mapping_to_arrow_schema( let mapped_field_opt = name_mapping .fields() .iter() - .find(|f| f.names().contains(&field.name().to_string())); + .find(|f| contains_name(f.names(), field.name())); Review Comment: Removing the `to_string()` here is a good fix. The lookup is still a linear scan of every mapped field for every Arrow field, though, and your 2,048-field numbers (8.51 ms with the borrowed comparison) show the quadratic cost is still there after the allocation is gone. Iceberg Java answers this lookup from an index it builds once ([`NameMapping.find`](https://github.com/apache/iceberg/blob/5e7169168db3d34e29354c6f59ec4d6e420b8d2d/core/src/main/java/org/apache/iceberg/mapping/NameMapping.java#L63-L95) goes through `lazyFieldsByName`). What do you think about building a `HashMap<&str, Option<i32>>` at the top of `apply_name_mapping_to_arrow_schema` and looking each field up in it? Using `entry(...).or_insert(...)` keeps today's first-match behavior when two mapped fields share a name: ```rust let mut field_ids_by_name = HashMap::new(); for mapped_field in name_mapping.fields() { for name in mapped_field.names() { field_ids_by_name .entry(name.as_str()) .or_insert(mapped_field.field_id()); } } ``` `HashMap` is already imported in this file. With this in place, the `contains_name` helper and `name_mapping_lookup.rs` go away. If you prefer to keep the linear scan, `f.names().iter().any(|name| name == field.name())` inline does the same thing as the helper without a new module. ########## crates/iceberg/Cargo.toml: ########## @@ -97,6 +98,14 @@ regex = { workspace = true } serde_arrow = { version = "0.14", features = ["arrow-59"] } tempfile = { workspace = true } +[[bench]] +harness = false +name = "row_group_intersection" + +[[bench]] +harness = false +name = "name_mapping_lookup" Review Comment: Could we drop the two benchmarks and the `criterion` dependency from this PR, and keep the numbers in the PR description? Each benchmark compares the new function against a copy of the pre-PR code (`apply_current_lookup`, `intersect_with_contains`). After merge, those copies are dead code that doesn't track anything in the crate. To reach the functions they measure, the benches pull source files in with `#[path = "../src/..."]`. That's also why `row_group_intersection.rs` and `name_mapping_lookup.rs` are separate modules that each hold one function. This would also be the first benchmark and the first `criterion` dependency in the workspace, and it adds 171 lines to `Cargo.lock`. The last attempt to add reader benchmarks, #2558, is on hold until the file format API refactor lands, because of the upkeep cost on people changing reader internals. Recent reader perf PRs such as #3080 and #3015 kept their harnesses local and put the before and after numbers in the PR description, and that would work well here too. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
