laskoviymishka commented on code in PR #3396:
URL: https://github.com/apache/iceberg-rust/pull/3396#discussion_r4241385230
##########
crates/iceberg/src/arrow/reader/projection.rs:
##########
@@ -178,23 +178,16 @@ impl ArrowReader {
// Pre-project only the fields that have been selected, possibly
avoiding converting
// some Arrow types that are not yet supported.
- let mut projected_fields: HashMap<arrow_schema::FieldRef, i32> =
HashMap::new();
let projected_arrow_schema = ArrowSchema::new_with_metadata(
fields.filter_leaves(|_, f| {
- f.metadata()
- .get(PARQUET_FIELD_ID_META_KEY)
- .and_then(|field_id| i32::from_str(field_id).ok())
- .is_some_and(|field_id| {
- projected_fields.insert((*f).clone(), field_id);
- leaf_field_id_set.contains(&field_id)
- })
+ parquet_field_id(f).is_some_and(|field_id|
leaf_field_id_set.contains(&field_id))
Review Comment:
tiny thing — the second pass and the `build_field_id_map_from_arrow_schema`
call site both name this `field`; only this first-pass closure calls it `f`.
I'd match them so it reads consistently.
##########
crates/iceberg/src/arrow/reader/projection.rs:
##########
@@ -379,18 +372,22 @@ pub(super) fn build_fallback_field_id_map(
fn build_field_id_map_from_arrow_schema(arrow_schema: &ArrowSchemaRef) ->
HashMap<i32, usize> {
let mut column_map = HashMap::new();
arrow_schema.fields().filter_leaves(|idx, field| {
- if let Some(field_id) = field
- .metadata()
- .get(PARQUET_FIELD_ID_META_KEY)
- .and_then(|value| i32::from_str(value).ok())
- {
+ if let Some(field_id) = parquet_field_id(field) {
column_map.insert(field_id, idx);
}
false
});
column_map
}
+/// Returns the field ID in `field`'s `PARQUET:field_id` metadata, if it has a
valid one.
Review Comment:
I'd spell out what `None` means here — key absent *or* value not a valid
`i32` — since that case isn't a no-op: the leaf drops out of the projection and
gets null-filled downstream, so a corrupt `PARQUET:field_id` surfaces as a
missing column rather than an error. One line does it:
```suggestion
/// Returns the field ID stored in `field`'s `PARQUET:field_id` metadata, or
`None` if the key is absent or not a valid `i32`.
```
##########
crates/iceberg/src/arrow/reader/projection.rs:
##########
@@ -379,18 +372,22 @@ pub(super) fn build_fallback_field_id_map(
fn build_field_id_map_from_arrow_schema(arrow_schema: &ArrowSchemaRef) ->
HashMap<i32, usize> {
let mut column_map = HashMap::new();
arrow_schema.fields().filter_leaves(|idx, field| {
- if let Some(field_id) = field
- .metadata()
- .get(PARQUET_FIELD_ID_META_KEY)
- .and_then(|value| i32::from_str(value).ok())
- {
+ if let Some(field_id) = parquet_field_id(field) {
column_map.insert(field_id, idx);
}
false
});
column_map
}
+/// Returns the field ID in `field`'s `PARQUET:field_id` metadata, if it has a
valid one.
+fn parquet_field_id(field: &Field) -> Option<i32> {
Review Comment:
The whole equivalence rests on `None` → excluded in the second pass, and
that's the one branch the existing tests don't pin. A small unit test would
lock it: one leaf with a valid id, one with no metadata, one with `"abc"`,
asserting only the valid leaf lands in the mask. Not blocking — the happy path
is well covered — but it'd guard `build_field_id_map_from_arrow_schema` against
the same edge 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]
