mbutrovich commented on code in PR #3257: URL: https://github.com/apache/iceberg-rust/pull/3257#discussion_r4482965279
########## crates/iceberg/src/arrow/reader/row_filter.rs: ########## Review Comment: Could `PageIndexEvaluator` borrow the row group's page index instead of taking cloned vectors? As written, this deep-clones every column's `ColumnIndexMetaData` and `OffsetIndexMetaData` for every selected row group, including columns the predicate never reads. Each clone copies the per-page min and max values, null pages, null counts, level histograms, and page locations. On `main` the evaluator borrowed these straight from the metadata ([row_filter.rs](https://github.com/apache/iceberg-rust/blob/f533cd6175bb42b5788537f589cd9450a3054d27/crates/iceberg/src/arrow/reader/row_filter.rs#L139-L146)), so this is new work per file on the scan path. Parquet 60's [`RowGroupPageIndex`](https://github.com/apache/arrow-rs/blob/b391e5af7dff9f6909af45c01f7dddb68ba08c0f/parquet/src/file/metadata/page_index.rs#L268-L322) already has per-column `column_index(column_idx)` and `offset_index(column_idx)` lookups. If `PageIndexEvaluator` holds `page_index: &'a RowGroupPageIndex` in place of the two slices, the lookups in `calc_row_selection` become `self.page_index.column_index(parquet_column_index)` and `self.page_index.offset_index(parquet_column_index)`, and this call site becomes the suggestion below. The import is `parquet::file::metadata::page_index::RowGroupPageIndex`, because `parquet::file::metadata` doesn't re-export it. `calc_row_counts` sizes its `Vec` with `self.offset_index.len()`, which is the column count, so that would become `page_locations.len()`. In the tests, `get_test_metadata` and the copy in `eval_matches_no_rows_for_empty_row_group` reduce to `metadata.page_index_for_row_group(0)`. ```suggestion let selections_for_page = PageIndexEvaluator::eval( predicate, &parquet_metadata.page_index_for_row_group(idx), row_group_metadata, field_id_map, snapshot_schema, )?; ``` ########## crates/iceberg/src/expr/visitors/page_index_evaluator.rs: ########## @@ -185,12 +187,15 @@ impl<'a> PageIndexEvaluator<'a> { match self.row_count_cache.get(&parquet_column_index) { Some(count) => count.clone(), None => { - let Some(offset_index) = self.offset_index.get(parquet_column_index) else { - // if we have a column index, we should always have an offset index. - return Err(Error::new( - ErrorKind::Unexpected, - format!("Missing offset index for field id {field_id}"), - )); + let Some(offset_index) = self + .offset_index + .get(parquet_column_index) + .and_then(Option::as_ref) + else { + // A page index provider can load column and offset indexes + // independently. Without offsets, we cannot construct a row + // selection, so preserve correctness by skipping page pruning. + return self.select_all_rows(); Review Comment: On `main` a missing offset index returned `ErrorKind::Unexpected`, and here it selects every row. Could we add a test for this branch? No existing test reaches it: changing this `select_all_rows()` to `skip_all_rows()` leaves the existing `page_index_evaluator` tests passing. With the `RowGroupPageIndex` change above, a test along these lines covers it by dropping the offset index for `col_float` only: ```rust #[test] fn eval_selects_all_rows_when_column_lacks_offset_index() -> Result<()> { let (metadata, _temp_file) = create_test_parquet_file()?; let (iceberg_schema_ref, field_id_map) = build_iceberg_schema_and_field_map()?; // Keep both column indexes, but the offset index only for col_string. let page_index = metadata.page_index().unwrap(); let mut builder = page_index .as_any() .downcast_ref::<PageIndex>() .unwrap() .clone() .into_builder(); builder.allocate_offset_indexes(1, 2); builder.put_offset_index(page_index.offset_index(0, 1).unwrap().clone(), 0, 1); let metadata = metadata .as_ref() .clone() .into_builder() .set_page_index(Some(Arc::new(builder.build()))) .build(); let filter = Reference::new("col_float") .is_not_null() .bind(iceberg_schema_ref.clone(), false)?; let result = PageIndexEvaluator::eval( &filter, &metadata.page_index_for_row_group(0), metadata.row_group(0), &field_id_map, iceberg_schema_ref.as_ref(), )?; assert_eq!(result, vec![RowSelector::select(4096)]); Ok(()) } ``` With the full page index, the same predicate on this fixture returns `[skip(2048), select(2048)]` (`eval_is_not_null_dont_select_pages_with_all_nulls`), so the test tells the two paths apart. ########## crates/iceberg/src/expr/visitors/page_index_evaluator.rs: ########## @@ -162,7 +160,11 @@ impl<'a> PageIndexEvaluator<'a> { )); }; - let Some(column_index) = self.column_index.get(parquet_column_index) else { + let Some(column_index) = self + .column_index + .get(parquet_column_index) + .and_then(Option::as_ref) + else { // This should not happen, but we fail soft anyway so that the scan is still // successful, just a bit slower Review Comment: Should this comment change now that `ColumnIndexMetaData::NONE` is gone? In parquet 60 a column chunk without a column index is left as `None` in the page index ([parser.rs](https://github.com/apache/arrow-rs/blob/b391e5af7dff9f6909af45c01f7dddb68ba08c0f/parquet/src/file/metadata/parser.rs#L311-L316)), so it lands in this branch. On `main` that case was the `NONE` arm in `apply_predicate_to_column_index` ([page_index_evaluator.rs](https://github.com/apache/iceberg-rust/blob/f533cd6175bb42b5788537f589cd9450a3054d27/crates/iceberg/src/expr/visitors/page_index_evaluator.rs#L265-L268)). A writer that skips page statistics for a column produces it, so this is an expected path now. ```suggestion // The writer can omit the column index for a column chunk, for example // when page statistics are disabled. Without it we can't prune pages. ``` ########## crates/iceberg/src/arrow/value.rs: ########## @@ -1406,7 +1408,17 @@ mod test { let map_key_builder = Int32Builder::new(); let map_value_builder = Int32Builder::new(); - let map_builder = MapBuilder::new(None, map_key_builder, map_value_builder); + // Arrow 60 defaults to `key` and `value`; this fixture's schema uses + // the Iceberg field names `keys` and `values`. Review Comment: Could this comment say the names come from the fixture rather than from Iceberg? iceberg-rust names map fields `key` and `value` ([datatypes.rs](https://github.com/apache/iceberg-rust/blob/f533cd6175bb42b5788537f589cd9450a3054d27/crates/iceberg/src/spec/datatypes.rs#L41-L43)), so "the Iceberg field names `keys` and `values`" points a reader the wrong way. `keys` and `values` are what the fixture's hand-written Arrow type spells out at lines 1444-1445, which is why the builder has to match them. ```suggestion // Arrow 60 `MapBuilder` defaults to `key` and `value`, but this fixture's // Arrow type names the map fields `keys` and `values`. ``` -- 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]
