kshivam4781 opened a new pull request, #11040: URL: https://github.com/apache/arrow-rs/pull/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. -- 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]
