From 0482a20c89e183228250c54ac1d82829c7f5ffe3 Mon Sep 17 00:00:00 2001 From: mwish Date: Thu, 23 Oct 2025 23:11:57 +0800 Subject: [PATCH 1/6] Add unittest --- arrow-array/src/array/byte_view_array.rs | 54 ++++++++++++++++++++++++ 1 file changed, 54 insertions(+) diff --git a/arrow-array/src/array/byte_view_array.rs b/arrow-array/src/array/byte_view_array.rs index 0319363bf5d8..b1217303bf43 100644 --- a/arrow-array/src/array/byte_view_array.rs +++ b/arrow-array/src/array/byte_view_array.rs @@ -1404,6 +1404,60 @@ mod tests { } } + #[test] + fn test_gc_huge_array() { + // Construct multiple 128 MiB BinaryView entries so total > 4 GiB + let block_len: usize = 128 * 1024 * 1024; // 128 MiB per view + let num_views: usize = 36; + + // Create a single 128 MiB data block with a simple byte pattern + let buffer = Buffer::from_vec(vec![0xAB; block_len]); + let buffer2 = Buffer::from_vec(vec![0xFF; block_len]); + + // Append this block and then add many views pointing to it + let mut builder = BinaryViewBuilder::new(); + let block_id = builder.append_block(buffer); + for _ in 0..num_views / 2 { + builder + .try_append_view(block_id, 0, block_len as u32) + .expect("append view into 128MiB block"); + } + let block_id2 = builder.append_block(buffer2); + for _ in 0..num_views / 2 { + builder + .try_append_view(block_id2, 0, block_len as u32) + .expect("append view into 128MiB block"); + } + + let array = builder.finish(); + let total = array.total_buffer_bytes_used(); + assert!( + total > u32::MAX as usize, + "Expected total non-inline bytes to exceed 4 GiB, got {}", + total + ); + + // Run gc and verify correctness + let gced = array.gc(); + assert_eq!(gced.len(), num_views, "Length mismatch after gc"); + assert_eq!(gced.null_count(), 0, "Null count mismatch after gc"); + assert_eq!( + gced.data_buffers().len(), + 1, + "gc should consolidate data into a single buffer" + ); + assert_eq!( + gced.data_buffers()[0].len(), + total, + "Consolidated buffer length should equal total non-inline bytes" + ); + + // Element-wise equality check across the entire array + array.iter().zip(gced.iter()).for_each(|(orig, got)| { + assert_eq!(orig, got, "Value mismatch after gc on huge array"); + }); + } + #[test] fn test_eq() { let test_data = [ From 7034580c54f844466d218991ede6e6be98d369d2 Mon Sep 17 00:00:00 2001 From: mwish Date: Sun, 26 Oct 2025 21:10:59 +0800 Subject: [PATCH 2/6] Pass the test --- arrow-array/src/array/byte_view_array.rs | 84 +++++++++++++++++++----- 1 file changed, 66 insertions(+), 18 deletions(-) diff --git a/arrow-array/src/array/byte_view_array.rs b/arrow-array/src/array/byte_view_array.rs index b1217303bf43..c2b660484732 100644 --- a/arrow-array/src/array/byte_view_array.rs +++ b/arrow-array/src/array/byte_view_array.rs @@ -512,18 +512,66 @@ impl GenericByteViewArray { }; } - // 3) Allocate exactly capacity for all non-inline data - let mut data_buf = Vec::with_capacity(total_large); + struct GcCopyGroup { + total_buffer_bytes: usize, + total_len: usize, + } - // 4) Iterate over views and process each inline/non-inline view - let views_buf: Vec = (0..len) - .map(|i| unsafe { self.copy_view_to_buffer(i, &mut data_buf) }) - .collect(); + let gc_copy_groups = if total_large > i32::MAX as usize { + // Slow-path: need to split into multiple copy groups + let mut groups = vec![]; + let mut current_length = 0; + let mut current_elements = 0; - // 5) Wrap up buffers - let data_block = Buffer::from_vec(data_buf); + for view in self.views() { + let len = *view as u32; + if len > MAX_INLINE_VIEW_LEN { + if current_length + len > i32::MAX as u32 { + // Start a new group + groups.push(GcCopyGroup { + total_buffer_bytes: current_length as usize, + total_len: current_elements, + }); + current_length = 0; + current_elements = 0; + } + current_length += len; + current_elements += 1; + } + } + if current_elements != 0 { + groups.push(GcCopyGroup { + total_buffer_bytes: current_length as usize, + total_len: current_elements, + }); + } + groups + } else { + let gc_copy_group = GcCopyGroup { + total_buffer_bytes: total_large, + total_len: len, + }; + vec![gc_copy_group] + }; + assert!(gc_copy_groups.len() <= i32::MAX as usize); + + let mut gc_copy_group_begin = 0; + let mut views_buf: Vec = vec![]; + let mut data_blocks = vec![]; + // 3) Copy the buffers groups by group + for (idx, gc_copy_group) in gc_copy_groups.iter().enumerate() { + let mut data_buf = Vec::with_capacity(gc_copy_group.total_buffer_bytes); + let v: Vec = (gc_copy_group_begin..gc_copy_group_begin + gc_copy_group.total_len) + .map(|i| unsafe { self.copy_view_to_buffer(i, idx as i32, &mut data_buf) }) + .collect(); + views_buf.extend(v); + let data_block = Buffer::from_vec(data_buf); + data_blocks.push(data_block); + gc_copy_group_begin += gc_copy_group.total_len; + } + + // 4) Wrap up buffers let views_scalar = ScalarBuffer::from(views_buf); - let data_blocks = vec![data_block]; // SAFETY: views_scalar, data_blocks, and nulls are correctly aligned and sized unsafe { GenericByteViewArray::new_unchecked(views_scalar, data_blocks, nulls) } @@ -541,7 +589,12 @@ impl GenericByteViewArray { /// `buffer_index` reset to `0` and its `offset` updated so that it points /// into the bytes just appended at the end of `data_buf`. #[inline(always)] - unsafe fn copy_view_to_buffer(&self, i: usize, data_buf: &mut Vec) -> u128 { + unsafe fn copy_view_to_buffer( + &self, + i: usize, + buffer_idx: i32, + data_buf: &mut Vec, + ) -> u128 { // SAFETY: `i < self.len()` ensures this is in‑bounds. let raw_view = unsafe { *self.views().get_unchecked(i) }; let mut bv = ByteView::from(raw_view); @@ -561,7 +614,7 @@ impl GenericByteViewArray { let new_offset = data_buf.len() as u32; data_buf.extend_from_slice(slice); - bv.buffer_index = 0; + bv.buffer_index = buffer_idx as u32; bv.offset = new_offset; bv.into() } @@ -1441,15 +1494,10 @@ mod tests { let gced = array.gc(); assert_eq!(gced.len(), num_views, "Length mismatch after gc"); assert_eq!(gced.null_count(), 0, "Null count mismatch after gc"); - assert_eq!( + assert_ne!( gced.data_buffers().len(), 1, - "gc should consolidate data into a single buffer" - ); - assert_eq!( - gced.data_buffers()[0].len(), - total, - "Consolidated buffer length should equal total non-inline bytes" + "gc with huge buffer should not consolidate data into a single buffer" ); // Element-wise equality check across the entire array From 464db0364c18648dfe758db49fb2eba3625ea4ab Mon Sep 17 00:00:00 2001 From: mwish Date: Sun, 26 Oct 2025 21:18:18 +0800 Subject: [PATCH 3/6] Make the code prettier --- arrow-array/src/array/byte_view_array.rs | 28 ++++++++++++++---------- 1 file changed, 17 insertions(+), 11 deletions(-) diff --git a/arrow-array/src/array/byte_view_array.rs b/arrow-array/src/array/byte_view_array.rs index c2b660484732..8bf16c6ad66d 100644 --- a/arrow-array/src/array/byte_view_array.rs +++ b/arrow-array/src/array/byte_view_array.rs @@ -555,19 +555,25 @@ impl GenericByteViewArray { }; assert!(gc_copy_groups.len() <= i32::MAX as usize); - let mut gc_copy_group_begin = 0; - let mut views_buf: Vec = vec![]; - let mut data_blocks = vec![]; - // 3) Copy the buffers groups by group - for (idx, gc_copy_group) in gc_copy_groups.iter().enumerate() { + // 3) Copy the buffers group by group + let mut views_buf = Vec::with_capacity(len); + let mut data_blocks = Vec::with_capacity(gc_copy_groups.len()); + + let mut current_view_idx = 0; + + for (group_idx, gc_copy_group) in gc_copy_groups.iter().enumerate() { let mut data_buf = Vec::with_capacity(gc_copy_group.total_buffer_bytes); - let v: Vec = (gc_copy_group_begin..gc_copy_group_begin + gc_copy_group.total_len) - .map(|i| unsafe { self.copy_view_to_buffer(i, idx as i32, &mut data_buf) }) + + let group_views: Vec = (current_view_idx + ..current_view_idx + gc_copy_group.total_len) + .map(|view_idx| unsafe { + self.copy_view_to_buffer(view_idx, group_idx as i32, &mut data_buf) + }) .collect(); - views_buf.extend(v); - let data_block = Buffer::from_vec(data_buf); - data_blocks.push(data_block); - gc_copy_group_begin += gc_copy_group.total_len; + + views_buf.extend(group_views); + data_blocks.push(Buffer::from_vec(data_buf)); + current_view_idx += gc_copy_group.total_len; } // 4) Wrap up buffers From 142284c34daed2219983d21010e5c1ba4d3c5b59 Mon Sep 17 00:00:00 2001 From: mwish Date: Sun, 26 Oct 2025 21:31:31 +0800 Subject: [PATCH 4/6] Minor change --- arrow-array/src/array/byte_view_array.rs | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/arrow-array/src/array/byte_view_array.rs b/arrow-array/src/array/byte_view_array.rs index 8bf16c6ad66d..33a1543f74c6 100644 --- a/arrow-array/src/array/byte_view_array.rs +++ b/arrow-array/src/array/byte_view_array.rs @@ -564,14 +564,13 @@ impl GenericByteViewArray { for (group_idx, gc_copy_group) in gc_copy_groups.iter().enumerate() { let mut data_buf = Vec::with_capacity(gc_copy_group.total_buffer_bytes); - let group_views: Vec = (current_view_idx - ..current_view_idx + gc_copy_group.total_len) - .map(|view_idx| unsafe { - self.copy_view_to_buffer(view_idx, group_idx as i32, &mut data_buf) - }) - .collect(); + // Directly push views to avoid intermediate Vec allocation + for view_idx in current_view_idx..current_view_idx + gc_copy_group.total_len { + let view = + unsafe { self.copy_view_to_buffer(view_idx, group_idx as i32, &mut data_buf) }; + views_buf.push(view); + } - views_buf.extend(group_views); data_blocks.push(Buffer::from_vec(data_buf)); current_view_idx += gc_copy_group.total_len; } From 3f7c50dd97259801c65ce60b28ea4461e5f72020 Mon Sep 17 00:00:00 2001 From: mwish Date: Wed, 29 Oct 2025 22:15:57 +0800 Subject: [PATCH 5/6] Fix some comments --- arrow-array/src/array/byte_view_array.rs | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/arrow-array/src/array/byte_view_array.rs b/arrow-array/src/array/byte_view_array.rs index 33a1543f74c6..c8128e9bb3be 100644 --- a/arrow-array/src/array/byte_view_array.rs +++ b/arrow-array/src/array/byte_view_array.rs @@ -517,9 +517,13 @@ impl GenericByteViewArray { total_len: usize, } + let mut groups = vec![]; + let one_group = [GcCopyGroup { + total_buffer_bytes: total_large, + total_len: len, + }]; let gc_copy_groups = if total_large > i32::MAX as usize { // Slow-path: need to split into multiple copy groups - let mut groups = vec![]; let mut current_length = 0; let mut current_elements = 0; @@ -545,15 +549,11 @@ impl GenericByteViewArray { total_len: current_elements, }); } - groups + &groups } else { - let gc_copy_group = GcCopyGroup { - total_buffer_bytes: total_large, - total_len: len, - }; - vec![gc_copy_group] + one_group.as_slice() }; - assert!(gc_copy_groups.len() <= i32::MAX as usize); + debug_assert!(gc_copy_groups.len() <= i32::MAX as usize); // 3) Copy the buffers group by group let mut views_buf = Vec::with_capacity(len); @@ -1463,6 +1463,7 @@ mod tests { } #[test] + #[cfg_attr(miri, ignore)] // Takes too long fn test_gc_huge_array() { // Construct multiple 128 MiB BinaryView entries so total > 4 GiB let block_len: usize = 128 * 1024 * 1024; // 128 MiB per view From 2110c46262af1c26f9de68fdfcdb5aaed3caf93b Mon Sep 17 00:00:00 2001 From: mwish Date: Wed, 29 Oct 2025 22:16:52 +0800 Subject: [PATCH 6/6] fix comment --- arrow-array/src/array/byte_view_array.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arrow-array/src/array/byte_view_array.rs b/arrow-array/src/array/byte_view_array.rs index c8128e9bb3be..0a2bc500423a 100644 --- a/arrow-array/src/array/byte_view_array.rs +++ b/arrow-array/src/array/byte_view_array.rs @@ -591,7 +591,7 @@ impl GenericByteViewArray { /// inside one of `self.buffers`. /// - `data_buf` must be ready to have additional bytes appended. /// - After this call, the returned view will have its - /// `buffer_index` reset to `0` and its `offset` updated so that it points + /// `buffer_index` reset to `buffer_idx` and its `offset` updated so that it points /// into the bytes just appended at the end of `data_buf`. #[inline(always)] unsafe fn copy_view_to_buffer(