Add TakeSlices for fixed-size-list take#8674
Conversation
Merging this PR will degrade performance by 53.71%
Warning Please fix the performance issues or acknowledge them on CodSpeed. Performance Changes
Tip Investigate this regression by commenting Comparing Footnotes
|
0908aec to
a039028
Compare
a039028 to
949cfcb
Compare
|
Claude finished @danking's task in 4m 27s —— View job PR Review: Add
|
Polar Signals Profiling ResultsLatest Run
Powered by Polar Signals Cloud |
|
Codex Review: Didn't find any major issues. What shall we delve into next? Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Benchmarks: PolarSignals ProfilingVortex (geomean): 0.067x ✅ How to read Verdict and Engines
datafusion / vortex-file-compressed (0.067x ✅, 10↑ 0↓)
File Size Changes (1 files changed, -100.0% overall, 0↑ 1↓)
Totals:
|
Benchmarks: TPC-H SF=1 on NVMEVerdict: No clear signal (environment too noisy confidence) How to read Verdict and Engines
datafusion / vortex-file-compressed (1.023x ➖, 0↑ 1↓)
datafusion / vortex-compact (1.017x ➖, 0↑ 1↓)
datafusion / parquet (0.996x ➖, 1↑ 1↓)
datafusion / arrow (1.004x ➖, 0↑ 1↓)
duckdb / vortex-file-compressed (1.000x ➖, 0↑ 0↓)
duckdb / vortex-compact (1.002x ➖, 0↑ 0↓)
duckdb / parquet (1.008x ➖, 1↑ 2↓)
duckdb / duckdb (0.999x ➖, 0↑ 0↓)
File Size Changes (10 files changed, -0.1% overall, 6↑ 4↓)
Totals:
|
Benchmarks: FineWeb NVMeVerdict: No clear signal (low confidence) How to read Verdict and Engines
datafusion / vortex-file-compressed (0.993x ➖, 1↑ 1↓)
datafusion / vortex-compact (1.001x ➖, 0↑ 0↓)
datafusion / parquet (1.002x ➖, 0↑ 0↓)
duckdb / vortex-file-compressed (1.000x ➖, 0↑ 0↓)
duckdb / vortex-compact (0.990x ➖, 0↑ 0↓)
duckdb / parquet (1.005x ➖, 0↑ 0↓)
File Size Changes (1 files changed, +0.0% overall, 1↑ 0↓)
Totals:
|
🚨🚨🚨❌❌❌ SQL BENCHMARK FAILED ❌❌❌🚨🚨🚨Benchmark |
Benchmarks: TPC-DS SF=1 on NVMEVerdict: No clear signal (low confidence) How to read Verdict and Engines
datafusion / vortex-file-compressed (1.008x ➖, 0↑ 1↓)
datafusion / vortex-compact (1.000x ➖, 0↑ 0↓)
datafusion / parquet (0.998x ➖, 3↑ 1↓)
duckdb / vortex-file-compressed (1.000x ➖, 0↑ 0↓)
duckdb / vortex-compact (0.998x ➖, 0↑ 0↓)
duckdb / parquet (0.999x ➖, 1↑ 0↓)
duckdb / duckdb (0.994x ➖, 0↑ 0↓)
File Size Changes (6 files changed, +0.0% overall, 5↑ 1↓)
Totals:
|
Introduce a lazy TakeSlices array for ordered child ranges, wire FixedSizeList take to emit element ranges, and add a primitive child execution kernel for contiguous range copies. Expand FSL take benchmarks and cover TakeSlices edge cases, nullable validity, generic slice-based execution, and nullable FSL take with nullable indices. Signed-off-by: Daniel King <dan@spiraldb.com>
Refactor fixed-size-list take around structural cases, tighten TakeSlices validation, and extend edge-case coverage. Also document the local-reasoning guideline that would have avoided threading fallback state through the main path. Signed-off-by: Daniel King <dan@spiraldb.com>
Replace ambiguous ordered-range wording with caller-provided sequence terminology so the API docs do not imply sorted or disjoint ranges. Signed-off-by: Daniel King <dan@spiraldb.com>
Switch TakeSlices to starts/lengths selector arrays and reuse the contiguous-run gather for ListView rebuilds. Signed-off-by: Daniel King <dan@spiraldb.com>
Benchmarks: FineWeb S3Verdict: No clear signal (environment too noisy confidence) How to read Verdict and Engines
datafusion / vortex-file-compressed (1.009x ➖, 0↑ 0↓)
datafusion / vortex-compact (0.961x ➖, 0↑ 0↓)
datafusion / parquet (1.004x ➖, 0↑ 0↓)
duckdb / vortex-file-compressed (0.940x ➖, 0↑ 0↓)
duckdb / vortex-compact (1.045x ➖, 0↑ 1↓)
duckdb / parquet (1.043x ➖, 0↑ 0↓)
|
Push TakeSlices through struct fields and execute VarBinView run gathers by copying view ranges while reusing data buffers. Signed-off-by: Daniel King <dan@spiraldb.com>
f80606e to
99d0126
Compare
Benchmarks: Clickbench Sorted on NVMEVerdict: No clear signal (low confidence) How to read Verdict and Engines
datafusion / vortex-file-compressed (1.005x ➖, 1↑ 1↓)
datafusion / parquet (0.991x ➖, 0↑ 0↓)
duckdb / vortex-file-compressed (0.950x ➖, 3↑ 0↓)
duckdb / parquet (1.002x ➖, 0↑ 0↓)
duckdb / duckdb (0.997x ➖, 0↑ 0↓)
File Size Changes (201 files changed, +0.0% overall, 103↑ 98↓)
Totals:
|
Benchmarks: TPC-H SF=10 on NVMEVerdict: No clear signal (low confidence) How to read Verdict and Engines
datafusion / vortex-file-compressed (1.046x ➖, 0↑ 1↓)
datafusion / vortex-compact (1.017x ➖, 0↑ 1↓)
datafusion / parquet (1.005x ➖, 0↑ 0↓)
datafusion / arrow (1.004x ➖, 0↑ 0↓)
duckdb / vortex-file-compressed (1.007x ➖, 0↑ 0↓)
duckdb / vortex-compact (1.003x ➖, 0↑ 0↓)
duckdb / parquet (0.999x ➖, 0↑ 0↓)
duckdb / duckdb (0.990x ➖, 0↑ 0↓)
File Size Changes (27 files changed, -0.0% overall, 13↑ 14↓)
Totals:
|
Benchmarks: Random AccessVortex (geomean): 1.003x ➖ How to read Verdict and Engines
unknown / unknown (1.002x ➖, 0↑ 0↓)
|
Clarify signed fixed-size-list index reinterpretation, remove redundant selector length validation, and hoist ListView validity during rebuild. Signed-off-by: Daniel King <dan@spiraldb.com>
Benchmarks: Clickbench on NVMEVerdict: No clear signal (low confidence) How to read Verdict and Engines
datafusion / vortex-file-compressed (0.987x ➖, 0↑ 2↓)
datafusion / parquet (0.986x ➖, 0↑ 2↓)
duckdb / vortex-file-compressed (1.012x ➖, 1↑ 6↓)
duckdb / parquet (0.994x ➖, 0↑ 0↓)
duckdb / duckdb (0.997x ➖, 0↑ 0↓)
File Size Changes (104 files changed, +0.0% overall, 51↑ 53↓)
Totals:
|
Add explicit coverage that nullable take indices over a non-nullable fixed-size-list produce nullable output rows. Signed-off-by: Daniel King <dan@spiraldb.com>
Benchmarks: TPC-H SF=1 on S3Verdict: No clear signal (environment too noisy confidence) How to read Verdict and Engines
datafusion / vortex-file-compressed (0.994x ➖, 1↑ 3↓)
datafusion / vortex-compact (1.132x ➖, 0↑ 1↓)
datafusion / parquet (1.017x ➖, 0↑ 5↓)
duckdb / vortex-file-compressed (1.022x ➖, 0↑ 0↓)
duckdb / vortex-compact (1.017x ➖, 0↑ 0↓)
duckdb / parquet (0.986x ➖, 0↑ 0↓)
|
Benchmarks: Appian on NVMEVerdict: No clear signal (low confidence) How to read Verdict and Engines
datafusion / vortex-file-compressed (1.075x ➖, 0↑ 3↓)
datafusion / parquet (0.977x ➖, 0↑ 0↓)
duckdb / vortex-file-compressed (1.003x ➖, 0↑ 0↓)
duckdb / parquet (1.060x ➖, 0↑ 3↓)
duckdb / duckdb (1.098x ➖, 0↑ 3↓)
File Size Changes (4 files changed, -0.0% overall, 1↑ 3↓)
Totals:
|
Benchmarks: TPC-H SF=10 on S3Verdict: No clear signal (environment too noisy confidence) How to read Verdict and Engines
datafusion / vortex-file-compressed (1.003x ➖, 0↑ 0↓)
datafusion / vortex-compact (1.055x ➖, 0↑ 1↓)
datafusion / parquet (1.067x ➖, 0↑ 2↓)
duckdb / vortex-file-compressed (0.991x ➖, 0↑ 0↓)
duckdb / vortex-compact (0.929x ➖, 0↑ 0↓)
duckdb / parquet (1.027x ➖, 0↑ 0↓)
|
Benchmarks: CompressionVortex (geomean): 0.991x ➖ How to read Verdict and Engines
unknown / unknown (1.024x ➖, 2↑ 18↓)
|
Validate existing converted Vortex files against the source Parquet footer row count before reusing them. Regenerate files that are unreadable or have stale row counts so benchmark data caches cannot preserve empty Vortex artifacts. Signed-off-by: Daniel King <dan@spiraldb.com>
c96c771 to
4285b93
Compare
Add context-aware TakeSlices construction and validity paths so callers with an ExecutionCtx do not fall back to legacy session execution. Centralize run selector validation/output length in RunSelectors and use it from execution kernels.\n\nClarify that TakeSlices remains a lazy non-serializable compute encoding like Slice, remove the unrelated AGENTS.md guidance change, tighten Struct length derivation, document the FSL null-placeholder run invariant, and rename the benchmark cache helper to describe its row-count-only check. Signed-off-by: Daniel King <dan@spiraldb.com>
| let len = selector_output_len_with_ctx(child.len(), &starts, &lengths, ctx)?; | ||
| Ok(Self::new_validated(child, starts, lengths, len)) |
There was a problem hiding this comment.
I don't think we should do this validation on construction. As long as starts, lengths are correct size then starts being out of bounds should be an execution error
| check_selector_arrays(starts, lengths)?; | ||
| let starts = starts.clone().execute::<PrimitiveArray>(ctx)?; | ||
| let lengths = lengths.clone().execute::<PrimitiveArray>(ctx)?; |
There was a problem hiding this comment.
this really should account for constant arrays, that's what you end up with for fixedsizelist
Signed-off-by: Daniel King <dan@spiraldb.com>
Signed-off-by: Daniel King <dan@spiraldb.com>
Signed-off-by: Daniel King <dan@spiraldb.com>
Signed-off-by: Daniel King <dan@spiraldb.com>
The key issue here is that
fixed_size_list.take(indices)explodes the indices into one index per list element. For a fixed-size list of length 4096, this means the number of take indices expands by three orders of magnitude. Moreover, the take kernel must either copy each index separately or reconstruct the fact that these are actually contiguous ranges so it can use range copies.This PR now models that operation directly as a gather of contiguous runs:
starts[i]andlengths[i]describe each child run, and the output is the concatenation ofvalues[starts[i]..starts[i] + lengths[i]]. The runs may overlap, repeat, and appear in any order; this is a gather of runs, not a normal slice.For
FixedSizeList<100, u8>, this:now becomes run selectors over the child elements:
instead of expanding to:
This also gives
ListView -> Listpacking a direct fast path:ListViewrebuild now uses original offsets as starts and normalized sizes as lengths, so it can gather the referenced element runs without building a flat per-element take index buffer or falling back to per-list builders.Structnow pushes those run selectors into each field, andVarBinViewnow executes run gathers by copying contiguous view ranges while reusing the original data buffers.This is a table of results for
FixedSizeList<N, f16>:per-index / take_slicesCodex:
Summary
TakeSlicesArrayfrom aVec<Range<usize>>constructor to selector arrays: non-nullable unsignedstartsandlengths.FixedSizeListtake through contiguous child runs viatake_slices, leaving child encodings responsible for optimization.TakeSlicesExecutekernel that copies each requested run directly from the primitive slice.StructTakeSlicesReduceso run gathers push down into each field, matching the existingtakeshape.VarBinViewTakeSlicesExecuteso run gathers copy view ranges directly and reuse backing data buffers.take_slicesto rebuild non-zero-copyListViewArraylayouts into packed, zero-copy-to-list layouts.take_fslbenchmarks andtake_slicescoverage for selector arrays, overlapping/repeated runs, empty runs, nestedTakeSlices, nullable validity, generic fallback, primitive execution, Struct pushdown, VarBinView view-buffer execution, and FSL take.Correctness and Coverage
TakeSlicesvalidates public construction selectors before creating the lazy array.TakeSlicesexecution repeatedly slices the child and appends each run; specialized children can bypass that path.ListViewrebuild zeroes null-list lengths before gathering elements, preserving packed offsets without copying invalid-row payloads.Structpreserves struct-level validity while applying the same run selectors to every unmasked field.VarBinViewpreserves validity and shares the original data-bufferArcwhile copying only selected views.Validation
cargo +nightly fmt --allcargo test -p vortex-array take_slicescargo test -p vortex-array fixed_size_list::tests::takecargo test -p vortex-array listviewcargo check -p vortex-array --all-targetscargo clippy --all-targets --all-featuresgit diff --check