You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "klion26 (via GitHub)" <gi...@apache.org> on 2025/07/24 03:46:27 UTC
[PR] [Variant] Avoid extra buffer allocation in ListBuilder [arrow-rs]
klion26 opened a new pull request, #7987:
URL: https://github.com/apache/arrow-rs/pull/7987
This commit will reuse parent buffer for ListBuilder, so that it doesn't need to copy the buffer when finishing the builder.
# Which issue does this PR close?
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax.
- Closes #7977 .
# Rationale for this change
This pr wants to avoid the extra buffer allocation in ListBuilder.
# What changes are included in this PR?
- Reuse the parent's buffer when creating a `ListBuilder`, all contents will be written to the buffer of the parent directly
- When `ListBuilder::finish`, we'll fill the header for the current list in the parent's buffer
- Will roll back the value of has written into the parent's buffer in `drop` if `ListBuilder::finish` has not been called.
# Are these changes tested?
The change was covered by existing tests mainly `test_nested_list_with_heterogeneous_fields_for_buffer_reuse`
# 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] [Variant] Avoid extra buffer allocation in ListBuilder [arrow-rs]
Posted by "klion26 (via GitHub)" <gi...@apache.org>.
klion26 commented on code in PR #7987:
URL: https://github.com/apache/arrow-rs/pull/7987#discussion_r2227278027
##########
parquet-variant/src/builder.rs:
##########
@@ -1242,7 +1294,18 @@ impl<'a> ListBuilder<'a> {
/// This is to ensure that the list is always finalized before its parent builder
/// is finalized.
impl Drop for ListBuilder<'_> {
- fn drop(&mut self) {}
+ fn drop(&mut self) {
Review Comment:
Verified that the `drop` for `ListBuilder` was covered with `cargo llvm-cov --html test -p parquet-variant`
<img width="718" height="349" alt="image" src="https://github.com/user-attachments/assets/4f7dd4f5-a3a6-41f5-b443-3e8fc1612f5a" />
--
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] [Variant] Avoid extra buffer allocation in ListBuilder [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #7987:
URL: https://github.com/apache/arrow-rs/pull/7987
--
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] [Variant] Avoid extra buffer allocation in ListBuilder [arrow-rs]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #7987:
URL: https://github.com/apache/arrow-rs/pull/7987#discussion_r2232641962
##########
parquet-variant/src/builder.rs:
##########
@@ -1216,24 +1211,45 @@ impl<'a> ListBuilder<'a> {
/// Finalizes this list and appends it to its parent, which otherwise remains unmodified.
pub fn finish(mut self) {
- let data_size = self.buffer.offset();
+ let buffer = self.parent_state.buffer();
+
+ let data_size = buffer.offset() - self.parent_value_offset_base;
+
let num_elements = self.offsets.len();
let is_large = num_elements > u8::MAX as usize;
let offset_size = int_size(data_size);
- // Get parent's buffer
- let parent_buffer = self.parent_state.buffer();
- let starting_offset = parent_buffer.offset();
+ let starting_offset = self.parent_value_offset_base;
+
+ let header_size = 1 + // header
+ if is_large { 4 } else { 1 } + // is_large
+ (self.offsets.len() + 1) * offset_size as usize; // offsets and data size
+ // Calculated header size becomes a hint; being wrong only risks extra allocations.
+ // Make sure to reserve enough capacity to handle the extra bytes we'll truncate.
Review Comment:
Hmm, can we rephrase the comment, I don't quite get what it means. Do you try to say the `header_size` is just a hint and we will allocate extra space?
--
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] [Variant] Avoid extra buffer allocation in ListBuilder [arrow-rs]
Posted by "klion26 (via GitHub)" <gi...@apache.org>.
klion26 commented on PR #7987:
URL: https://github.com/apache/arrow-rs/pull/7987#issuecomment-3111877503
@alamb @scovich @viirya, please help review this when you're free, thanks.
I've created benchmarks for various implementations. The current implementation is the winner, and the alternatives are
1. Current implementation with `PackedU32Iterator`
2. Splice with `Iterator` ([code here](https://github.com/klion26/arrow-rs/commit/1d594b3b4a461a44ad72ecac730cbdfc537767d4#diff-19c7b0b0d73ef11489af7932f49046a19ec7790896a8960add5a3ded21d5657aR1220))
3. Collect the header with iterator before splice([code here](https://github.com/klion26/arrow-rs/blob/7179a56258429d8431273d525ced836dd706e3e4/parquet-variant/src/builder.rs#L1238))
4. Splice with `[1u8, header_size]` followed by fill the header ([code here](https://github.com/klion26/arrow-rs/blob/9cc1b04a007c54274db81059d317747b2512e169/parquet-variant/src/builder.rs#L1282))
The benchmark comparison result from my laptop
## 1 PackedU32 Iterator
```
group 7977_packedu32_iterator main
----- ----------------------- ----
batch_json_string_to_variant json_list 8k string 1.00 41.7±5.53ms ? ?/sec 1.22 51.0±7.14ms ? ?/sec
batch_json_string_to_variant random_json(2633 bytes per document) 1.00 414.0±41.45ms ? ?/sec 1.11 458.7±48.08ms ? ?/sec
batch_json_string_to_variant repeated_struct 8k string 1.00 15.7±2.04ms ? ?/sec 1.01 15.9±1.67ms ? ?/sec
variant_get_primitive 1.09 2.7±0.34ms ? ?/sec 1.00 2.5±0.28ms ? ?/sec
```
## 2 Splice with `Iterator`
```
group 7977_avoid_allocation_for_list_builder main
----- -------------------------------------- ----
batch_json_string_to_variant json_list 8k string 1.00 46.7±6.23ms ? ?/sec 1.09 51.0±7.14ms ? ?/sec
batch_json_string_to_variant random_json(2633 bytes per document) 1.00 418.0±42.38ms ? ?/sec 1.10 458.7±48.08ms ? ?/sec
batch_json_string_to_variant repeated_struct 8k string 1.00 15.9±1.97ms ? ?/sec 1.00 15.9±1.67ms ? ?/sec
variant_get_primitive 1.01 2.5±0.28ms ? ?/sec 1.00 2.5±0.28ms ? ?/sec
```
## 3 Collect the header with iterator before splice
```
group 7977_collect_before_splice main
----- -------------------------- ----
batch_json_string_to_variant json_list 8k string 1.00 46.4±4.60ms ? ?/sec 1.10 51.0±7.14ms ? ?/sec
batch_json_string_to_variant random_json(2633 bytes per document) 1.00 424.5±43.27ms ? ?/sec 1.08 458.7±48.08ms ? ?/sec
batch_json_string_to_variant repeated_struct 8k string 1.00 15.9±1.83ms ? ?/sec 1.00 15.9±1.67ms ? ?/sec
variant_get_primitive 1.02 2.5±0.31ms ? ?/sec 1.00 2.5±0.28ms ? ?/sec
```
## 4 Splice with `[1u8,header_size]` followed by fill the header
```
group 7977_fill_before_splice main
----- ----------------------- ----
batch_json_string_to_variant json_list 8k string 1.00 45.1±2.68ms ? ?/sec 1.13 51.0±7.14ms ? ?/sec
batch_json_string_to_variant random_json(2633 bytes per document) 1.00 419.6±40.92ms ? ?/sec 1.09 458.7±48.08ms ? ?/sec
batch_json_string_to_variant repeated_struct 8k string 1.04 16.5±1.20ms ? ?/sec 1.00 15.9±1.67ms ? ?/sec
variant_get_primitive 1.12 2.8±0.26ms ? ?/sec 1.00 2.5±0.28ms ? ?/sec
```
--
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] [Variant] Avoid extra buffer allocation in ListBuilder [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on code in PR #7987:
URL: https://github.com/apache/arrow-rs/pull/7987#discussion_r2232860635
##########
parquet-variant/src/builder.rs:
##########
@@ -1216,24 +1211,45 @@ impl<'a> ListBuilder<'a> {
/// Finalizes this list and appends it to its parent, which otherwise remains unmodified.
pub fn finish(mut self) {
- let data_size = self.buffer.offset();
+ let buffer = self.parent_state.buffer();
+
+ let data_size = buffer.offset() - self.parent_value_offset_base;
+
let num_elements = self.offsets.len();
let is_large = num_elements > u8::MAX as usize;
let offset_size = int_size(data_size);
- // Get parent's buffer
- let parent_buffer = self.parent_state.buffer();
- let starting_offset = parent_buffer.offset();
+ let starting_offset = self.parent_value_offset_base;
+
+ let header_size = 1 + // header
+ if is_large { 4 } else { 1 } + // is_large
+ (self.offsets.len() + 1) * offset_size as usize; // offsets and data size
+ // Calculated header size becomes a hint; being wrong only risks extra allocations.
+ // Make sure to reserve enough capacity to handle the extra bytes we'll truncate.
Review Comment:
> When `header_size` will be incorrect?
The size if calculated separately, and then the actual bytes are appended. That opens up a bug surface -- any time the two disagree, `header_size` will be wrong. If the code directly relied on the size being correct, e.g. because we allocate that many bytes and then index them, we could have produce a bad variant value (either because there's an extra run of inserted bytes, or because of a buffer overflow while indexing. But because the calculated size is only a capacity hint for the vec, the cost of being wrong is very low.
--
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] [Variant] Avoid extra buffer allocation in ListBuilder [arrow-rs]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #7987:
URL: https://github.com/apache/arrow-rs/pull/7987#discussion_r2232620246
##########
parquet-variant/src/builder.rs:
##########
@@ -70,6 +70,13 @@ fn write_offset_at_pos(buf: &mut [u8], start_pos: usize, value: usize, nbytes: u
buf[start_pos..start_pos + nbytes as usize].copy_from_slice(&bytes[..nbytes as usize]);
}
+/// Append `value_bytes` of given `value` into `dest`.
Review Comment:
`value_bytes` is the byte width of the value?
```suggestion
/// Append `value_bytes` bytes of given `value` into `dest`.
```
--
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] [Variant] Avoid extra buffer allocation in ListBuilder [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #7987:
URL: https://github.com/apache/arrow-rs/pull/7987#issuecomment-3127646297
Thanks again @klion26 @scovich @viirya 🚀
--
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] [Variant] Avoid extra buffer allocation in ListBuilder [arrow-rs]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #7987:
URL: https://github.com/apache/arrow-rs/pull/7987#discussion_r2232642859
##########
parquet-variant/src/builder.rs:
##########
@@ -1216,24 +1211,45 @@ impl<'a> ListBuilder<'a> {
/// Finalizes this list and appends it to its parent, which otherwise remains unmodified.
pub fn finish(mut self) {
- let data_size = self.buffer.offset();
+ let buffer = self.parent_state.buffer();
+
+ let data_size = buffer.offset() - self.parent_value_offset_base;
+
let num_elements = self.offsets.len();
let is_large = num_elements > u8::MAX as usize;
let offset_size = int_size(data_size);
- // Get parent's buffer
- let parent_buffer = self.parent_state.buffer();
- let starting_offset = parent_buffer.offset();
+ let starting_offset = self.parent_value_offset_base;
+
+ let header_size = 1 + // header
+ if is_large { 4 } else { 1 } + // is_large
Review Comment:
```suggestion
if is_large { 4 } else { 1 } + // is_large: 4 bytes, else 1 byte.
```
--
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] [Variant] Avoid extra buffer allocation in ListBuilder [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #7987:
URL: https://github.com/apache/arrow-rs/pull/7987#issuecomment-3121765370
@klion26 it looks like there are some good suggestions from @viirya and @scovich -- so I will wait to merge this PR until you have a chance to review them. I think it would be fine to either
1. merge this PR as is and address suggestions as a follow on
2. address the suggestions directly before merging
Please let us know what you prefer
--
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] [Variant] Avoid extra buffer allocation in ListBuilder [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on PR #7987:
URL: https://github.com/apache/arrow-rs/pull/7987#issuecomment-3122190114
> I meant avoiding that allocation by shifting the byes over in one go and then writing directly into the output buffer:
>
> ```rust
> // insert header_size bytes into the output, shifting existing bytes down
> buffer.splice(starting_offset..starting_offset+header_length, std::iter::repeat(0));
> // write header directly into buffer[starting_offset], buffer[starting_offset+1], etc
> ```
Yeah, IIRC that was the original approach, but I had cautioned that calculating the splice size incorrectly would cause subsequent indexing to corrupt the variant (either by leaving unused zeros or by overflowing the spliced region). And since the original approach was anyway using `vec![0u8; header_length]` as the source of zeros, I suggested to populate the vec directly instead. Safe and not more expensive.
It could be that not allocating the temp buffer at all does improve performance even further, tho it would come with the risk of corruption if the spliced region were ever the wrong size.
--
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] [Variant] Avoid extra buffer allocation in ListBuilder [arrow-rs]
Posted by "klion26 (via GitHub)" <gi...@apache.org>.
klion26 commented on code in PR #7987:
URL: https://github.com/apache/arrow-rs/pull/7987#discussion_r2233945914
##########
parquet-variant/src/builder.rs:
##########
@@ -1216,24 +1211,45 @@ impl<'a> ListBuilder<'a> {
/// Finalizes this list and appends it to its parent, which otherwise remains unmodified.
pub fn finish(mut self) {
- let data_size = self.buffer.offset();
+ let buffer = self.parent_state.buffer();
+
+ let data_size = buffer.offset() - self.parent_value_offset_base;
Review Comment:
fixed
##########
parquet-variant/src/builder.rs:
##########
@@ -1256,29 +1221,28 @@ impl<'a> ListBuilder<'a> {
let starting_offset = self.parent_value_offset_base;
+ let header_size = 1 + // header
+ if is_large { 4 } else { 1 } + // is_large
Review Comment:
fixed
##########
parquet-variant/src/builder.rs:
##########
@@ -1216,24 +1211,45 @@ impl<'a> ListBuilder<'a> {
/// Finalizes this list and appends it to its parent, which otherwise remains unmodified.
pub fn finish(mut self) {
- let data_size = self.buffer.offset();
+ let buffer = self.parent_state.buffer();
+
+ let data_size = buffer.offset() - self.parent_value_offset_base;
+
let num_elements = self.offsets.len();
let is_large = num_elements > u8::MAX as usize;
let offset_size = int_size(data_size);
- // Get parent's buffer
- let parent_buffer = self.parent_state.buffer();
- let starting_offset = parent_buffer.offset();
+ let starting_offset = self.parent_value_offset_base;
+
+ let header_size = 1 + // header
+ if is_large { 4 } else { 1 } + // is_large
Review Comment:
fixed
##########
parquet-variant/src/builder.rs:
##########
@@ -1256,29 +1221,28 @@ impl<'a> ListBuilder<'a> {
let starting_offset = self.parent_value_offset_base;
+ let header_size = 1 + // header
+ if is_large { 4 } else { 1 } + // is_large
+ (self.offsets.len() + 1) * offset_size as usize; // offsets and data size
+
+ // Calculated header size becomes a hint; being wrong only risks extra allocations.
+ // Make sure to reserve enough capacity to handle the extra bytes we'll truncate.
+ let mut bytes_to_splice = Vec::with_capacity(header_size + 3);
// Write header
let header = array_header(is_large, offset_size);
+ bytes_to_splice.push(header);
- let num_elements_bytes =
- num_elements
- .to_le_bytes()
- .into_iter()
- .take(if is_large { 4 } else { 1 });
- let offsets = PackedU32Iterator::new(
- offset_size as usize,
- self.offsets
- .clone()
- .into_iter()
- .map(|offset| (offset as u32).to_le_bytes()),
+ append_packed_u32(
+ &mut bytes_to_splice,
+ num_elements as u32,
+ if is_large { 4 } else { 1 },
Review Comment:
fixed
##########
parquet-variant/src/builder.rs:
##########
@@ -1121,16 +1099,27 @@ impl VariantBuilder {
pub struct ListBuilder<'a> {
parent_state: ParentState<'a>,
offsets: Vec<usize>,
- buffer: ValueBuffer,
+ /// The starting offset in the parent's buffer where this list starts
+ parent_value_offset_base: usize,
+ /// The starting offset in the parent's metadata buffer where this list starts
+ /// used to truncate the written fields in `drop` if the current list has not been finished
+ parent_metadata_offset_base: usize,
+ /// Whether the list has been finished, the written content of the current list
+ /// will be truncated in `drop` if `has_been_finished` is false
+ has_been_finished: bool,
validate_unique_fields: bool,
}
impl<'a> ListBuilder<'a> {
fn new(parent_state: ParentState<'a>, validate_unique_fields: bool) -> Self {
+ let offset_base = parent_state.buffer_current_offset();
+ let meta_offset_base = parent_state.metadata_current_offset();
Self {
parent_state,
offsets: vec![],
- buffer: ValueBuffer::default(),
+ parent_value_offset_base: offset_base,
+ has_been_finished: false,
+ parent_metadata_offset_base: meta_offset_base,
Review Comment:
Has changed the local variable name, the current implementation aims to make the compiler happy, as `parent_state` has been moved before(the first parameter).
##########
parquet-variant/src/builder.rs:
##########
@@ -1256,29 +1221,28 @@ impl<'a> ListBuilder<'a> {
let starting_offset = self.parent_value_offset_base;
+ let header_size = 1 + // header
+ if is_large { 4 } else { 1 } + // is_large
+ (self.offsets.len() + 1) * offset_size as usize; // offsets and data size
Review Comment:
Fixed
##########
parquet-variant/src/builder.rs:
##########
@@ -1216,24 +1211,45 @@ impl<'a> ListBuilder<'a> {
/// Finalizes this list and appends it to its parent, which otherwise remains unmodified.
pub fn finish(mut self) {
- let data_size = self.buffer.offset();
+ let buffer = self.parent_state.buffer();
+
+ let data_size = buffer.offset() - self.parent_value_offset_base;
+
let num_elements = self.offsets.len();
let is_large = num_elements > u8::MAX as usize;
let offset_size = int_size(data_size);
- // Get parent's buffer
- let parent_buffer = self.parent_state.buffer();
- let starting_offset = parent_buffer.offset();
+ let starting_offset = self.parent_value_offset_base;
+
+ let header_size = 1 + // header
Review Comment:
fixed
##########
parquet-variant/src/builder.rs:
##########
@@ -70,6 +70,13 @@ fn write_offset_at_pos(buf: &mut [u8], start_pos: usize, value: usize, nbytes: u
buf[start_pos..start_pos + nbytes as usize].copy_from_slice(&bytes[..nbytes as usize]);
}
+/// Append `value_bytes` of given `value` into `dest`.
Review Comment:
fixed
--
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] [Variant] Avoid extra buffer allocation in ListBuilder [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on code in PR #7987:
URL: https://github.com/apache/arrow-rs/pull/7987#discussion_r2229500164
##########
parquet-variant/src/builder.rs:
##########
@@ -1115,22 +1086,75 @@ impl VariantBuilder {
}
}
+/// An iterator that yields the bytes of a packed u32 iterator.
+/// Will yield the first `packed_bytes` bytes of each item in the iterator.
+struct PackedU32Iterator<T: Iterator<Item = [u8; 4]>> {
+ packed_bytes: usize,
+ iterator: T,
+ current_item: [u8; 4],
+ current_byte: usize, // 0..3
+}
+
+impl<T: Iterator<Item = [u8; 4]>> PackedU32Iterator<T> {
+ fn new(packed_bytes: usize, iterator: T) -> Self {
+ // eliminate corner cases in `next` by initializing with a fake already-consumed "first" item
+ Self {
+ packed_bytes,
+ iterator,
+ current_item: [0; 4],
+ current_byte: packed_bytes,
+ }
+ }
+}
+
+impl<T: Iterator<Item = [u8; 4]>> Iterator for PackedU32Iterator<T> {
+ type Item = u8;
+
+ fn next(&mut self) -> Option<u8> {
+ if self.current_byte >= self.packed_bytes {
+ let next_item = self.iterator.next()?;
+ self.current_item = next_item;
Review Comment:
Split into two statements due to lifetime issues, I suppose?
##########
parquet-variant/src/builder.rs:
##########
@@ -1216,24 +1245,46 @@ impl<'a> ListBuilder<'a> {
/// Finalizes this list and appends it to its parent, which otherwise remains unmodified.
pub fn finish(mut self) {
- let data_size = self.buffer.offset();
+ let buffer = self.parent_state.buffer();
+
+ let data_size = buffer.offset() - self.parent_value_offset_base;
+
let num_elements = self.offsets.len();
let is_large = num_elements > u8::MAX as usize;
let offset_size = int_size(data_size);
- // Get parent's buffer
- let parent_buffer = self.parent_state.buffer();
- let starting_offset = parent_buffer.offset();
+ let starting_offset = self.parent_value_offset_base;
// Write header
let header = array_header(is_large, offset_size);
- parent_buffer.append_header(header, is_large, num_elements);
- // Write out the offset array followed by the value bytes
- let offsets = std::mem::take(&mut self.offsets);
- parent_buffer.append_offset_array(offsets, Some(data_size), offset_size);
- parent_buffer.append_slice(self.buffer.inner());
+ let num_elements_bytes =
+ num_elements
+ .to_le_bytes()
+ .into_iter()
+ .take(if is_large { 4 } else { 1 });
+ let offsets = PackedU32Iterator::new(
+ offset_size as usize,
+ self.offsets
+ .clone()
Review Comment:
Just do `self.offsets.iter().map(|&offset| ...)`, relying on the fact that u32 is Copy -- instead of cloning the whole vec?
##########
parquet-variant/src/builder.rs:
##########
@@ -1216,24 +1245,46 @@ impl<'a> ListBuilder<'a> {
/// Finalizes this list and appends it to its parent, which otherwise remains unmodified.
pub fn finish(mut self) {
- let data_size = self.buffer.offset();
+ let buffer = self.parent_state.buffer();
+
+ let data_size = buffer.offset() - self.parent_value_offset_base;
+
let num_elements = self.offsets.len();
let is_large = num_elements > u8::MAX as usize;
let offset_size = int_size(data_size);
- // Get parent's buffer
- let parent_buffer = self.parent_state.buffer();
- let starting_offset = parent_buffer.offset();
+ let starting_offset = self.parent_value_offset_base;
// Write header
let header = array_header(is_large, offset_size);
- parent_buffer.append_header(header, is_large, num_elements);
- // Write out the offset array followed by the value bytes
- let offsets = std::mem::take(&mut self.offsets);
- parent_buffer.append_offset_array(offsets, Some(data_size), offset_size);
- parent_buffer.append_slice(self.buffer.inner());
+ let num_elements_bytes =
+ num_elements
+ .to_le_bytes()
+ .into_iter()
+ .take(if is_large { 4 } else { 1 });
+ let offsets = PackedU32Iterator::new(
+ offset_size as usize,
+ self.offsets
+ .clone()
Review Comment:
Or,
```rust
let offsets = std::mem::take(self.offsets).into_iter();
let offsets = offsets.map(|offset| (offset as u32).to_le_bytes());
let offsets = Packedu32Iterator::new(offset_size as usize, offsets);
```
--
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] [Variant] Avoid extra buffer allocation in ListBuilder [arrow-rs]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #7987:
URL: https://github.com/apache/arrow-rs/pull/7987#discussion_r2232643607
##########
parquet-variant/src/builder.rs:
##########
@@ -1216,24 +1211,45 @@ impl<'a> ListBuilder<'a> {
/// Finalizes this list and appends it to its parent, which otherwise remains unmodified.
pub fn finish(mut self) {
- let data_size = self.buffer.offset();
+ let buffer = self.parent_state.buffer();
+
+ let data_size = buffer.offset() - self.parent_value_offset_base;
+
let num_elements = self.offsets.len();
let is_large = num_elements > u8::MAX as usize;
let offset_size = int_size(data_size);
- // Get parent's buffer
- let parent_buffer = self.parent_state.buffer();
- let starting_offset = parent_buffer.offset();
+ let starting_offset = self.parent_value_offset_base;
+
+ let header_size = 1 + // header
Review Comment:
```suggestion
let header_size = 1 + // header (i.e., `array_header`)
```
--
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] [Variant] Avoid extra buffer allocation in ListBuilder [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #7987:
URL: https://github.com/apache/arrow-rs/pull/7987#issuecomment-3121763470
> What do you mean by "shifting" and "appending" sorry? The buffer already contains the value bytes by the time we know the header info, so AFAIK we only have three choices:
>
> 1. Guess (correctly!) beforehand how many header bytes are needed, and allocate space for them before appending the value bytes. Error-prone unless `splice` is used to replace the pre-allocated space with the actual header bytes.
> 2. Directly splice in the header bytes (what this PR does). Safe, but still has to shift bytes over.
> 3. Splice in a zero-byte region of the correct size to shift the bytes, and then loop back over in order to populate the region. Error-prone but doesn't need a temp vector.
>
> Were you referring to one of the above? Or something else?
I meant specifically, https://github.com/apache/arrow-rs/pull/7987/files#diff-19c7b0b0d73ef11489af7932f49046a19ec7790896a8960add5a3ded21d5657aR1230 ( I thought I left a specific comment about this but I can't find it now 🤔 )
Basically rather than allocating a new temporary vector to create the header and then splicing those bytes in like
```rust
let mut bytes_to_splice = Vec::with_capacity(header_size + 3);
// .... build header
// slice
buffer
.inner_mut()
.splice(starting_offset..starting_offset, bytes_to_splice);
```
I meant avoiding that allocation by shifting the byes over in one go and then writing directly into the output buffer:
```shell
// insert header_size bytes into the output, shifting existing bytes down
buffer.splice(starting_offset..starting_offset+header_length, std::iter::repeat(0));
// write header directly into buffer[starting_offset], buffer[starting_offset+1], etc
```
This looks somewhat similar to what @klion26 did in
> Splice with Iterator ([code here](https://github.com/klion26/arrow-rs/blob/1d594b3b4a461a44ad72ecac730cbdfc537767d4/parquet-variant/src/builder.rs#L1220-L1240))
Though in that example the header is created during the insertion
My suggestion is to merge this PR as is and then we can fiddle around with potentially other optimizations as a follow on PR
--
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] [Variant] Avoid extra buffer allocation in ListBuilder [arrow-rs]
Posted by "klion26 (via GitHub)" <gi...@apache.org>.
klion26 commented on PR #7987:
URL: https://github.com/apache/arrow-rs/pull/7987#issuecomment-3131413760
@alamb @scovich @viirya thanks very much for the review and merging!
--
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] [Variant] Avoid extra buffer allocation in ListBuilder [arrow-rs]
Posted by "klion26 (via GitHub)" <gi...@apache.org>.
klion26 commented on code in PR #7987:
URL: https://github.com/apache/arrow-rs/pull/7987#discussion_r2227352892
##########
parquet-variant/src/builder.rs:
##########
@@ -1216,24 +1245,46 @@ impl<'a> ListBuilder<'a> {
/// Finalizes this list and appends it to its parent, which otherwise remains unmodified.
pub fn finish(mut self) {
- let data_size = self.buffer.offset();
+ let buffer = self.parent_state.buffer();
+
+ let data_size = buffer.offset() - self.parent_value_offset_base;
+
let num_elements = self.offsets.len();
let is_large = num_elements > u8::MAX as usize;
let offset_size = int_size(data_size);
- // Get parent's buffer
- let parent_buffer = self.parent_state.buffer();
- let starting_offset = parent_buffer.offset();
+ let starting_offset = self.parent_value_offset_base;
// Write header
let header = array_header(is_large, offset_size);
- parent_buffer.append_header(header, is_large, num_elements);
- // Write out the offset array followed by the value bytes
- let offsets = std::mem::take(&mut self.offsets);
- parent_buffer.append_offset_array(offsets, Some(data_size), offset_size);
- parent_buffer.append_slice(self.buffer.inner());
+ let num_elements_bytes =
+ num_elements
+ .to_le_bytes()
+ .into_iter()
+ .take(if is_large { 4 } else { 1 });
+ let offsets = PackedU32Iterator::new(
+ offset_size as usize,
+ self.offsets
+ .clone()
Review Comment:
Add `clone()` here to make compile happy, or the compile will throw `cannot move out of type `ListBuilder<'_>`, which implements the `Drop` trait`
--
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] [Variant] Avoid extra buffer allocation in ListBuilder [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #7987:
URL: https://github.com/apache/arrow-rs/pull/7987#issuecomment-3119747194
🤖: Benchmark completed
<details><summary>Details</summary>
<p>
```
group 7977_avoid_extra_buffer_with_packedu32_iterator main
----- ----------------------------------------------- ----
batch_json_string_to_variant json_list 8k string 1.00 28.3±0.13ms ? ?/sec 1.02 28.8±0.10ms ? ?/sec
batch_json_string_to_variant random_json(2633 bytes per document) 1.00 335.7±1.21ms ? ?/sec 1.09 366.4±4.37ms ? ?/sec
batch_json_string_to_variant repeated_struct 8k string 1.00 8.2±0.02ms ? ?/sec 1.00 8.2±0.02ms ? ?/sec
variant_get_primitive 1.00 1359.2±2.98µs ? ?/sec 1.03 1396.4±3.18µs ? ?/sec
```
</p>
</details>
--
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] [Variant] Avoid extra buffer allocation in ListBuilder [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on code in PR #7987:
URL: https://github.com/apache/arrow-rs/pull/7987#discussion_r2232259020
##########
parquet-variant/src/builder.rs:
##########
@@ -1256,29 +1221,28 @@ impl<'a> ListBuilder<'a> {
let starting_offset = self.parent_value_offset_base;
+ let header_size = 1 + // header
+ if is_large { 4 } else { 1 } + // is_large
Review Comment:
```suggestion
let num_elements_size = if is_large { 4 } else { 1 }
let header_size = 1 + // header
num_elements_size + // num_elements
```
(and then can reuse `num_elements_size` below)
##########
parquet-variant/src/builder.rs:
##########
@@ -1121,16 +1099,27 @@ impl VariantBuilder {
pub struct ListBuilder<'a> {
parent_state: ParentState<'a>,
offsets: Vec<usize>,
- buffer: ValueBuffer,
+ /// The starting offset in the parent's buffer where this list starts
+ parent_value_offset_base: usize,
+ /// The starting offset in the parent's metadata buffer where this list starts
+ /// used to truncate the written fields in `drop` if the current list has not been finished
+ parent_metadata_offset_base: usize,
+ /// Whether the list has been finished, the written content of the current list
+ /// will be truncated in `drop` if `has_been_finished` is false
+ has_been_finished: bool,
validate_unique_fields: bool,
}
impl<'a> ListBuilder<'a> {
fn new(parent_state: ParentState<'a>, validate_unique_fields: bool) -> Self {
+ let offset_base = parent_state.buffer_current_offset();
+ let meta_offset_base = parent_state.metadata_current_offset();
Self {
parent_state,
offsets: vec![],
- buffer: ValueBuffer::default(),
+ parent_value_offset_base: offset_base,
+ has_been_finished: false,
+ parent_metadata_offset_base: meta_offset_base,
Review Comment:
If we're anyway doing `:`, why not just fold in the logic directly?
```suggestion
parent_value_offset_base: parent_state.buffer_current_offset(),
has_been_finished: false,
parent_metadata_offset_base: parent_state.metadata_current_offset(),
```
Alternatively, the `let` above could give the correct name from the start, so it can just be passed directly:
```suggestion
parent_value_offset_base,
has_been_finished: false,
parent_metadata_offset_base,
```
##########
parquet-variant/src/builder.rs:
##########
@@ -1256,29 +1221,28 @@ impl<'a> ListBuilder<'a> {
let starting_offset = self.parent_value_offset_base;
+ let header_size = 1 + // header
+ if is_large { 4 } else { 1 } + // is_large
+ (self.offsets.len() + 1) * offset_size as usize; // offsets and data size
+
+ // Calculated header size becomes a hint; being wrong only risks extra allocations.
+ // Make sure to reserve enough capacity to handle the extra bytes we'll truncate.
+ let mut bytes_to_splice = Vec::with_capacity(header_size + 3);
// Write header
let header = array_header(is_large, offset_size);
+ bytes_to_splice.push(header);
- let num_elements_bytes =
- num_elements
- .to_le_bytes()
- .into_iter()
- .take(if is_large { 4 } else { 1 });
- let offsets = PackedU32Iterator::new(
- offset_size as usize,
- self.offsets
- .clone()
- .into_iter()
- .map(|offset| (offset as u32).to_le_bytes()),
+ append_packed_u32(
+ &mut bytes_to_splice,
+ num_elements as u32,
+ if is_large { 4 } else { 1 },
Review Comment:
```suggestion
num_elements_size,
```
##########
parquet-variant/src/builder.rs:
##########
@@ -1256,29 +1221,28 @@ impl<'a> ListBuilder<'a> {
let starting_offset = self.parent_value_offset_base;
+ let header_size = 1 + // header
+ if is_large { 4 } else { 1 } + // is_large
+ (self.offsets.len() + 1) * offset_size as usize; // offsets and data size
Review Comment:
```suggestion
(num_elements + 1) * offset_size as usize; // offsets and data size
```
--
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] [Variant] Avoid extra buffer allocation in ListBuilder [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #7987:
URL: https://github.com/apache/arrow-rs/pull/7987#issuecomment-3119713590
🤖 `./gh_compare_arrow.sh` [Benchmark Script](https://github.com/alamb/datafusion-benchmarking/blob/main/gh_compare_arrow.sh) Running
Linux aal-dev 6.11.0-1016-gcp #16~24.04.1-Ubuntu SMP Wed May 28 02:40:52 UTC 2025 x86_64 x86_64 x86_64 GNU/Linux
Comparing 7977_avoid_extra_buffer_with_packedu32_iterator (bec3ba8d39e5ab8cf584afa9d7443222dfd33362) to ec81db35bb2573fa6776051e9fd613da80f34d6d [diff](https://github.com/apache/arrow-rs/compare/ec81db35bb2573fa6776051e9fd613da80f34d6d..bec3ba8d39e5ab8cf584afa9d7443222dfd33362)
BENCH_NAME=variant_kernels
BENCH_COMMAND=cargo bench --features=arrow,async,test_common,experimental --bench variant_kernels
BENCH_FILTER=
BENCH_BRANCH_NAME=7977_avoid_extra_buffer_with_packedu32_iterator
Results will be posted here when complete
--
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] [Variant] Avoid extra buffer allocation in ListBuilder [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #7987:
URL: https://github.com/apache/arrow-rs/pull/7987#issuecomment-3124300415
> t could be that not allocating the temp buffer at all does improve performance even further, tho it would come with the risk of corruption if the spliced region were ever the wrong size.
I agree
So I think we should proceed with the approach in this PR and then we can go with the even fewer allocations approach if we can get some benchmarks that show it makes any measurable difference
--
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] [Variant] Avoid extra buffer allocation in ListBuilder [arrow-rs]
Posted by "klion26 (via GitHub)" <gi...@apache.org>.
klion26 commented on PR #7987:
URL: https://github.com/apache/arrow-rs/pull/7987#issuecomment-3118389103
@scovich thanks for the detailed review and suggestion.
> Do the benchmarks cover different offset sizes, is_large true/false, etc? Or are they always the same offset size?
The benchmark contains vary lenght of lists, but all the lenght less than 255 -- `is_large` is always false
I've run the following benchmarks
1. previous `1` with clone-free
2. Fifth approach
3. Sixth approach
The results show that the `sixth`is better. I've updated the implementation to the sixth approach.
> The steps to generate the result:
> 1. Change the previous `1` approach to clone-free
> 2. Code fifth approach
> 3. Code sixth approach
> 4. Execute the command `cargo bench --features=arrow,async,test_common,experimental --bench variant_kernels -- --save-baseline $BRANCH_NAME` for the three approaches and the main branch, one by one.
> 5. execute the command `critcmp main ${BRANCH_NAME} ` for the three approaches.
## pervious `1` with clone-free
```
group 7977_avoid_extra_buffer_with_packedu32_iterator main
----- ----------------------------------------------- ----
batch_json_string_to_variant json_list 8k string 1.00 33.5±1.17ms ? ?/sec 1.10 36.8±1.49ms ? ?/sec
batch_json_string_to_variant random_json(2633 bytes per document) 1.00 338.9±7.71ms ? ?/sec 1.10 373.5±8.07ms ? ?/sec
batch_json_string_to_variant repeated_struct 8k string 1.02 13.6±0.45ms ? ?/sec 1.00 13.3±0.47ms ? ?/sec
variant_get_primitive 1.00 2.1±0.07ms ? ?/sec 1.01 2.1±0.07ms ? ?/sec
```
## fifth approach
```
group 7977_pre_populate_with_push_exten main
----- --------------------------------- ----
batch_json_string_to_variant json_list 8k string 1.00 35.8±1.52ms ? ?/sec 1.03 36.8±1.49ms ? ?/sec
batch_json_string_to_variant random_json(2633 bytes per document) 1.00 342.9±9.65ms ? ?/sec 1.09 373.5±8.07ms ? ?/sec
batch_json_string_to_variant repeated_struct 8k string 1.00 13.1±0.46ms ? ?/sec 1.01 13.3±0.47ms ? ?/sec
variant_get_primitive 1.00 2.1±0.07ms ? ?/sec 1.00 2.1±0.07ms ? ?/sec
```
<details>
<summary>code for fifth approach</summary>
let header = array_header(is_large, offset_size);
let mut bytes_to_splice = vec![header];
let num_elements_bytes =
num_elements
.to_le_bytes()
.into_iter()
.take(if is_large { 4 } else { 1 });
bytes_to_splice.extend(num_elements_bytes);
let offsets = PackedU32Iterator::new(
offset_size as usize,
self.offsets
.iter()
.map(|&offset| (offset as u32).to_le_bytes()),
);
bytes_to_splice.extend(offsets);
let data_size_bytes = data_size
.to_le_bytes()
.into_iter()
.take(offset_size as usize);
bytes_to_splice.extend(data_size_bytes);
buffer
.inner_mut()
.splice(starting_offset..starting_offset, bytes_to_splice);
</details>
## sixth approach
```
group 7977_pre_populate_with_directly_append_bytes main
----- -------------------------------------------- ----
batch_json_string_to_variant json_list 8k string 1.00 33.7±1.21ms ? ?/sec 1.09 36.8±1.49ms ? ?/sec
batch_json_string_to_variant random_json(2633 bytes per document) 1.00 333.4±7.89ms ? ?/sec 1.12 373.5±8.07ms ? ?/sec
batch_json_string_to_variant repeated_struct 8k string 1.00 13.2±0.46ms ? ?/sec 1.01 13.3±0.47ms ? ?/sec
variant_get_primitive 1.00 2.1±0.08ms ? ?/sec 1.00 2.1±0.07ms ? ?/sec```
--
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] [Variant] Avoid extra buffer allocation in ListBuilder [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #7987:
URL: https://github.com/apache/arrow-rs/pull/7987#discussion_r2231689129
##########
parquet-variant/src/builder.rs:
##########
@@ -2861,8 +2888,7 @@ mod tests {
// Only the second attempt should appear in the final variant
let (metadata, value) = builder.finish();
let metadata = VariantMetadata::try_new(&metadata).unwrap();
- assert_eq!(metadata.len(), 1);
- assert_eq!(&metadata[0], "name"); // not rolled back
+ assert!(metadata.is_empty()); // rolled back
Review Comment:
nice!
##########
parquet-variant/src/builder.rs:
##########
@@ -1216,24 +1211,45 @@ impl<'a> ListBuilder<'a> {
/// Finalizes this list and appends it to its parent, which otherwise remains unmodified.
pub fn finish(mut self) {
- let data_size = self.buffer.offset();
+ let buffer = self.parent_state.buffer();
+
+ let data_size = buffer.offset() - self.parent_value_offset_base;
Review Comment:
should we do a checked sub here to avoid underflow? An underflow would only happen with a bug in the implementation so this is probably fine
##########
parquet-variant/src/builder.rs:
##########
@@ -1121,16 +1099,27 @@ impl VariantBuilder {
pub struct ListBuilder<'a> {
parent_state: ParentState<'a>,
offsets: Vec<usize>,
- buffer: ValueBuffer,
+ /// The starting offset in the parent's buffer where this list starts
+ parent_value_offset_base: usize,
+ /// The starting offset in the parent's metadata buffer where this list starts
+ /// used to truncate the written fields in `drop` if the current list has not been finished
+ parent_metadata_offset_base: usize,
Review Comment:
this is a good idea
--
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] [Variant] Avoid extra buffer allocation in ListBuilder [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on code in PR #7987:
URL: https://github.com/apache/arrow-rs/pull/7987#discussion_r2232858996
##########
parquet-variant/src/builder.rs:
##########
@@ -70,6 +70,13 @@ fn write_offset_at_pos(buf: &mut [u8], start_pos: usize, value: usize, nbytes: u
buf[start_pos..start_pos + nbytes as usize].copy_from_slice(&bytes[..nbytes as usize]);
}
+/// Append `value_bytes` of given `value` into `dest`.
Review Comment:
Or we could just call it `value_size` like most of the other parts of the code 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] [Variant] Avoid extra buffer allocation in ListBuilder [arrow-rs]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #7987:
URL: https://github.com/apache/arrow-rs/pull/7987#discussion_r2232663787
##########
parquet-variant/src/builder.rs:
##########
@@ -1216,24 +1211,45 @@ impl<'a> ListBuilder<'a> {
/// Finalizes this list and appends it to its parent, which otherwise remains unmodified.
pub fn finish(mut self) {
- let data_size = self.buffer.offset();
+ let buffer = self.parent_state.buffer();
+
+ let data_size = buffer.offset() - self.parent_value_offset_base;
+
let num_elements = self.offsets.len();
let is_large = num_elements > u8::MAX as usize;
let offset_size = int_size(data_size);
- // Get parent's buffer
- let parent_buffer = self.parent_state.buffer();
- let starting_offset = parent_buffer.offset();
+ let starting_offset = self.parent_value_offset_base;
+
+ let header_size = 1 + // header
+ if is_large { 4 } else { 1 } + // is_large
+ (self.offsets.len() + 1) * offset_size as usize; // offsets and data size
+ // Calculated header size becomes a hint; being wrong only risks extra allocations.
+ // Make sure to reserve enough capacity to handle the extra bytes we'll truncate.
Review Comment:
When `header_size` will be incorrect?
--
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