viirya commented on PR #11279: URL: https://github.com/apache/arrow-rs/pull/11279#issuecomment-5936029851
Thanks for the PR. A couple of things I ran into when checking it against the existing `interleave` + `gc()` path: - I benchmarked this against `interleave(...)` followed by `.gc()` (4 sources × 8192 rows, random indices, release build). At 8192 selected rows the compact path is ~1.3–1.8x slower across 13–20, 20–60 and 100–400 byte strings, and only ~10% faster at 512 rows. The output buffer capacity is identical in every case, since `gc()` already sizes a single exact buffer. Per-value `try_append_value` plus the second random pass over the views seems to outweigh skipping the intermediate views. Given that, what does this give us over `interleave` + `gc` apart from skipping payload under nulls? This also relates to @cetra3's question. - `bytes < u32::MAX` lets a single block grow to ~4 GiB, so offsets can exceed `i32::MAX`. `gc()` deliberately caps a buffer at `i32::MAX` and splits beyond that, and other implementations (e.g. C++ `BinaryViewType::c_type`) treat the offset as `int32`. The comment "Larger outputs use the builder's normal growth" also doesn't match outputs between 2 MiB and 4 GiB, which get one large fixed block. If we keep this, I'd cap at `i32::MAX` or reuse `gc`'s grouping. - Repeating a value copies it each time (10k selections of a 1 KiB value → 10 MB vs 1 KiB shared). For the join-spill use case this is the apache/datafusion#23564 shape. A cheap middle ground could be reusing the output view when the same source `(array, buffer_index, offset, len)` repeats, which avoids hashing the contents. - If the direct-copy idea holds up, it might fit better inside the existing `interleave`/`gc` machinery (an indices-driven variant reusing `copy_view_to_buffer`), or behind the options/struct approach @alamb suggested, rather than another free function. That would also cover the >2 GiB grouping for free. - Minor: the existing interleave tests live in `mod tests` in `interleave.rs`; I'd put these there too. A benchmark in `benches/interleave_kernels.rs` would help settle the performance question. -- 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]
