alamb commented on code in PR #9717:
URL: https://github.com/apache/arrow-rs/pull/9717#discussion_r3111121846
##########
arrow-ipc/src/writer.rs:
##########
@@ -1736,10 +1738,9 @@ fn get_byte_array_buffers<O: OffsetSizeTrait>(data:
&ArrayData) -> (Buffer, Buff
/// of a values buffer.
fn get_list_array_buffers<O: OffsetSizeTrait>(data: &ArrayData) -> (Buffer,
ArrayData) {
if data.is_empty() {
- return (
- MutableBuffer::new(0).into(),
- data.child_data()[0].slice(0, 0),
- );
+ let mut offsets = MutableBuffer::new(size_of::<O>());
Review Comment:
Likewise here a comment for future readers explaining what this is doing
would be helpful
##########
arrow-ipc/src/writer.rs:
##########
@@ -1724,7 +1724,9 @@ fn reencode_offsets<O: OffsetSizeTrait>(
/// size of sliced arrays, as values that have been sliced away are not encoded
fn get_byte_array_buffers<O: OffsetSizeTrait>(data: &ArrayData) -> (Buffer,
Buffer) {
if data.is_empty() {
- return (MutableBuffer::new(0).into(), MutableBuffer::new(0).into());
+ let mut offsets = MutableBuffer::new(size_of::<O>());
Review Comment:
Can we please add a comment to this code explaining what it is doing (namely
ensuring that the offsets have N + 1 elements)?
##########
arrow-ipc/src/writer.rs:
##########
@@ -2372,6 +2373,62 @@ mod tests {
}
}
+ #[test]
+ fn test_empty_utf8_ipc_writes_nonempty_offsets_buffer() {
+ let name = StringArray::from(Vec::<String>::new());
+ let (offsets, values) = get_byte_array_buffers::<i32>(&name.to_data());
+
+ assert_eq!(name.len(), 0);
+ assert_eq!(
+ offsets.len(),
+ std::mem::size_of::<i32>(),
+ "offsets buffer should contain one zero i32 offset"
+ );
+ assert_eq!(values.len(), 0, "values buffer should remain empty");
+ }
+
+ #[test]
+ fn test_empty_large_utf8_ipc_writes_nonempty_offsets_buffer() {
+ let name = LargeStringArray::from(Vec::<String>::new());
+ let (offsets, values) = get_byte_array_buffers::<i64>(&name.to_data());
+
+ assert_eq!(name.len(), 0);
+ assert_eq!(
+ offsets.len(),
+ std::mem::size_of::<i64>(),
+ "offsets buffer should contain one zero i64 offset"
+ );
+ assert_eq!(values.len(), 0, "values buffer should remain empty");
+ }
+
+ #[test]
+ fn test_empty_list_ipc_writes_nonempty_offsets_buffer() {
+ let list = GenericListBuilder::<i32,
_>::new(UInt32Builder::new()).finish();
+ let (offsets, child_data) =
get_list_array_buffers::<i32>(&list.to_data());
+
+ assert_eq!(list.len(), 0);
+ assert_eq!(
+ offsets.len(),
+ std::mem::size_of::<i32>(),
+ "offsets buffer should contain one zero i32 offset"
+ );
+ assert_eq!(child_data.len(), 0, "child data should remain empty");
+ }
+
+ #[test]
Review Comment:
Can we please also add a test that shows IPC files without any offets are
still accepted?
--
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]
