Skip to content

Adding try_append_value implementation to ByteViewBuilder #8594

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Oct 19, 2025
Merged

Adding try_append_value implementation to ByteViewBuilder #8594

Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
8859ff7
Adding try_append_value implementation.
samueleresca Oct 12, 2025
bfe53a8
Merge branch 'main' into safer-appendvalue-bytes-view
samueleresca Oct 16, 2025
3fa929c
fix: incorrect assertion in `BitChunks::new` (#8620)
rluvaton Oct 17, 2025
a4eb148
[Variant] impl `PartialEq` and `FromIterator<Option<..>>` for `Varian…
friendlymatthew Oct 17, 2025
2f3f34a
Introduce a ThriftProtocolError to avoid allocating and formattings s…
jhorstmann Oct 17, 2025
a0e79a4
Updating.
samueleresca Oct 18, 2025
89e40d5
Merge branch 'apache:main' into safer-appendvalue-bytes-view
samueleresca Oct 18, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 45 additions & 6 deletions arrow-array/src/builder/generic_bytes_view_builder.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -306,15 +306,30 @@ impl<T: ByteViewType + ?Sized> GenericByteViewBuilder<T> {
/// - String length exceeds `u32::MAX`
#[inline]
pub fn append_value(&mut self, value: impl AsRef<T::Native>) {
self.try_append_value(value).unwrap()
}

/// Appends a value into the builder
///
/// # Errors
///
/// Returns an error if:
/// - String buffer count exceeds `u32::MAX`
/// - String length exceeds `u32::MAX`
#[inline]
pub fn try_append_value(&mut self, value: impl AsRef<T::Native>) -> Result<(), ArrowError> {
let v: &[u8] = value.as_ref().as_ref();
let length: u32 = v.len().try_into().unwrap();
let length: u32 = v.len().try_into().map_err(|_| {
ArrowError::InvalidArgumentError(format!("String length {} exceeds u32::MAX", v.len()))
})?;

if length <= MAX_INLINE_VIEW_LEN {
let mut view_buffer = [0; 16];
view_buffer[0..4].copy_from_slice(&length.to_le_bytes());
view_buffer[4..4 + v.len()].copy_from_slice(v);
self.views_buffer.push(u128::from_le_bytes(view_buffer));
self.null_buffer_builder.append_non_null();
return;
return Ok(());
}

// Deduplication if:
Expand All @@ -339,7 +354,7 @@ impl<T: ByteViewType + ?Sized> GenericByteViewBuilder<T> {
self.views_buffer.push(self.views_buffer[*idx]);
self.null_buffer_builder.append_non_null();
self.string_tracker = Some((ht, hasher));
return;
return Ok(());
}
Entry::Vacant(vacant) => {
// o.w. we insert the (string hash -> view index)
Expand All @@ -356,17 +371,41 @@ impl<T: ByteViewType + ?Sized> GenericByteViewBuilder<T> {
let to_reserve = v.len().max(self.block_size.next_size() as usize);
self.in_progress.reserve(to_reserve);
};
let offset = self.in_progress.len() as u32;
let offset: u32 = self.in_progress.len().try_into().map_err(|_| {
ArrowError::InvalidArgumentError(format!(
"In-progress buffer length {} exceeds u32::MAX",
Copy link
Contributor

@ctsk ctsk Oct 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. I think the method can recover by starting a new in-progress buffer instead of returning an error here.

  2. I am unsure if this error is even reachable.

Copy link
Contributor

@alamb alamb Oct 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a new buffer would be allocated in the line immediately above this. Maybe we should do a checked add in let required_cap = self.in_progress.len() + v.len(); 🤔

To error here, we would need a usize that doesn't fit into a u32.. I think all platforms we care about have usize that is at least u32 (aka 32-bit architectures)

Copy link
Member Author

@samueleresca samueleresca Oct 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To error here, we would need a usize that doesn't fit into a u32.. I think all platforms we care about have usize that is at least u32 (aka 32-bit architectures)

I think that would be the opposite, a usize in a 64-bit arch wouldn't fit a u32? Anyway, I will review and update these changes over the weekend

Copy link
Contributor

@alamb alamb Oct 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you are right -- thank you

Copy link
Member Author

@samueleresca samueleresca Oct 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am unsure if this error is even reachable.

I think this is right. I wasn't able to trigger this. For the panic to verify, it would need to skip the flush_in_progress() operation that happens when the buffer reaches the required capacity.

On top of that, the push_completed used by the flush_in_progress asserts on the block.len() (see below). Therefore, let offset: u32 = self.in_progress.len() wouldn't be reached:

arrow-rs/arrow-array/src/builder/generic_bytes_view_builder.rs

Lines 273 to 276 in d49f017

fn push_completed(&mut self, block: Buffer) {
assert!(block.len() < u32::MAX as usize, "Block too large");
assert!(self.completed.len() < u32::MAX as usize, "Too many blocks");
self.completed.push(block);

I'm proceeding by reverting this the map_err in the offset initialization.

self.in_progress.len()
))
})?;
self.in_progress.extend_from_slice(v);

let prefix = v
.get(0..4)
.and_then(|slice| slice.try_into().ok())
.map(u32::from_le_bytes)
.ok_or_else(|| {
ArrowError::InvalidArgumentError(
"String must be at least 4 bytes for non-inline view".to_string(),
Copy link
Contributor

@ctsk ctsk Oct 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This error is unreachable as we checked that the value is longer than MAX_INLINE_VIEW_LEN (12 bytes) above.

)
})?;

let buffer_index: u32 = self.completed.len().try_into().map_err(|_| {
ArrowError::InvalidArgumentError(format!(
"Buffer count {} exceeds u32::MAX",
self.completed.len()
))
})?;

let view = ByteView {
length,
prefix: u32::from_le_bytes(v[0..4].try_into().unwrap()),
buffer_index: self.completed.len() as u32,
prefix,
buffer_index,
offset,
};
self.views_buffer.push(view.into());
self.null_buffer_builder.append_non_null();

Ok(())
}

/// Append an `Option` value into the builder
Expand Down
Loading