perf(arrow): build repeated constant string/binary columns without per-row clones - #3080
perf(arrow): build repeated constant string/binary columns without per-row clones#3080anoopj wants to merge 1 commit into
Conversation
…r-row clones create_primitive_array_repeated built a throwaway `vec![value.clone(); num_rows]` for the Utf8/Binary/LargeBinary/FixedSizeBinary arms before handing it to the array constructor. For strings that clones the value into num_rows separate heap allocations per batch; the binary arms allocate a throwaway intermediate Vec. Stream the single value straight into the Arrow buffer via `from_iter_values` with `std::iter::repeat_n` instead. Benchmarks (release, throwaway harness, old vs new toggled on this file only): - leaf create_primitive_array_repeated (Utf8, 8192-row batch): 23.0 -> 4.1 ns/row (5.6x) - full scan select(x, _partition) over a real manifest + 262k-row parquet file, string-partitioned: 20.9 -> 5.7 ns/row (3.7x) Note that the gain is allocation-counts and scales with how much of the projection is the string partition column. Fixes apache#3079
laskoviymishka
left a comment
laskoviymishka
left a comment
There was a problem hiding this comment.
This is a nice!
One small non-blocking thing: the FixedSizeBinary arm. try_from_iter infers the fixed width from the data rather than the declared len, so an empty batch over a fixed[n] partition column looks like it'd come back typed FixedSizeBinary(0) and mismatch the declared schema. It's pre-existing, not something this PR introduced, but since the new empty-batch test already covers Utf8 it's a natural moment to extend it to fixed[n] and either prove it's fine or pin it down. Left the details inline.
One small description note: the Binary/LargeBinary arms weren't doing per-row allocations before — those were only replicating a &[u8] pointer, so the genuine per-row win is Utf8. Worth tightening the framing so the next person auditing this path isn't misled.
None of this blocks merge from my side.
Will soak for a day or two before merging
| LargeBinaryArray::from_iter_values(std::iter::repeat_n(value.as_slice(), num_rows)), | ||
| ), | ||
| (DataType::FixedSizeBinary(len), Some(PrimitiveLiteral::Binary(value))) => Arc::new( | ||
| FixedSizeBinaryArray::try_from_iter(std::iter::repeat_n(value.as_slice(), num_rows)) |
There was a problem hiding this comment.
I think there's a subtle edge worth nailing down while we're touching this arm. try_from_iter infers the fixed width from the data it's handed, not from the len we matched on — so with num_rows == 0 the iterator is empty and we get back a FixedSizeBinary(0) array no matter what the schema declared, and an empty batch over a fixed[n] partition column (any filter that drops all rows) would then hand a type-mismatched array to RecordBatch::try_new. Same story if a literal's length doesn't equal *len — len only feeds the error string today, so a wrong-width value silently produces the wrong type rather than erroring.
Both are pre-existing rather than introduced here, but the new empty-batch test only covers Utf8. Extending test_create_string_array_repeated_empty to FixedSizeBinary(4) + num_rows=0 and asserting data_type() would either prove it's fine or pin it down, and a value.len() == *len guard before try_from_iter would close the wrong-width case. wdyt?
2 participants
Which issue does this PR close?
Fixes #3079
What changes are included in this PR?
create_primitive_array_repeatedbuilt a throwawayvec![value.clone(); num_rows]for the Utf8/Binary/LargeBinary/FixedSizeBinary arms before handing it to the array constructor. For strings that clones the value into num_rows separate heap allocations per batch; the binary arms allocate a throwaway intermediate Vec.Stream the single value straight into the Arrow buffer via
from_iter_valueswithstd::iter::repeat_ninstead.Benchmarks (release, throwaway harness, old vs new toggled on this file only):
Note that the gain is allocation-counts and scales with how much of the projection is the string partition column.
Are these changes tested?
New + old tests