Repository navigation
Conversation
…leaves Fields::filter_leaves, which builds the projection mask, treats ListView and LargeListView as leaves. A list view column therefore read as NULL next to another column, and a list view of structs shifted the leaf index of every later column, so those columns were read from the wrong Parquet leaf. Rewrite list views to List and LargeList in the schema hint passed to parquet-rs, before name mapping and INT96 coercion. Closes apache#3393
- Rewrite list views in one pass that returns None for a schema without any, and drop contains_list_view. - Rename list_views_as_lists to coerce_list_views, after coerce_int96_timestamps, and merge the pipeline branches that both return the file schema. - Move the reader test next to the INT96 reader tests to reuse metadata_projection_task and scan_task, and cover every arm of the rewrite in the schema test.
There was a problem hiding this comment.
🟢 Approval recommended
The focused change preserves metadata and fits existing reader handling, with no blocking issues identified; tests were not executed.
0 open findings
What changed in this PR
Fixes #3393 by normalizing Arrow list views before projection, preventing missing list values and incorrect selection of later columns.
Changes:
- Recursively converts list views to lists while preserving metadata.
- Applies normalization before field-ID handling and timestamp coercion.
- Adds schema-conversion and reader regression tests.
| File | Description |
|---|---|
crates/iceberg/src/arrow/reader/projection.rs |
Adds recursive normalization and unit coverage. |
crates/iceberg/src/arrow/reader/pipeline.rs |
Integrates normalization and adds a reader regression test. |
crates/iceberg/src/arrow/reader/mod.rs |
Makes the helper available within the reader module. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
ewoodbury
left a comment
ewoodbury
left a comment
There was a problem hiding this comment.
Chiming in with a short review, fix looks good. I checked the main correctness claims locally against a8df712, and they all line up:
- On main, projecting only id after a list_view<struct<a, b>> returns all NULLs, and projecting both columns errors. Both read correctly here.
- test_read_list_view_columns fails with the rewrite disabled. Reader tests, clippy, fmt and rustdoc are clean, and the branch merges cleanly with main.
- These also read correctly here (all broken on main): LargeListView, a row filter on the column after the list view, and files without field IDs through name mapping (including a pyarrow-written file) and through position fallback.
- iceberg-java 1.12.0 reads the same file identically.
- The INT96 claim holds: main reads 0 for a 3333-01-01 value next to a list view, and this branch reads it correctly.
Rewriting once in the schema hint is the right approach since every later step sees a plain List, with no public API change.
As a nit, I left one test suggestion inline regarding the name mapping branch for imported files, just to pin that fix.
| Arc::clone(arrow_metadata.schema()), | ||
| name_mapping, | ||
| )? | ||
| apply_name_mapping_to_arrow_schema(Arc::clone(file_schema), name_mapping)? |
There was a problem hiding this comment.
Imported files are probably the most common source of list views. iceberg-rust's writers emit List, and a pyarrow pa.list_view(...) file has no field IDs, so it is read through this name-mapping branch.
This branch doesn't seem to be covered by the new tests yet. If I pass the uncoerced arrow_metadata.schema() here, both tests still pass. Meanwhile, a pyarrow-written lv list_view<struct<a, b>>, id file read with a name mapping goes back to returning id as all NULLs, and to erroring when lv is also projected.
I think it would be worth having test_read_list_view_columns also read the file through a name mapping, for example by writing the same batch without the field-ID metadata and setting with_name_mapping on the task. Not blocking.
|
Thanks for the PR! I'm not sure if this should be fixed in the iceberg code, it seems like a bug in arrow-rs related to im a bit worried about the maintenance burden of working around it here, since we'd be patching arrow types one at a time ( What do you think about picking up the fix from upstream arrow-rs/parquet-rs instead, and keeping #3393 open until then? 😄 |
laskoviymishka
left a comment
laskoviymishka
left a comment
There was a problem hiding this comment.
Approving. Fixing this on the ARROW:schema hint, before the ID branches, means INT96 and the row-number install get the fix for free. Since Parquet has no list-view type, the rewrite produces the same List shape schema_to_arrow already gives us.
The inline comments are follow-ups, not blockers. +1 to @ewoodbury's name-mapping test, and I'd pin position fallback the same way, since leaf-position ID assignment is exactly what this bug broke. The other two are small hardening. Landing them here or in a follow-up PR are both fine with me.
I'll leave this open for a day or two in case anyone else wants a look.
| // 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()); |
There was a problem hiding this 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:
| let without_list_views = coerce_list_views(arrow_metadata.schema()); | |
| // 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.
| 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) |
There was a problem hiding this 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.
| } | ||
| // `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`. |
There was a problem hiding this 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.
| 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); |
There was a problem hiding this 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.
Agreed, especially since I think it'll make the 60.1.0 maintenance release this week. apache/arrow-rs#10525 |
|
okie, lets wait for 60.1.0 and btw we already have Arrow 60 migration opened in #3257 |
Which issue does this PR close?
What changes are included in this PR?
parquet-rs reads a Parquet LIST as
ListVieworLargeListViewwhen the file's embedded Arrow schema says so. The projection mask is built with arrow-schema'sFields::filter_leaves, which treats list views as leaves. So a list view column read as NULL whenever another column was projected, and a list view of structs shifted the leaf index of every later column, which was then read from the wrong Parquet leaf.configure_arrow_reader_metadatanow callscoerce_list_views, which rewrites everyListViewandLargeListViewtoListandLargeListin the schema hint passed to parquet-rs, at any depth and keeping field metadata. Name mapping, fallback IDs, and INT96 coercion then start from that schema. The rewrite is a single pass, and for a schema without list views it returnsNonewithout allocating. The transformer then handles these columns as it already handlesListandLargeList.This also re-enables INT96 coercion for files that contain a list view.
ArrowSchemaVisitor::visit_typehas no list view arm, socoerce_int96_timestampsused to fail on such a schema and silently skip the coercion. That follows from reading the code, and I did not run it.Iceberg Java, Spark, and DuckDB select columns on the Parquet schema, so they do not hit this. DataFusion keeps list views and counts their leaves with its own walker (
count_leavesinnested_schema_pruning.rs). arrow-rsmain(60.0.0) still treats list views as leaves infilter_leaves.Are these changes tested?
Yes.
test_read_list_view_columnsinpipeline.rsreads the file from ListView columns read as NULL and shift later columns to the wrong Parquet leaf #3393, alist_view<struct<a, b>>followed by anint, and checks both columns. Onmainthat read fails withcannot promote ListView(...) to List(...), and projecting only theintreturns NULL.test_coerce_list_viewsinprojection.rschecks the rewrite at every nesting position, with unchanged fields before and after a list view, and that a schema without list views is left alone.As a spot check, I broke the change in 15 ways, one at a time, from skipping the rewrite in the pipeline to dropping the fields around the first list view. Each mutation failed at least one of the two tests.
AI Disclosure
Drafted with Claude Code. The behavior on
mainwas checked againstmainatb3470c846.