laskoviymishka commented on code in PR #3438:
URL: https://github.com/apache/iceberg-rust/pull/3438#discussion_r4537029434
##########
crates/iceberg/src/arrow/reader/projection.rs:
##########
@@ -395,6 +417,118 @@ fn parquet_field_id(field: &Field) -> Option<i32> {
.and_then(|field_id| i32::from_str(field_id).ok())
}
+struct PresenceCandidate {
+ repetition: u32,
+ /// Leaf indexes to read: one leaf, or a whole map.
+ leaves: Range<usize>,
+}
+
+/// One file leaf under each projected struct that no selected leaf covers, so
the
+/// struct keeps its definition level. Like Java's `PresenceColumnSelector`,
inner
+/// structs go first, and the leaf with the lowest repetition level wins, then
the first.
+fn presence_leaves(
+ fields: &Fields,
+ projected_structs: &HashSet<i32>,
+ leaf_field_ids: &HashSet<i32>,
+) -> Vec<PresenceCandidate> {
+ if projected_structs.is_empty() {
+ return Vec::new();
+ }
+ let mut walk = PresenceWalk {
+ projected_structs,
+ leaf_field_ids,
+ index: 0,
+ chosen: Vec::new(),
+ };
+ for field in fields {
+ walk.visit(field, 0, false);
+ }
+ walk.chosen
+}
+
+struct PresenceWalk<'a> {
+ projected_structs: &'a HashSet<i32>,
+ leaf_field_ids: &'a HashSet<i32>,
+ index: usize,
+ chosen: Vec<PresenceCandidate>,
+}
+
+impl PresenceWalk<'_> {
+ /// Returns whether a selected leaf is under `field`, and if not, the best
presence
+ /// candidate. A candidate also goes through the projection's type check,
so what it
+ /// reads must convert and every field on its path needs a field id.
Candidates are
+ /// only built under a projected struct, since nothing else can use them.
+ fn visit(
+ &mut self,
+ field: &Field,
+ repetition: u32,
+ in_projected: bool,
+ ) -> (bool, Option<PresenceCandidate>) {
+ // Unwrap once, like `Fields::filter_leaves`, to count leaves the same
way.
+ let data_type = match field.data_type() {
Review Comment:
Could we add a case to `test_projection_mask_reads_one_presence_leaf` with a
Dictionary or FixedSizeList field under the struct? This counter mirrors
arrow's
[`filter_leaves`](https://github.com/apache/arrow-rs/blob/9ea30dfacb7003de9ffc87ef50231f18229fcdb5/arrow-schema/src/fields.rs#L147-L247)
by hand, and its indexes feed both `filter_leaves` and
`ProjectionMask::leaves`. Those arms are reachable through an `ARROW:schema`
hint, but no test drives them through the walk, so a change in arrow's
traversal would drift without a compile error.
##########
crates/iceberg/src/arrow/reader/projection.rs:
##########
@@ -69,22 +70,30 @@ impl ArrowReader {
/// Recursively extract leaf field IDs because Parquet projection works at
the leaf column level.
/// Nested types (struct/list/map) are flattened in Parquet's columnar
format.
- fn include_leaf_field_id(field: &NestedField, field_ids: &mut Vec<i32>) {
+ /// Also collects the ids of non-empty structs, which may need a presence
leaf.
+ fn include_leaf_field_id(
Review Comment:
Could the two accumulators travel as one value, e.g. `struct ProjectedFields
{ leaf_ids: Vec<i32>, struct_ids: HashSet<i32> }` with this walk as a method?
The name would stop saying "leaf" while it also collects struct ids, and
[`get_arrow_projection_mask_with_field_ids`](https://github.com/apache/iceberg-rust/blob/6bf5310569a88308da47f79e47bfe2faa2a20f06/crates/iceberg/src/arrow/reader/projection.rs#L159-L201)
could take `&ProjectedFields` instead of a sixth positional param. `leaf_ids`
should stay a `Vec`, since its order is used further down.
--
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]
