Copilot commented on code in PR #3624:
URL: https://github.com/apache/iceberg-rust/pull/3624#discussion_r4464365230
##########
crates/iceberg/src/arrow/reader/projection.rs:
##########
@@ -293,10 +265,9 @@ pub(super) fn build_field_id_map(
let field_type = field.self_type();
match field_type {
ParquetType::PrimitiveType { basic_info, .. } => {
- if !basic_info.has_id() {
- return Ok(None);
+ if basic_info.has_id() {
+ column_map.insert(basic_info.id(), idx);
}
Review Comment:
This preserves embedded IDs only when the caller reaches the field-ID
projection path, but `pipeline.rs` still sets `use_position_fallback` from only
the first top-level Arrow field. For a partially annotated file whose first
leaf is id-less and a later leaf has an embedded ID, the caller takes the
positional fallback and this map is ignored, so a request for the later field
is read as missing or from the wrong root. Please base the fallback decision on
the complete Parquet schema (or otherwise use the embedded map whenever any
valid IDs exist) so the new mixed-ID behavior is effective.
##########
crates/iceberg/src/arrow/reader/pipeline.rs:
##########
@@ -305,14 +305,14 @@ impl FileScanTaskReader {
// Create projection mask based on field IDs
// - If file has embedded IDs: field-ID-based projection
- // - If name mapping applied: field-ID-based projection using the IDs
the name
- // mapping assigned to the Arrow schema
+ // - If name mapping applied: field-ID-based projection using mapped
Parquet names
// - Otherwise: position-based fallback projection
let mut projection_mask = ArrowReader::get_arrow_projection_mask(
&project_field_ids_without_metadata,
task.schema(),
record_batch_stream_builder.parquet_schema(),
record_batch_stream_builder.schema(),
+ task.name_mapping().map(Arc::as_ref),
Review Comment:
This still lets `use_position_fallback` override the Parquet field-ID
mapping for a partially annotated file. The flag is derived from whether the
first top-level field has an ID, so an id-less top-level field before an
ID-bearing field causes this call to take the position-based branch; the
requested ID is then interpreted as `root_index = id - 1` and can read the
wrong leaf or be dropped. Determine the fallback only when the file has no
usable embedded IDs at all (and handle unmapped leaves as missing), so the new
Parquet leaf map is used whenever valid IDs are present.
##########
crates/iceberg/src/arrow/reader/projection.rs:
##########
@@ -367,26 +338,119 @@ pub(super) fn build_fallback_field_id_map(
column_map
}
-/// Builds a mapping from field IDs to leaf column indices using the field-id
metadata
-/// carried by the Arrow schema.
-///
-/// Used for Parquet files without embedded field IDs when a name mapping has
assigned
-/// IDs to the Arrow schema (see [`apply_name_mapping_to_arrow_schema`]): the
Parquet
-/// schema descriptor itself still has no IDs, but the Arrow leaves are
flattened in the
-/// same depth-first order as Parquet leaf columns, so the Arrow leaf index
lines up with
-/// the Parquet column index. Columns the mapping did not match carry no
field-id
-/// metadata and are simply absent from the 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) = parquet_field_id(field) {
- column_map.insert(field_id, idx);
+fn build_field_id_map_from_name_mapping(
+ parquet_schema: &SchemaDescriptor,
+ name_mapping: &NameMapping,
+) -> HashMap<i32, usize> {
+ fn visit(
+ ty: &ParquetType,
+ mapped: Option<&MappedField>,
+ list_element: bool,
+ leaf_index: &mut usize,
+ column_map: &mut HashMap<i32, usize>,
+ ) {
+ let Some(mapped) = mapped.filter(|field| field.field_id().is_some())
else {
+ *leaf_index += leaf_count(ty);
+ return;
+ };
+ let child_mapping = |name|
find_mapped_field(mapped.fields().iter().map(Arc::as_ref), name);
+ if !list_element && ty.get_basic_info().repetition() ==
Repetition::REPEATED {
+ visit(
Review Comment:
Legacy two-level maps are also represented by a repeated group, but this
guard classifies every repeated group as a list before the `MAP_KEY_VALUE` arm
is reached. For a `repeated group ... (MAP_KEY_VALUE) { key; value; }`,
`child_mapping(LIST_FIELD_NAME)` normally finds nothing, so both map leaves are
skipped without advancing `leaf_index`; mapped IDs after the map then resolve
to the wrong Parquet columns (and map predicates cannot resolve). Handle
`MAP_KEY_VALUE` before the repeated-list case and walk the group's key/value
children directly.
--
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]
