laskoviymishka commented on code in PR #3467:
URL: https://github.com/apache/iceberg-rust/pull/3467#discussion_r4323674395
##########
crates/iceberg/src/arrow/reader/pipeline.rs:
##########
@@ -655,25 +655,24 @@ impl FileScanTaskReader {
// otherwise, use applyNameMapping() followed by pruneColumns() when
configured, or
// addFallbackIds() followed by pruneColumnsFallback() for
position-based fallback.
// The fast path returns early without materializing an owned schema.
+ let without_list_views = coerce_list_views(arrow_metadata.schema());
Review Comment:
The comment block above only covers the field-ID strategy, but this step now
runs before all of it and changes the emitted type. I'd call that out here:
```suggestion
// Rewrite list views to lists first: `filter_leaves` treats them as
leaves (#3393),
// so everything below, including the emitted batches, sees
`List`/`LargeList`.
let without_list_views = coerce_list_views(arrow_metadata.schema());
```
That also explains the `without_list_views.is_some()` arm further down.
Callers who wrote `ListView` now get `List` back. That's the right contract for
an Iceberg `list`, but it's worth a line in the PR description too.
##########
crates/iceberg/src/arrow/reader/projection.rs:
##########
@@ -499,8 +499,7 @@ fn apply_name_mapping_to_field(
.with_data_type(DataType::Struct(entry_fields));
DataType::Map(Arc::new(entries), *sorted)
}
- // `Fields::filter_leaves` treats `ListView` and `LargeListView` as
leaves, so projection
- // cannot select their elements even with IDs.
+ // List views were already rewritten to lists by `coerce_list_views`.
Review Comment:
This comment is now a precondition on every caller of
`apply_name_mapping_to_arrow_schema`, but the `other => other.clone()` arm
still lets a list view through silently. I'd state the precondition in that
function's doc and add `debug_assert!(!matches!(other, DataType::ListView(_) |
DataType::LargeListView(_)))` here. Then a new call site that skips
`coerce_list_views` fails in tests instead of quietly bringing back the
all-NULL projection.
##########
crates/iceberg/src/arrow/reader/pipeline.rs:
##########
@@ -655,25 +655,24 @@ impl FileScanTaskReader {
// otherwise, use applyNameMapping() followed by pruneColumns() when
configured, or
// addFallbackIds() followed by pruneColumnsFallback() for
position-based fallback.
// The fast path returns early without materializing an owned schema.
+ let without_list_views = coerce_list_views(arrow_metadata.schema());
+ let file_schema = without_list_views
+ .as_ref()
+ .unwrap_or(arrow_metadata.schema());
let arrow_schema = if missing_field_ids {
let schema = if let Some(name_mapping) = task.name_mapping() {
- apply_name_mapping_to_arrow_schema(
- Arc::clone(arrow_metadata.schema()),
- name_mapping,
- )?
+ apply_name_mapping_to_arrow_schema(Arc::clone(file_schema),
name_mapping)?
} else {
- add_fallback_field_ids_to_arrow_schema(arrow_metadata.schema())
+ add_fallback_field_ids_to_arrow_schema(file_schema)
Review Comment:
This is the position-fallback counterpart to @ewoodbury's name-mapping note.
The test file carries field IDs, so nothing reaches this line. Fallback IDs are
assigned by leaf position, and leaf position is exactly what the list-view bug
threw off. Writing the batch without IDs and reading it twice, with and without
a name mapping, would pin both branches in one test. Making `lv` a
`LargeListView` in one of those runs would also cover that arm end to end. Fine
as a follow-up.
##########
crates/iceberg/src/arrow/reader/projection.rs:
##########
@@ -1393,6 +1450,64 @@ message schema {
);
}
+ /// List views become lists at any depth and keep their metadata, and a
schema without one
+ /// is left as it is.
+ #[test]
+ fn test_coerce_list_views() {
+ use arrow_schema::{FieldRef, Fields};
+
+ use super::coerce_list_views;
+
+ // The same schema, built with list views or with lists.
+ let schema = |list: fn(FieldRef) -> DataType, large_list: fn(FieldRef)
-> DataType| {
+ let id = HashMap::from([(PARQUET_FIELD_ID_META_KEY.to_string(),
"9".to_string())]);
+ let int = |name: &str| Field::new(name, DataType::Int32, true);
+ let element = |data_type| Arc::new(Field::new("element",
data_type, true));
+ let ints = element(DataType::Int32);
Review Comment:
Only `nested` carries an ID, so this test would still pass if the rewrite
dropped metadata on a list element or on the fields inside a struct. I'd give
`ints` and the fields inside `s` IDs too, since projection selects by exactly
those fields.
--
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]
