alamb commented on code in PR #7419:
URL: https://github.com/apache/arrow-rs/pull/7419#discussion_r2073617536
##########
arrow-row/src/lib.rs:
##########
@@ -1330,9 +1335,18 @@ fn encode_column(
.skip(1)
.enumerate()
.for_each(|(idx, offset)| {
- let (row, sentinel) = match array.is_valid(idx) {
- true => (rows.row(idx), 0x01),
- false => (*null, null_sentinel),
+ let (row, sentinel) = if array.is_valid(idx) {
+ let row = if rows.num_rows() == 0 {
Review Comment:
I am worried about adding a new test on each row of the conversion as that
may slow down things significantly
I think we could check if the input array's size was greater than zero and
just ignore the offsets if not
```rust
let array = as_struct_array(column);
if array.len() == 0 {
return OK(())
}
```
Or something
##########
arrow-row/src/lib.rs:
##########
@@ -2539,4 +2553,17 @@ mod tests {
let rows = converter.convert_columns(&[Arc::new(a) as _]).unwrap();
assert_eq!(rows.row(0).cmp(&rows.row(1)), Ordering::Less);
}
+
+ #[test]
+ fn test_empty_struct() {
+ let s = Arc::new(StructArray::new_empty_fields(5, None)) as ArrayRef;
+
+ let sort_fields = vec![SortField::new(s.data_type().clone())];
+ let converter = RowConverter::new(sort_fields).unwrap();
+ let r = converter.convert_columns(&[Arc::clone(&s)]).unwrap();
+
+ let back = converter.convert_rows(&r).unwrap();
+ assert_eq!(back.len(), 1);
+ assert_eq!(&back[0], &s);
+ }
Review Comment:
Can you also verify we have tests covering empty arrays of other offset
based types (such as ListArray, StringArray, etc) ?
--
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]
