feat(arrow): export StringArray and BinaryArray directly - #8902
Conversation
0dade7d to
4807bd2
Compare
9a91ff0 to
02fa0e3
Compare
Problem Vortex exports UTF-8 and binary data as Arrow view arrays by default. Some consumers require Arrow arrays that use 32-bit or 64-bit offsets. These consumers must cast each view array and rebuild its buffers. Implementation Add VarBinBufferBuilder for 32-bit and 64-bit offsets. Write Arrow value, offset, and validity buffers in this builder. Decode FSST, OnPair, Zstd, Dict, RunEnd, and Sparse arrays directly into this builder. Do not create an intermediate VarBinViewArray for these paths. Interface Add an optional Arrow type to the Rust and Python array APIs. Add an optional Arrow schema to the Python file API. Keep Arrow view arrays as the default output. Result Arrow uses the builder buffers without another copy. Callers can select StringArray, BinaryArray, LargeStringArray, or LargeBinaryArray. Signed-off-by: cl <cailue@apache.org>
02fa0e3 to
a377a9b
Compare
StringArray and BinaryArray
StringArray and BinaryArraySigned-off-by: cl <cailue@apache.org>
Signed-off-by: cl <cailue@apache.org>
Signed-off-by: cl <cailue@apache.org>
e3a099c to
6f03c1d
Compare
Signed-off-by: cl <cailue@apache.org>
Signed-off-by: cl <cailue@apache.org>
b99072f to
413501e
Compare
Signed-off-by: 蔡略 <cailue@apache.org>
Signed-off-by: 蔡略 <cailue@apache.org>
Merging this PR will improve performance by 12.8%
Performance Changes
Tip Curious why this is faster? Comment Comparing Footnotes
|
…w-byte-arrays Signed-off-by: 蔡略 <cailue@apache.org>
90b3f97 to
ef75f15
Compare
|
The regression is strange, nowhere can the code be touched. GPT blames codegen unit difference, it insists code in this patch disturbs LLVM's optimization and thus produced benchmark with different layouts. I've benchmarked the patch against baseline under 1 CGU. Their results are close, and regression turns out to be solved. It is interesting, but I will keep this patch free of codegen changes. |
|
Some of the microbenchmarks are sensitive to code changes. I don't think it's related to your change. Thanks for your contribution. I will review it tomorrow! |
…w-byte-arrays Signed-off-by: 蔡略 <cailue@apache.org> # Conflicts: # vortex-array/src/arrays/varbin/builder.rs # vortex-arrow/src/executor/byte.rs
|
@robert3005 please rerun benchmark. |
Signed-off-by: 蔡略 <cailue@apache.org>
robert3005
left a comment
There was a problem hiding this comment.
Reviewed at 02637a4. Nice work — the composition is correct, and I tried fairly hard to break it:
- Zstd: every slice range (
0..=12×0..=12) × 4 frame sizes, with nulls, differentially against the canonical path — all match. Thevalue_idx_start - n_skipped_valuesmath is equivalent to the views path. - FSST: every slice range, plus three slices of one FSST array composed as chunks into a single builder — all match.
- Dict: empty dict, all-null codes, empty values + all-null codes, nullable values, dict-of-dict, constant values, null codes with out-of-range payloads — all match the old StringView→cast path.
- Sliced VarBin, sliced Dict, chunked-with-mixed-encodings — all match.
- Suites green:
vortex-array3143 + 72;vortex-arrow/fsst/onpair/runend/sparse/zstd529. 0 failures.
The 4157bd9 unification is a real improvement, and I checked all four migrated call sites — FSST/OnPair's .scan() cumulative sum, append_varbin's first_offset normalization, and filter.rs's Mask::new_true(end - start) all preserve prior semantics. The Zstd change is a pure code move. It also fixes a latent bug it doesn't mention: the old append_values did its offset arithmetic in O's type, so VarBinBuilder<i32> could silently wrap in release; doing checked_add in usize first means that's now detected.
One blocking item: the offset-overflow panic (inline on builder.rs). Requesting pa.string() on >2 GiB of strings now aborts where the old StringView→cast path returned a clean Err.
One I'd push on: the new arrow_type= / schema= parameters route an unvalidated target type into a bare assert! — vx.array([1,2,3]).to_arrow_array(arrow_type=pa.binary()) panics. The assert is pre-existing (it fires on develop too for all 12 dtype×target combinations I tried), but develop gave Python callers no way to pick the target type, so this PR is what makes it reachable.
The other three inline comments are non-blocking; the finish/new_unchecked one is pre-existing and this PR narrows it rather than widening it, so it's fine as a follow-up.
Minor note not worth its own thread: append_varbin now indexes offsets[0] directly, which panics on a zero-length offsets array where windows(2) yielded empty. Builder output always has ≥1 offset and VarBinData::validate requires non-empty, so this is defensive-only — just a strictly narrower input domain than before.
I did not run the Python tests, docs doctests, clippy, or fmt; your reported results cover those and none of my findings depend on them. The >2 GiB overflow path I reasoned from code — materializing 2 GiB wasn't practical.
Generated by Claude Code
| self.offsets.extend(end_offsets.map(|offset| { | ||
| let relative_end = offset.as_(); | ||
| let end = data_start.checked_add(relative_end).unwrap_or_else(|| { | ||
| vortex_panic!("Byte offset overflow: {data_start} + {relative_end}") |
There was a problem hiding this comment.
Offset overflow should be a VortexError, not a panic.
Both this checked_add and the O::from(end) conversion below panic. For the Utf8/Binary targets (i32 offsets), that means >2 GiB of string data in one chunk aborts instead of erroring — and >2 GiB is exactly what a caller hits asking for pa.string() on a large scan.
The old path got this right for free: it built a StringViewArray and let arrow_cast::cast handle it, which returns Err(ArrowError::ComputeError) on offset overflow. So this is a behavioral regression on the failure path. In a DataFusion/DuckDB worker it's a panic rather than a surfaced error, and through PyO3 it becomes a PanicException.
Suggest making append_values return VortexResult<()> and using vortex_ensure!. Blast radius is small — filter.rs plus the four new call sites, all of which already sit in VortexResult functions.
Separately, credit where due: computing this in usize with checked_add before O::from is a real improvement over the old offset + self.data.len().as_(), which did the arithmetic in O's type and so could silently wrap for VarBinBuilder<i32> in release. That latent corruption in the filter.rs path is now detected — the only remaining gap is that detection is a panic rather than a recoverable error.
Generated by Claude Code
There was a problem hiding this comment.
Correcting the last paragraph of this comment, and the equivalent line in my review summary — I overstated it.
I claimed the old offset + self.data.len().as_() was a latent bug that this change fixes. The mechanism is real (arithmetic in O's type, so VarBinBuilder<i32> wraps negative in release rather than panicking as it would in debug), but it wasn't reachable. append_values had exactly one caller before this PR — filter.rs:157 — and that caller is size-constrained:
builder: &mut VarBinBuilder<O>takesOfrommatch_each_integer_ptype!(offsets.ptype(), ..), i.e.Ois the input array's offset type;- filter selects a subset, so cumulative output bytes ≤ input total bytes;
- the input's total bytes ≤
O::MAXby construction, since its own final offset is anO.
So data.len() + relative_end could never exceed O::MAX there. Defensive hardening, not a fix — apologies for the inflation.
What I should have said is that the guard matters because of the call sites this PR adds, which are not subset-constrained: to_arrow_byte_array picks i32 whenever the target is pa.string() regardless of source size, and the Dict path deliberately builds i64 values then narrows into a possibly-i32 target. That's exactly where the overflow becomes reachable, so doing the arithmetic in usize first is the right call — it just needs to surface as a VortexError rather than a panic, which is the substance of the comment above and unaffected by this correction.
Generated by Claude Code
| _ => unreachable!("VarBinViewArray must have Utf8 or Binary dtype"), | ||
| }?; | ||
| arrow_cast::cast(&binary_view, &T::DATA_TYPE).map_err(VortexError::from) | ||
| let mut builder = DynVarBinBuilder::with_capacity( |
There was a problem hiding this comment.
The new arrow_type= / schema= parameters can reach a bare assert!.
execute_arrow_naive dispatches purely on the target Arrow DataType (executor/mod.rs:151-154) and neither it nor execute_arrow checks the target against the array's dtype. So a non-byte source falls past the source_is_utf8 != target_is_utf8 guard on line 41 and lands in DynVarBinBuilder::with_capacity, whose first line is assert!(matches!(dtype, DType::Utf8(_) | DType::Binary(_))).
vx.array([1, 2, 3]).to_arrow_array(arrow_type=pa.binary()) # panicsI checked this against develop first — the panic is pre-existing internally, and all 12 dtype×target combinations I tried panic there too. But on develop a Python caller had no way to select the target type, so this PR is what makes it reachable. Worth a vortex_bail! here when array.dtype() isn't Utf8/Binary, or validation at the Python boundary.
Unrelated nit while you're in with_capacity: that assert! has no message, whereas the equivalent in VarBinViewBuilder::new reads "VarBinViewBuilder DType must be Utf8 or Binary." Worth matching.
Generated by Claude Code
| ) | ||
| }) | ||
| })); | ||
| debug_assert_eq!(len, validity.len()); |
There was a problem hiding this comment.
finish builds via new_unchecked with no length check — worth adding one while you're here.
VarBinArray::new_unchecked derives len = offsets.len().saturating_sub(1), so if offsets and validity desync you get an array whose len() disagrees with its validity mask, silently in release.
This is pre-existing, and the PR improves it — the old append_values had no checks at all, so a mismatched num desynced in every build profile; these two debug_assert_eq! at least catch it in debug. But VarBinViewBuilder::finish_into_varbinview already asserts this unconditionally (builders/varbinview.rs:239-243, "View and validity length must match"), and VarBinBuilder::finish doesn't. Since this PR adds a second public append path into the same builder, the asymmetry is more likely to bite now.
Cheapest fix that matches the existing convention — in finish, before new_unchecked:
assert_eq!(
self.offsets.len() - 1,
self.validity.len(),
"Offset and validity length must match"
);offsets is never empty (with_capacity pushes a leading 0), so len() - 1 is safe. That covers the pre-existing append_values path too, which a check inside this method wouldn't.
Non-blocking — happy for this to be a follow-up if you'd rather keep the PR tight.
Generated by Claude Code
| builder: &mut dyn ArrayBuilder, | ||
| ctx: &mut ExecutionCtx, | ||
| ) -> VortexResult<()> { | ||
| if let Some(builder) = builder.as_any_mut().downcast_mut::<DynVarBinBuilder>() { |
There was a problem hiding this comment.
This branch preempts the existing dict fast path and drops its guards.
It runs whenever the builder is a DynVarBinBuilder, ahead of the take_canonical path at line 240 — and skips both of that path's short-circuits (!array.is_empty(), !codes.validity()?.definitely_all_null()). execute has the same two guards at lines 190 and 199, so this is the one place in the vtable without them.
It also does three full copies of the expanded data: decode all values into an i64 VarBin, take to full logical length (fresh offsets + bytes), then append_varbin into the target. For a dict that's the case the encoding exists to make cheap.
Separately, <VarBin as TakeExecute>::take is called directly, bypassing precondition(), then assumed to be Some and specifically VarBin. Both hold today — I traced every return path and they all build a VarBinArray — but the signature is Option<ArrayRef> and the doc says "The indices are guaranteed to be non-empty," which this call site violates for empty dicts. It'll break quietly if VarBin's take ever gains a constant-folding or view-returning fast path.
I tested the empty and all-null-codes cases and they happen to produce correct results, so this is brittleness rather than a live bug — but reusing the existing path (or at least its guards) seems preferable to duplicating it.
Generated by Claude Code
| } | ||
| Mask::AllFalse(len) => self.push_nulls(*len), | ||
| Mask::Values(values) => { | ||
| for (index, is_valid) in values.bit_buffer().iter().enumerate() { |
There was a problem hiding this comment.
Per-call overhead in the fallback path.
This is the path every encoding without a direct branch takes — Sparse, RunEnd, and anything on the VTable::append_to_builder default, which canonicalizes to VarBinView and lands here. The loop itself is irreducible (view→offset conversion has to gather scattered bytes), but three things inside it are avoidable:
values.bit_buffer().iter().enumerate()walks validity a bit at a time.BitBuffer::for_each_set_index(vortex-buffer/src/bit/buf.rs:450) plus null gap-filling is the same shape your ZstdAllOr::Some(valid_indices)arm already uses — pervortex-mask/benches/mask_iteration.rsit's 2-45x on the iteration itself.bytes_at(index)returns an ownedByteBufferper row whenappend_valueonly needs&[u8]. The inlined branch is the expensive one (views_handle().as_host().clone().into_byte_buffer().slice_ref(..)) and inlined means ≤12 bytes — the common short-string case. Hoistingviews()and the buffer list and slicing directly avoids the per-row refcount and slice construction.- The byte total is computable up front from the view lengths in one pass over the fixed-width views, so
self.datacould be sized exactly instead of reallocating as it grows.
Your benchmark reports Sparse gaining 55%; I'd expect a chunk of the remainder to be here. Non-blocking.
Generated by Claude Code
Signed-off-by: 蔡略 <cailue@apache.org>
robert3005
left a comment
There was a problem hiding this comment.
Thanks for the contribution. There's couple of follow ups that I have found while reviewing this code but they're independent of your changes.
Summary
This PR adds direct export to Arrow
StringArrayandBinaryArray.A caller can request one of these Arrow types:
Utf8LargeUtf8BinaryLargeBinaryVortex builds a
VarBinArrayfor the requested Arrow type. Arrow uses the buffers in theVarBinArray.The Python API accepts the requested Arrow type:
Vortex still exports
StringViewArrayandBinaryViewArraywhen the caller does not request a type or schema.Direct export paths
These encodings can write directly to the
VarBinArraybuilder:The Dict path also supports compressed dictionary values. Examples include
Dict<FSST>andDict<Zstd>.Sparse and RunEnd use the normal canonical path. The Rust benchmark showed a 55% gain for the Sparse canonical path. The Vortex compressor does not create RunEnd string arrays.
Reason for the change
The old path created a
StringViewArrayorBinaryViewArray. Arrow then cast the result to aStringArrayorBinaryArray.The cast copied or reordered the value bytes.
The new path creates a
StringArrayorBinaryArraydirectly.Performance
This Python benchmark used 500,000 repeated URL strings. Each result is the median of 10 runs.
StringArrayStringViewArray, then cast toStringArrayDirect export was 59.4% faster in this benchmark.
The Rust benchmark covers these layouts and encodings:
Dict<FSST>Dict<Zstd>Chunked<FSST>The Filter cases do not use the direct encoding paths.
Chunked<FSST>uses the direct FSST path. Chunked passes the output builder to each child.AI assistance
I used GPT5.6sol to help develop this change. I reviewed and tested all changes.
Verification
cargo nextest run -p vortex-arrow -p vortex-fsst -p vortex-onpair -p vortex-runend -p vortex-sparse -p vortex-zstdpassed 529 tests.cargo nextest run -p vortex-array --no-fail-fastpassed 3,003 tests and skipped 1 test.cargo clippy -p vortex-arrow -p vortex-sparse -p vortex-runend --all-targets --all-features -- -D warningspassed.cargo +nightly fmt --allpassed.git diff --checkpassed.Coverage
Rust coverage used LLVM branch instrumentation for
vortex-array,vortex-arrow,vortex-fsst,vortex-onpair,vortex-runend,vortex-sparse, andvortex-zstd.The original targeted run passed 3,550 tests and skipped 2 tests. After adding focused tests, this command refreshed coverage for the changed
vortex-arraypaths:The refreshed run passed 3,003 tests and skipped 1 test.
The changed-line results compare this branch with
57962f4df9af99ebad584bf3e59310e4bf552fc1. They intersect added lines with the LLVM LCOV line and branch records. The report combines the refreshedvortex-arrayLCOV with the existing LCOV for the unchanged encoding and Arrow crates. This Rust scope does not include benchmarks or the PyO3 crate.Python branch coverage used the changed array and file test modules.
The run completed with 19 passed, 1 pre-existing xfail (
test_scalar_at_out_of_bounds), and 0 failed.vortex/file.py: 32 of 47 statements, or 68.09%.vortex/file.py: 0 of 2 branches. Both branches are in unchanged code.Python coverage does not instrument the Rust code in the extension. The Python tests exercise that code through the public API.
Next steps
Pass the output builder through lazy arrays such as Filter, Slice, and Take. This change can remove more intermediate canonical arrays. Track this work in #7674.