laskoviymishka commented on code in PR #3629:
URL: https://github.com/apache/iceberg-rust/pull/3629#discussion_r4490537502
##########
crates/iceberg/src/arrow/record_batch_transformer.rs:
##########
@@ -1972,6 +2026,85 @@ mod test {
assert_eq!(out.as_list::<i32>(), &expected);
}
+ #[test]
+ fn promote_fixed_size_list_file_column_to_list() {
Review Comment:
Could we add one test that writes a Parquet file with a `FixedSizeList`
column and a null row, then reads it back through `ArrowReader`? All three
tests build the `FixedSizeListArray` by hand, so nothing exercises the #3471
scan itself or the parquet-rs [null-row
padding](https://github.com/apache/arrow-rs/blob/f90e061326bd821a7af09281d9e92de6f3b603d9/parquet/src/arrow/array_reader/fixed_size_list_array.rs#L166-L185)
the doc comment relies on. The `fixed_size_list_column` test in #3471 looks
close to reusable, ideally with one nullable and one required element.
##########
crates/iceberg/src/arrow/record_batch_transformer.rs:
##########
@@ -464,6 +483,30 @@ impl PromotePlan {
}
}
+/// parquet-rs pads each null FixedSizeList row with `size` null children;
drop them so a null
+/// list owns no child slots, as in a 3-level LIST and Java. Required elements
reject the pads.
+fn list_from_fixed_size_list(
+ fixed_size_list: &FixedSizeListArray,
+ element_field: &FieldRef,
+) -> Result<ArrayRef> {
+ let Some(nulls) = fixed_size_list.nulls().filter(|n| n.null_count() > 0)
else {
+ return Ok(cast(
+ fixed_size_list,
+ &DataType::List(Arc::clone(element_field)),
+ )?);
+ };
+ let size = fixed_size_list.value_length() as usize;
+ let offsets =
+ OffsetBuffer::<i32>::from_lengths(nulls.iter().map(|valid| if valid {
size } else { 0 }));
Review Comment:
Could we check the retained child count against `i32::MAX` before the `let
Some(nulls)` split and return an error instead? A batch past that panics in
either branch:
[`from_lengths`](https://github.com/apache/arrow-rs/blob/9ea30dfacb7003de9ffc87ef50231f18229fcdb5/arrow-buffer/src/buffer/offset.rs#L118-L137)
here, and
[`From<FixedSizeListArray>`](https://github.com/apache/arrow-rs/blob/9ea30dfacb7003de9ffc87ef50231f18229fcdb5/arrow-array/src/array/list_array.rs#L510-L526)
behind the `cast` path, both via `expect("offset overflow")`. On
[`main`](https://github.com/apache/iceberg-rust/blob/f533cd6175bb42b5788537f589cd9450a3054d27/crates/iceberg/src/arrow/record_batch_transformer.rs#L311-L316)
this column was a `cannot promote` error, and the `LargeList` arm goes through
[`cast_list`](https://github.com/apache/arrow-rs/blob/9ea30dfacb7003de9ffc87ef50231f18229fcdb5/arrow-cast/src/cast/list.rs#L319-L335),
which returns `ComputeError`. One check on `(len() - null_count()) *
value_length(
)` covers both branches.
--
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]
