You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "HawaiianSpork (via GitHub)" <gi...@apache.org> on 2025/01/20 03:35:30 UTC
[PR] fix: Panic on reencoding offsets [arrow-rs]
HawaiianSpork opened a new pull request, #6998:
URL: https://github.com/apache/arrow-rs/pull/6998
Code was incorrectly defining the end of the offset slice to be the start + slice_length * 2 because slice_with_length adds the start to the end. This caused the encoded batches to be larger than they needed to be and would result in a panic for certain slices.
# Which issue does this PR close?
Closes #6997 .
# Rationale for this change
Prevent code panic during IPC write for legitimate input.
# What changes are included in this PR?
Corrects the slice length and adds unit tests that previously would cause a panic.
# Are there any user-facing changes?
No
--
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] fix: Panic on reencoding offsets [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #6998:
URL: https://github.com/apache/arrow-rs/pull/6998#issuecomment-2607404556
BTW I think this issue is important enough to hold the release for so I will be keeping an eye on it / trying to help out. I added it to the list on
- https://github.com/apache/arrow-rs/issues/6929
--
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] fix: Panic on reencoding offsets [arrow-rs]
Posted by "HawaiianSpork (via GitHub)" <gi...@apache.org>.
HawaiianSpork commented on code in PR #6998:
URL: https://github.com/apache/arrow-rs/pull/6998#discussion_r1924581179
##########
arrow-ipc/src/writer.rs:
##########
@@ -2615,6 +2615,35 @@ mod tests {
builder.finish()
}
+ #[test]
+ fn reencode_offsets_when_first_offset_is_not_zero() {
+ let original_list = generate_list_data::<i32>();
+ let original_data = original_list.into_data();
+ let slice_data = original_data.slice(75, 7);
+ let (new_offsets, original_start, length) = reencode_offsets::<i32>(&slice_data.buffers()[0], &slice_data);
Review Comment:
Thanks for doing that. I initially had written larger integration tests but found it difficult to recreate the scenario. I can try again tomorrow.
--
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] fix: Panic on reencoding offsets [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #6998:
URL: https://github.com/apache/arrow-rs/pull/6998#discussion_r1924188653
##########
arrow-ipc/src/writer.rs:
##########
@@ -2615,6 +2615,35 @@ mod tests {
builder.finish()
}
+ #[test]
+ fn reencode_offsets_when_first_offset_is_not_zero() {
+ let original_list = generate_list_data::<i32>();
+ let original_data = original_list.into_data();
+ let slice_data = original_data.slice(75, 7);
+ let (new_offsets, original_start, length) = reencode_offsets::<i32>(&slice_data.buffers()[0], &slice_data);
Review Comment:
I tried to write some tests here: https://github.com/apache/arrow-rs/pull/7004
--
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] fix: Panic on reencoding offsets in arrow-ipc with sliced nested arrays [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #6998:
URL: https://github.com/apache/arrow-rs/pull/6998
--
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] fix: Panic on reencoding offsets [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #6998:
URL: https://github.com/apache/arrow-rs/pull/6998#discussion_r1927214791
##########
arrow-ipc/src/writer.rs:
##########
@@ -2433,6 +2430,126 @@ mod tests {
);
}
+ #[test]
+ fn test_large_slice_uint32() {
+ ensure_roundtrip(Arc::new(UInt32Array::from_iter((0..8000).map(|i| {
+ if i % 2 == 0 {
+ Some(i)
+ } else {
+ None
+ }
+ }))));
+ }
+
+ #[test]
+ fn test_large_slice_string() {
+ let strings: Vec<_> = (0..8000)
+ .map(|i| {
+ if i % 2 == 0 {
+ Some(format!("value{}", i))
+ } else {
+ None
+ }
+ })
+ .collect();
+
+ ensure_roundtrip(Arc::new(StringArray::from(strings)));
+ }
+
+ #[test]
+ fn test_large_slice_string_list() {
+ let mut ls = ListBuilder::new(StringBuilder::new());
+
+ let mut s = String::new();
+ for row_number in 0..8000 {
+ if row_number % 2 == 0 {
+ for list_element in 0..1000 {
+ s.clear();
+ use std::fmt::Write;
+ write!(&mut s, "value{row_number}-{list_element}").unwrap();
+ ls.values().append_value(&s);
+ }
+ ls.append(true)
+ } else {
+ ls.append(false); // null
+ }
+ }
+
+ ensure_roundtrip(Arc::new(ls.finish()));
+ }
+
+ #[test]
+ fn test_large_slice_string_list_of_lists() {
+ // The reason for the special test is to verify reencode_offsets which looks both at
+ // the starting offset and the data offset. So need a dataset where the starting_offset
+ // is zero but the data offset is not.
Review Comment:
👍 Ah, Lists of Lists 🤯
I verified this fails without the tests in this PR like this:
```
the offset of the new Buffer cannot exceed the existing length: slice offset=4 length=24004 selflen=24004
thread 'writer::tests::test_large_slice_string_list_of_lists' panicked at arrow-buffer/src/buffer/immutable.rs:281:9:
the offset of the new Buffer cannot exceed the existing length: slice offset=4 length=24004 selflen=24004
```
💯
--
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] fix: Panic on reencoding offsets [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #6998:
URL: https://github.com/apache/arrow-rs/pull/6998#issuecomment-2602380703
Looks like `cargo fmt` needs to be run to get CI to pass:
* https://github.com/apache/arrow-rs/actions/runs/12860519755/job/35852539902?pr=6998
--
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] fix: Panic on reencoding offsets [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #6998:
URL: https://github.com/apache/arrow-rs/pull/6998#discussion_r1924188653
##########
arrow-ipc/src/writer.rs:
##########
@@ -2615,6 +2615,35 @@ mod tests {
builder.finish()
}
+ #[test]
+ fn reencode_offsets_when_first_offset_is_not_zero() {
+ let original_list = generate_list_data::<i32>();
+ let original_data = original_list.into_data();
+ let slice_data = original_data.slice(75, 7);
+ let (new_offsets, original_start, length) = reencode_offsets::<i32>(&slice_data.buffers()[0], &slice_data);
Review Comment:
- I tried to write some tests here: https://github.com/apache/arrow-rs/pull/7004 but was not successful reproducing the issue
--
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] fix: Panic on reencoding offsets [arrow-rs]
Posted by "HawaiianSpork (via GitHub)" <gi...@apache.org>.
HawaiianSpork commented on code in PR #6998:
URL: https://github.com/apache/arrow-rs/pull/6998#discussion_r1926922172
##########
arrow-ipc/src/writer.rs:
##########
@@ -2615,6 +2615,35 @@ mod tests {
builder.finish()
}
+ #[test]
+ fn reencode_offsets_when_first_offset_is_not_zero() {
+ let original_list = generate_list_data::<i32>();
+ let original_data = original_list.into_data();
+ let slice_data = original_data.slice(75, 7);
+ let (new_offsets, original_start, length) = reencode_offsets::<i32>(&slice_data.buffers()[0], &slice_data);
Review Comment:
Thanks for the test code in #7004 . I merged it in (I hope you don't mind) and then added an end to end test that failed before the fix but passed afterwards.
--
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] fix: Panic on reencoding offsets in arrow-ipc with sliced nested arrays [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #6998:
URL: https://github.com/apache/arrow-rs/pull/6998#issuecomment-2613987101
Thanks again @HawaiianSpork
--
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