Replace BufferBuilder with Vec in UnionBuilder's FieldDataValues - #11040
Open
kshivam4781 wants to merge 1 commit into
Open
Replace BufferBuilder with Vec in UnionBuilder's FieldDataValues#11040kshivam4781 wants to merge 1 commit into
kshivam4781 wants to merge 1 commit into
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Part of #10245.
Rationale for this change
UnionBuilder'sFieldDataValuestrait is implemented forBufferBuilder<T>and used purely as a growable, type-erased per-field value buffer:append/pushone value at a time, thenfinish()/finish_cloned()into aBuffer. 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>becomesimpl<T: ArrowNativeType> FieldDataValues for Vec<T>:append_null(self.advance(1)) becomesself.push(T::default()).BufferBuilder::advancezero-pads;ArrowNativeType: Defaultis 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 viamem::take) becomesBuffer::from_vec(std::mem::take(self))— same reset-and-return shape.finishis only reached throughUnionBuilder::build(self), which consumes the whole builder, so the reset value is never observed.finish_clonedkeeps the existingBuffer::from_slice_ref(self.as_slice())call unchanged —Vec<T>andBufferBuilder<T>both exposeas_slice().FieldData::newandFieldData::append_value'sdowncast_mutswitch fromBufferBuilder::<T::Native>toVec::<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 bothunion_buildertests plus everyunion_arraytest — dense/sparse, with/without nulls, offsets — sinceUnionArrayis built throughUnionBuilderor checked against its output in several of these).cargo test -p arrow-array --doc union_builder: 2 passed, 0 failed (the dense/sparse doctests onUnionBuilderitself).cargo clippy -p arrow-array --lib -- -D warnings: no diagnostics onunion_builder.rs. (Unrelated pre-existing clippy findings inarrow-data/src/data.rsreproduce identically on an unmodifiedmainunder 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 --releasemicrobenchmark (200,000UnionBuilder::append+ onefinish, 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_optionremoves and reinserts aBTreeMapentry and allocates aStringkey 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 moreBufferBuilderusage per #10245's stated goal, with no behavior or performance regression.Are there any user-facing changes?
No.
FieldDataValues/FieldDataare 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/UnionBuildercode to confirm thefinish-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.