This is an automated email from the ASF dual-hosted git repository.
Jefffrey pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/arrow-rs.git
The following commit(s) were added to refs/heads/main by this push:
new c3c6ba3db4 Replace BufferBuilder with Vec in UnionBuilder's
FieldDataValues (#11040)
c3c6ba3db4 is described below
commit c3c6ba3db409909d3de9f5ad8025472adb6e325e
Author: Shivam <[email protected]>
AuthorDate: Mon Sep 14 21:53:28 2026 -0700
Replace BufferBuilder with Vec in UnionBuilder's FieldDataValues (#11040)
# Which issue does this PR close?
Part of #10245.
# Rationale for this change
`UnionBuilder`'s `FieldDataValues` trait is implemented for
`BufferBuilder<T>` and used purely as a growable, type-erased per-field
value buffer: `append`/`push` one value at a time, then
`finish()`/`finish_cloned()` into a `Buffer`. This is exactly the
scratch-buffer shape #10245 is tracking, and it was still on the
"remaining callsites" list (the two other listed sites, arity.rs and
zip.rs, already have PRs open: #10518 and #10913).
# What changes are included in this PR?
`impl<T: ArrowNativeType> FieldDataValues for BufferBuilder<T>` becomes
`impl<T: ArrowNativeType> FieldDataValues for Vec<T>`:
- `append_null` (`self.advance(1)`) becomes `self.push(T::default())`.
`BufferBuilder::advance` zero-pads; `ArrowNativeType: Default` is a
sealed, primitive-only supertrait bound, and zero is its default for
every implementor, so this is the same padding value.
- `finish` (`self.finish()`, which resets the builder via `mem::take`)
becomes `Buffer::from_vec(std::mem::take(self))` — same reset-and-return
shape. `finish` is only reached through `UnionBuilder::build(self)`,
which consumes the whole builder, so the reset value is never observed.
- `finish_cloned` keeps the existing
`Buffer::from_slice_ref(self.as_slice())` call unchanged — `Vec<T>` and
`BufferBuilder<T>` both expose `as_slice()`.
- `FieldData::new` and `FieldData::append_value`'s `downcast_mut` switch
from `BufferBuilder::<T::Native>` to `Vec::<T::Native>`, and
`.append(v)` becomes `.push(v)`.
`type_id_builder`/`value_offset_builder`
(`Int8BufferBuilder`/`Int32BufferBuilder`) are untouched — they're a
different pair of fields, not on #10245's callsite list, and out of
scope for this targeted change.
# Are these changes tested?
Yes, by the file's existing tests — no behavior changes, so no new test
was added (matching #10851, the other merged PR in this epic that also
didn't add one):
- `cargo test -p arrow-array --lib union`: 31 passed, 0 failed (includes
both `union_builder` tests plus every `union_array` test — dense/sparse,
with/without nulls, offsets — since `UnionArray` is built through
`UnionBuilder` or checked against its output in several of these).
- `cargo test -p arrow-array --doc union_builder`: 2 passed, 0 failed
(the dense/sparse doctests on `UnionBuilder` itself).
- `cargo clippy -p arrow-array --lib -- -D warnings`: no diagnostics on
`union_builder.rs`. (Unrelated pre-existing clippy findings in
`arrow-data/src/data.rs` reproduce identically on an unmodified `main`
under my clippy version and are not part of this PR.)
- `cargo fmt -p arrow-array -- --check`: clean.
I also ran a throwaway, uncommitted `cargo run --release` microbenchmark
(200,000 `UnionBuilder::append` + one `finish`, 5 runs, median reported)
to see whether this site shows the same kind of win as the zip PR. It
doesn't, and I want to report that honestly rather than imply a bigger
effect than exists: main (`BufferBuilder`) median 22.12ms (110.6 ns/row)
vs. this branch (`Vec`) median 22.04ms (110.2 ns/row) — roughly 0.3%,
inside run-to-run noise. `UnionBuilder::append_option` removes and
reinserts a `BTreeMap` entry and allocates a `String` key on every call,
which dominates the per-row cost far more than the value-buffer push
does, so this change doesn't move the needle on its own the way the zip
kernel's tighter loop did. I'm including it anyway because it still
removes one more `BufferBuilder` usage per #10245's stated goal, with no
behavior or performance regression.
# Are there any user-facing changes?
No. `FieldDataValues`/`FieldData` are private to this module;
`UnionBuilder`'s public API is unchanged.
# Automated assistance
This PR — the code change, the verification, and this description — was
drafted by an AI coding agent (Claude), reviewed and run by me before
opening. Per CONTRIBUTING.md's AI Generated Submissions guidance: I read
the whole diff and the surrounding `FieldDataValues`/`UnionBuilder` code
to confirm the `finish`-only-reachable-through-`build(self)` reasoning
above, ran every check listed under "Are these changes tested?" myself,
and the microbenchmark numbers are from an actual local run (not
fabricated) — reported as inconclusive/noise-level rather than rounded
up to look like a win.
---
arrow-array/src/builder/union_builder.rs | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/arrow-array/src/builder/union_builder.rs
b/arrow-array/src/builder/union_builder.rs
index 9e64d97d9f..63a401387b 100644
--- a/arrow-array/src/builder/union_builder.rs
+++ b/arrow-array/src/builder/union_builder.rs
@@ -15,8 +15,8 @@
// specific language governing permissions and limitations
// under the License.
+use crate::builder::ArrayBuilder;
use crate::builder::buffer_builder::{Int8BufferBuilder, Int32BufferBuilder};
-use crate::builder::{ArrayBuilder, BufferBuilder};
use crate::{ArrayRef, ArrowPrimitiveType, UnionArray, make_array};
use arrow_buffer::NullBufferBuilder;
use arrow_buffer::{ArrowNativeType, Buffer, ScalarBuffer};
@@ -41,7 +41,7 @@ struct FieldData {
null_buffer_builder: NullBufferBuilder,
}
-/// A type-erased [`BufferBuilder`] used by [`FieldData`]
+/// A type-erased growable value buffer used by [`FieldData`]
trait FieldDataValues: std::fmt::Debug + Send + Sync {
fn as_mut_any(&mut self) -> &mut dyn Any;
@@ -52,17 +52,17 @@ trait FieldDataValues: std::fmt::Debug + Send + Sync {
fn finish_cloned(&self) -> Buffer;
}
-impl<T: ArrowNativeType> FieldDataValues for BufferBuilder<T> {
+impl<T: ArrowNativeType> FieldDataValues for Vec<T> {
fn as_mut_any(&mut self) -> &mut dyn Any {
self
}
fn append_null(&mut self) {
- self.advance(1)
+ self.push(T::default())
}
fn finish(&mut self) -> Buffer {
- self.finish()
+ Buffer::from_vec(std::mem::take(self))
}
fn finish_cloned(&self) -> Buffer {
@@ -77,7 +77,7 @@ impl FieldData {
type_id,
data_type,
slots: 0,
- values_buffer: Box::new(BufferBuilder::<T::Native>::new(capacity)),
+ values_buffer: Box::new(Vec::<T::Native>::with_capacity(capacity)),
null_buffer_builder: NullBufferBuilder::new(capacity),
}
}
@@ -86,9 +86,9 @@ impl FieldData {
fn append_value<T: ArrowPrimitiveType>(&mut self, v: T::Native) {
self.values_buffer
.as_mut_any()
- .downcast_mut::<BufferBuilder<T::Native>>()
+ .downcast_mut::<Vec<T::Native>>()
.expect("Tried to append unexpected type")
- .append(v);
+ .push(v);
self.null_buffer_builder.append(true);
self.slots += 1;
