alamb commented on code in PR #6368:
URL: https://github.com/apache/arrow-rs/pull/6368#discussion_r1754048651


##########
arrow-buffer/src/buffer/immutable.rs:
##########
@@ -203,7 +203,9 @@ impl Buffer {
     pub fn advance(&mut self, offset: usize) {
         assert!(
             offset <= self.length,
-            "the offset of the new Buffer cannot exceed the existing length"
+            "the offset of the new Buffer cannot exceed the existing length: 
offset={} length={}",

Review Comment:
   👍 



##########
arrow/tests/pyarrow.rs:
##########
@@ -27,7 +28,9 @@ fn test_to_pyarrow() {
 
     let a: ArrayRef = Arc::new(Int32Array::from(vec![1, 2]));
     let b: ArrayRef = Arc::new(StringArray::from(vec!["a", "b"]));
-    let input = RecordBatch::try_from_iter(vec![("a", a), ("b", b)]).unwrap();
+    // The "very long string" will not be inlined, and force the creation of a 
data buffer.

Review Comment:
   Could you also test this with two other StringViewArrays so the total 
coverage is:
   
   1. No variadic buffers (all strings short (less than 12 bytes) ) -- 
currently not covered
   2. 1 variadic buffer (at least one long string (greater than 12 bytes)) -- 
covered
   3. 2 variadic buffers (example below)
   
   You can create a StringViewArray with multiple variadic buffers via 
`with_fixed_block_size` like this:
   
   
https://github.com/apache/arrow-rs/blob/4295d37b8e2737fe7ef9870effa1c2053c674e34/arrow-array/src/builder/generic_bytes_view_builder.rs#L558-L575



##########
arrow-array/src/ffi.rs:
##########
@@ -373,13 +380,30 @@ impl<'a> ImportedArrowArray<'a> {
 
     /// returns all buffers, as organized by Rust (i.e. null buffer is skipped 
if it's present
     /// in the spec of the type)
-    fn buffers(&self, can_contain_null_mask: bool) -> Result<Vec<Buffer>> {
+    fn buffers(&self, can_contain_null_mask: bool, variadic: bool) -> 
Result<Vec<Buffer>> {

Review Comment:
   Can you please add a unit test for this code similar to 
`round_trip_string_array` to match the existing test coverage that round trips 
3 string view arrays:
   1. No variadic buffers (all strings short (less than 12 bytes) )
   2. 1 variadic buffer (at least one long string (greater than 12 bytes))
   3. 2 variadic buffers (example below)



-- 
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]

Reply via email to