mbutrovich commented on code in PR #3629:
URL: https://github.com/apache/iceberg-rust/pull/3629#discussion_r4496322197
##########
crates/iceberg/src/arrow/record_batch_transformer.rs:
##########
@@ -254,10 +256,12 @@ impl PromotePlan {
fields: target_fields.clone(),
})
}
- // A file's Arrow schema hint can give either offset width;
`apply` converts it.
- // Table schemas are always `List`
(`ToArrowSchemaConverter::list`).
+ // File Arrow hints may be List, LargeList, or FixedSizeList.
Iceberg has no
+ // fixed-size list; tables are always List
(`ToArrowSchemaConverter::list`).
(
- DataType::List(source_field) |
DataType::LargeList(source_field),
+ DataType::List(source_field)
+ | DataType::LargeList(source_field)
+ | DataType::FixedSizeList(source_field, _),
Review Comment:
@laskoviymishka asked whether `PromotePlan` should accept `FixedSizeList` at
all. I don't think it should. Could we rewrite `FixedSizeList` to `List` in the
Arrow schema hint instead, the way #3467 does for list views, and drop the
`PromotePlan` arm and `list_from_fixed_size_list`?
parquet-rs takes the list type from the hint, so a `List` hint reads the
same Parquet `LIST` through its list reader. I prototyped this as a rewrite at
the top of
[`configure_arrow_reader_metadata`](https://github.com/apache/iceberg-rust/blob/59946648ef56dcd5b70d46cc7ef90b25a00ed9fb/crates/iceberg/src/arrow/reader/pipeline.rs#L676-L713),
before name mapping and fallback IDs are applied, which is where #3467 puts
`coerce_list_views`. I replaced the new `FixedSizeList` arm in `apply` with a
panic, and `test_read_fixed_size_list_column_as_list` still passes. A
pyarrow-written file without field IDs also reads correctly through both a name
mapping and fallback IDs, including `FixedSizeList` columns nested in a struct
and as a map value. So the transformer never sees a `FixedSizeList`.
That removes three things this PR has to carry. For a batch with a null row,
parquet-rs copies the decoded values into a padded child
([`fixed_size_list_array.rs`](https://github.com/apache/arrow-rs/blob/f90e061326bd821a7af09281d9e92de6f3b603d9/parquet/src/arrow/array_reader/fixed_size_list_array.rs#L164-L185)),
and
[`list_from_fixed_size_list`](https://github.com/apache/iceberg-rust/blob/6bbf39af7946d11be169fddfe7e87131c3dba865/crates/iceberg/src/arrow/record_batch_transformer.rs#L486-L521)
then copies them again with `filter` to drop the pads. With the hint, the list
reader uses the decoded values without either copy. The PR would also stop
depending on parquet-rs padding null rows, which the doc comment and the filter
assume. Finally, the overflow guard isn't needed, because the list reader
already returns an error when the child length doesn't fit `i32` offsets
([`list_array.rs`](https://github.com/apache/arrow-rs/blob/f90e061326bd821a7af09281d9e92de6f3b603d9/parquet/src/arrow
/array_reader/list_array.rs#L119-L124)).
It also keeps the normalization of file-side Arrow list types in one place.
`coerce_list_views` in #3467 already recurses through `FixedSizeList`, so this
becomes one changed arm there, `DataType::FixedSizeList(element, _) =>
DataType::List(coerce_list_views_or_share(element))`, plus a rename. Could we
build on #3467 here?
##########
crates/iceberg/src/arrow/reader/projection.rs:
##########
@@ -2482,4 +2482,162 @@ message schema {
.unwrap();
assert_eq!(result, vec![expected]);
}
+
+ /// parquet-rs can return a LIST as `FixedSizeList` when the file's
embedded Arrow schema
+ /// says so. Iceberg has no such type; the table column is a plain list.
Also covers a null
+ /// parent (parquet-rs pads `size` children) with both optional and
required elements.
+ #[tokio::test]
+ async fn test_read_fixed_size_list_column_as_list() {
Review Comment:
Could this test also cover files without field IDs? Those are where these
columns come from in practice. Iceberg's own writers don't produce them:
iceberg-rust's `ParquetWriter` builds its Arrow schema from the Iceberg schema
([`parquet_writer.rs`](https://github.com/apache/iceberg-rust/blob/59946648ef56dcd5b70d46cc7ef90b25a00ed9fb/crates/iceberg/src/writer/file_writer/parquet_writer.rs#L751-L758)),
and PyIceberg's `append` casts a `FixedSizeListArray` to a plain list before
writing
([`pyarrow.py`](https://github.com/apache/iceberg-python/blob/e3564a54c10960307147377c578680276865ed18/pyiceberg/io/pyarrow.py#L2157-L2166)).
They get in when existing files are added without a rewrite. PyIceberg's
`add_files` reads the file's Arrow schema, which comes from `ARROW:schema`
([`pyarrow.py`](https://github.com/apache/iceberg-python/blob/e3564a54c10960307147377c578680276865ed18/pyiceberg/io/pyarrow.py#L2983-L2991)),
accepts `FixedSizeListType` as a list ([`pyarrow.py`](https://github.com/ap
ache/iceberg-python/blob/e3564a54c10960307147377c578680276865ed18/pyiceberg/io/pyarrow.py#L1276-L1282)),
and sets a default name mapping when the table has none
([`__init__.py`](https://github.com/apache/iceberg-python/blob/e3564a54c10960307147377c578680276865ed18/pyiceberg/table/__init__.py#L1053-L1055)).
A pyarrow-written file has no field IDs, so the reader resolves it through
that name mapping.
This test writes field IDs at every level, so it only covers the less common
shape. These cases would cover the import path:
1. A file without field IDs, read through a name mapping, with a
`FixedSizeList` column at the top level, inside a struct, as a map value, as a
list element, and as the element of another `FixedSizeList`. Give each a null
row, and give the top-level column a required element.
2. The same file read without a name mapping, so the reader assigns fallback
IDs to the top-level columns.
`ArrowWriter` writes this shape when the Arrow fields carry no
`PARQUET:field_id` metadata, as in
`test_read_parquet_with_name_mapping_maps_nested_fields`, so the test doesn't
need pyarrow. At this PR's head, the top-level, struct, and map cases read
correctly through both paths in a local test with a pyarrow 25 file. These
tests would guard whichever approach lands.
--
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]
