You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "atwam (via GitHub)" <gi...@apache.org> on 2026/04/14 16:32:24 UTC
[PR] arrow-ipc: Ensure writer conforms to spec on length-0 variable-size arrays [arrow-rs]
atwam opened a new pull request, #9717:
URL: https://github.com/apache/arrow-rs/pull/9717
# Which issue does this PR close?
- Closes #9716 .
# Rationale for this change
Current version serializes a length-0 offsets buffer, and relies on the array constructor to set offsets to `[0]` for empty arrays. This does not conform with spec, which specifies that the offsets buffer should have `length + 1` elements.
This correctly serializes a length-1 offsets buffer containing `[0]`. I have left the current behavior of filling-in offsets with `[0]` for empty arrays, so that future versions can still read IPC files serialized by previous versions.
# What changes are included in this PR?
Added tests for serialization of empty arrays, ensuring length-1 offsets buffer. Fixed serialization of empty arrays.
This fixes serialization of empty Binary/Utf8/List/LargeList (and Map) arrays.
# Are these changes tested?
Yes
# Are there any user-facing changes?
There should not be any breaking change for users. Serialized files for empty record batches will be slightly different. Older serialized files should still be read fine.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] arrow-ipc: Ensure writer conforms to spec on length-0 variable-size arrays [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #9717:
URL: https://github.com/apache/arrow-rs/pull/9717#issuecomment-4247002504
> Current version serializes a length-0 offsets buffer, and relies on the array constructor to set offsets to [0] for empty arrays. This does not conform with spec, which specifies that the offsets buffer should have length + 1 elements.
I don't understand this statement. The spec you reference in https://github.com/apache/arrow-rs/issues/9716 is for the in memory structure (not the ipc format)
It almost sounds like a bug in polars -- shouldn't it accept zero length IPC buffers ?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] arrow-ipc: Write 0 offset buffer for length-0 variable-size arrays [arrow-rs]
Posted by "atwam (via GitHub)" <gi...@apache.org>.
atwam commented on code in PR #9717:
URL: https://github.com/apache/arrow-rs/pull/9717#discussion_r3116009372
##########
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:
Added two tests for legacy files.
Creating files with the legacy format means I could not rely on the writer (unless I added options to enable some legacy format, which was ugly), so I just went back to raw flatbuffers for the purpose of these tests.
Another alternative would be to generate such files once with the current version and embed some raw files, but that amounts to embedding small binary files, which I found equally repulsive.
I hope my approach will do.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] arrow-ipc: Write 0 offset buffer for length-0 variable-size arrays [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] arrow-ipc: Write 0 offset buffer for length-0 variable-size arrays [arrow-rs]
Posted by "atwam (via GitHub)" <gi...@apache.org>.
atwam commented on code in PR #9717:
URL: https://github.com/apache/arrow-rs/pull/9717#discussion_r3115998044
##########
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:
Good point, added.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] arrow-ipc: Write 0 offset buffer for length-0 variable-size arrays [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #9717:
URL: https://github.com/apache/arrow-rs/pull/9717#issuecomment-4281282111
For anyone else following this PR, please see discussion on the issue.
- https://github.com/apache/arrow-rs/issues/9716#issuecomment-4273961244
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] arrow-ipc: Write 0 offset buffer for length-0 variable-size arrays [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #9717:
URL: https://github.com/apache/arrow-rs/pull/9717#issuecomment-4296646994
Thanks again @atwam
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] arrow-ipc: Write 0 offset buffer for length-0 variable-size arrays [arrow-rs]
Posted by "atwam (via GitHub)" <gi...@apache.org>.
atwam commented on code in PR #9717:
URL: https://github.com/apache/arrow-rs/pull/9717#discussion_r3116009372
##########
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:
Added two tests for legacy files. Creating files with the legacy format means I could not rely on the writer (unless I added options to enable some legacy format, which was ugly), so I just went back to raw flatbuffers for the purpose of these tests. I hope that will do. Another alternative would be to generate such files once with the current version and embed some raw files, but that amounts to embedding small binary files, which I found equally repulsive.
I hope my approach will do.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] arrow-ipc: Write 0 offset buffer for length-0 variable-size arrays [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #9717:
URL: https://github.com/apache/arrow-rs/pull/9717#discussion_r3118199916
##########
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:
I think what you have is fine -- if it becomes too annoying to maintain we can make a checked in file
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] arrow-ipc: Write 0 offset buffer for length-0 variable-size arrays [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #9717:
URL: https://github.com/apache/arrow-rs/pull/9717
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org