mbutrovich commented on PR #3302:
URL: https://github.com/apache/iceberg-rust/pull/3302#issuecomment-6002546542
Thanks for the review, @comphead. I merged `main` and pushed changes for
each comment.
> `field_ids` in `residual_for_missing_fields` and
`MissingFieldResidualVisitor` only holds the missing fields that resolved to a
value. A name like `constant_field_ids` would match the `constants_map` wording
and set it apart from `predicate_field_ids`.
Renamed to `constant_field_ids`.
> A one-line comment at the `residual_for_missing_fields` call in
`pipeline.rs` would help.
Added.
> `constants_map` now runs twice per file when the file has partition data
and a predicate column is missing (once in `with_partition`, once in the
residual). It is cheap, so fine to leave.
I left it as is.
> The `read` closure in `assert_absent_b_reads_as_7` repeats the task and
reader setup in `read_once`, which has two callers. A `partition:
Option<(Arc<PartitionSpec>, Struct)>` parameter on `read_once` would let the
helper call it directly. The helper also builds `actual` and `expected` in
separate passes and calls `to_string()` twice per case. A single loop with
`assert_eq!(.., "{predicate}")` would do the same in one pass.
`read_once` now takes the `ArrowReader`, the partition, and the delete
files, and `assert_absent_b_reads_as_7` calls it. Passing in the reader lets
the bloom filter helper keep its reader options and lets the new helper turn
row selection on and off. The helper and the residual unit tests in
`predicate_visitor.rs` now use a single loop with a message per case.
--
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]
