mbutrovich commented on code in PR #3540:
URL: https://github.com/apache/iceberg-rust/pull/3540#discussion_r4404231632
##########
crates/iceberg/src/arrow/reader/row_filter.rs:
##########
@@ -62,8 +70,18 @@ impl ArrowReader {
// After collecting required leaf column indices used in the predicate,
// creates the projection mask for the Arrow predicates.
let projection_mask = ProjectionMask::leaves(parquet_schema,
column_indices.clone());
- let predicate_func = visit(&mut converter, predicates)?;
- let arrow_predicate = ArrowPredicateFn::new(projection_mask,
predicate_func);
+ let mut predicate_func = visit(&mut converter, predicates)?;
+ // Predicates are bound to the table schema, so evaluate them on the
filter columns
+ // converted to it, as the reader's output is (e.g. a file written
before an `int` ->
+ // `long` promotion).
+ let mut transformer =
+ RecordBatchTransformerBuilder::new(snapshot_schema,
&field_ids).build();
Review Comment:
> I'd like to scope `generate_batch_transform` to the projected fields in a
follow-up, since it changes the output transformer too and several open PRs
touch that file.
Could we fix the per-file cost for the filter transformer here without
touching `record_batch_transformer.rs`? `get_row_filter` could hand the builder
a schema that holds only the filter fields. The output transformer and
`generate_batch_transform` stay as they are, so the follow-up is only needed
for the output side:
```rust
let filter_schema = Schema::builder()
.with_schema_id(snapshot_schema.schema_id())
.with_fields(
field_ids
.iter()
.filter_map(|id| snapshot_schema.field_by_id(*id).cloned()),
)
.build()?;
let mut transformer =
RecordBatchTransformerBuilder::new(Arc::new(filter_schema),
&field_ids).build();
```
I tried this on the head commit, and the `arrow::` tests (278) and `scan`
tests (105) pass. I timed building the transformer and processing one 8192-row
`int` -> `long` batch for a single filter column, in a release build:
| Table width | Full schema | Filter fields only |
| --- | --- | --- |
| 1 column | 3.3 us | 2.3 us |
| 101 columns | 15.1 us | 1.9 us |
| 1001 columns | 135.1 us | 2.0 us |
The cost now grows with the number of filter columns, not the table's width.
The new cost is one small `Schema` build per file, and the 1-column row is
where it gains least. It isn't slower there. Since this PR adds the second
transformer, keeping its setup flat would mean the PR doesn't add a per-file
cost that depends on width. Should the follow-up for the output transformer
also get an issue, so we can link it from the description?
--
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]
