feat: add ListLayout#8071
Draft
mhk197 wants to merge 37 commits into
Draft
Conversation
1 task
mhk197
force-pushed
the
mk/add-list-layout
branch
from
2dafcdb to
b267f99
Compare
1 task
connortsui20
requested review from
gatesn and
robert3005
and removed request for
connortsui20
| session: VortexSession, | ||
| elements: LayoutReaderRef, | ||
| offsets: LayoutReaderRef, | ||
| validity: Option<LayoutReaderRef>, |
Contributor
There was a problem hiding this comment.
Are we sure this needs a layout?
Contributor
There was a problem hiding this comment.
You can probably can by with this being a segment
| offsets, | ||
| validity, | ||
| .. | ||
| } = list_from_list_view(array.execute::<ListViewArray>(&mut exec_ctx)?)?.into_data_parts(); |
Contributor
There was a problem hiding this comment.
Why to execute to a ListArray?
Comment on lines
+34
to
+42
| /// Strategy for writing list-typed arrays. | ||
| /// | ||
| /// Single-chunk only. The strategy: | ||
| /// 1. Canonicalizes the input chunk into a [`ListViewArray`]. | ||
| /// 2. Calls [`list_from_list_view`] to rebuild it into zero-copy-to-list form | ||
| /// (sorted, gapless, non-overlapping offsets) and produce a [`ListArray`]. | ||
| /// 3. Writes the `elements`, `offsets`, and (when nullable) `validity` columns into | ||
| /// separately configurable downstream strategies, producing a single [`ListLayout`]. | ||
| /// |
Contributor
There was a problem hiding this comment.
How does the sizing of the splits works, what is it based on?
Contributor
There was a problem hiding this comment.
We just talked through this. This is the layout that really highlights why row-batches suck... and really why engines should be pushing down unnest operations into the source. But... they don't.
For now, the best we can do is register the validity and offsets splits, so we just delegate to the child.
Member
|
Is there a tracking issue that explains why we do not want to store sizes as a (potentially optional) layout child? It would be nice to see some sort of design doc (or if that already exists, please attach it in the PR description). |
joseph-isaacs
added
the
action/benchmark
github-actions
Bot
removed
the
action/benchmark
Contributor
Polar Signals Profiling ResultsLatest Run
Previous Runs (3)
Powered by Polar Signals Cloud |
connortsui20
left a comment
connortsui20
left a comment
Member
There was a problem hiding this comment.
Mostly nits, didn't get a chance to read through writer.rs though
Comment on lines
+266
to
+270
| pub fn new(offsets_ptype: PType) -> Self { | ||
| let mut metadata = Self::default(); | ||
| metadata.set_offsets_ptype(offsets_ptype); | ||
| metadata | ||
| } |
Member
There was a problem hiding this comment.
This seems like a strange constructor? Why not just do Self { offsets_ptype }?
Contributor
Author
There was a problem hiding this comment.
conversion from enum to i32 in prost needs this apparently, see DictLayout
| // Bound the elements read using offsets[0] and offsets[-1] | ||
| let elements_range = calculate_elements_range(&offsets, &session)?; | ||
|
|
||
| // Rebase the offsets so they start at zero |
Member
There was a problem hiding this comment.
Nit: prose comments should end in periods (ask an llm to do this for you!).
Comment on lines
+241
to
+245
| let array = if !mask.all_true() { | ||
| array.filter(mask)? | ||
| } else { | ||
| array | ||
| }; |
Member
There was a problem hiding this comment.
@joseph-isaacs I feel like we should have this optimization in the .filter method for arrays instead of constructing the filter array and then immediately optimizing it away?
Basically for the "built-in" array methods we might as well do the optimizations immediately.
mhk197
force-pushed
the
mk/add-list-layout
branch
2 times, most recently
from
fcd1059 to
e94f42c
Compare
github-actions
Bot
removed
the
action/benchmark
mhk197
added
action/benchmark-sql
Contributor
Benchmarks: PolarSignals Profiling (base)Vortex (geomean): 1.161x ❌ How to read Verdict and Engines
datafusion / vortex-file-compressed (1.161x ❌, 0↑ 5↓)
File Size Changes (1 files changed, +0.0% overall, 1↑ 0↓)
Totals:
|
Merging this PR will improve performance by 12.58%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | Simulation | chunked_varbinview_canonical_into[(1000, 10)] |
154.8 µs | 191 µs | -18.98% |
| ❌ | Simulation | slice_empty_vortex |
310 ns | 368.3 ns | -15.84% |
| ⚡ | Simulation | chunked_bool_canonical_into[(1000, 10)] |
26.7 µs | 16.1 µs | +65.74% |
| ⚡ | Simulation | bitwise_not_vortex_buffer_mut[128] |
273.6 ns | 215.3 ns | +27.1% |
| ⚡ | Simulation | bitwise_not_vortex_buffer_mut[1024] |
333.9 ns | 275.6 ns | +21.17% |
| ⚡ | Simulation | bitwise_not_vortex_buffer_mut[2048] |
427.8 ns | 369.4 ns | +15.79% |
| ⚡ | Simulation | chunked_varbinview_canonical_into[(100, 100)] |
259.1 µs | 223.8 µs | +15.77% |
| ⚡ | Simulation | chunked_varbinview_into_canonical[(100, 100)] |
305.7 µs | 270.8 µs | +12.88% |
| ⚡ | Simulation | eq_i64_constant |
318.1 µs | 288.4 µs | +10.31% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing mk/add-list-layout (fa581c4) with develop (4668b6e)
Footnotes
-
4 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
Contributor
Benchmarks: TPC-H SF=1 on NVME (base)Verdict: No clear signal (environment too noisy confidence) How to read Verdict and Engines
datafusion / vortex-file-compressed (1.008x ➖, 0↑ 0↓)
datafusion / parquet (1.017x ➖, 2↑ 3↓)
datafusion / arrow (1.000x ➖, 0↑ 1↓)
duckdb / vortex-file-compressed (1.052x ➖, 0↑ 3↓)
duckdb / parquet (1.025x ➖, 0↑ 3↓)
File Size Changes (18 files changed, -44.5% overall, 3↑ 15↓)
Totals:
|
Contributor
Benchmarks: FineWeb NVMe (base)Verdict: No clear signal (low confidence) How to read Verdict and Engines
datafusion / vortex-file-compressed (1.019x ➖, 0↑ 0↓)
datafusion / parquet (0.940x ➖, 3↑ 0↓)
duckdb / vortex-file-compressed (1.024x ➖, 0↑ 1↓)
duckdb / parquet (1.002x ➖, 0↑ 0↓)
File Size Changes (3 files changed, -46.3% overall, 0↑ 3↓)
Totals:
|
Contributor
Benchmarks: TPC-DS SF=1 on NVME (base)Verdict: No clear signal (low confidence) How to read Verdict and Engines
datafusion / vortex-file-compressed (1.070x ➖, 1↑ 23↓)
datafusion / parquet (1.030x ➖, 0↑ 7↓)
duckdb / vortex-file-compressed (1.042x ➖, 3↑ 20↓)
duckdb / parquet (1.026x ➖, 0↑ 4↓)
File Size Changes (36 files changed, -43.5% overall, 1↑ 35↓)
Totals:
|
Contributor
Benchmarks: FineWeb S3 (base)Verdict: No clear signal (environment too noisy confidence) How to read Verdict and Engines
datafusion / vortex-file-compressed (0.953x ➖, 0↑ 0↓)
datafusion / parquet (0.861x ➖, 2↑ 1↓)
duckdb / vortex-file-compressed (0.951x ➖, 0↑ 1↓)
duckdb / parquet (0.960x ➖, 0↑ 0↓)
|
Contributor
Benchmarks: Statistical and Population Genetics (base)Verdict: No clear signal (low confidence) How to read Verdict and Engines
duckdb / vortex-file-compressed (0.964x ➖, 1↑ 0↓)
duckdb / parquet (0.993x ➖, 0↑ 0↓)
File Size Changes (3 files changed, -32.3% overall, 1↑ 2↓)
Totals:
|
Contributor
Benchmarks: Clickbench Sorted on NVME (base)Verdict: No clear signal (low confidence) How to read Verdict and Engines
datafusion / vortex-file-compressed (0.941x ➖, 3↑ 0↓)
datafusion / parquet (1.027x ➖, 0↑ 1↓)
duckdb / vortex-file-compressed (0.959x ➖, 3↑ 0↓)
duckdb / parquet (1.001x ➖, 0↑ 0↓)
File Size Changes (201 files changed, -42.6% overall, 50↑ 151↓)
Totals:
|
Contributor
Benchmarks: Clickbench on NVME (base)Verdict: No clear signal (low confidence) How to read Verdict and Engines
datafusion / vortex-file-compressed (0.935x ➖, 9↑ 0↓)
datafusion / parquet (0.961x ➖, 1↑ 0↓)
duckdb / vortex-file-compressed (0.912x ➖, 12↑ 1↓)
duckdb / parquet (0.988x ➖, 0↑ 0↓)
File Size Changes (201 files changed, -39.1% overall, 57↑ 144↓)
Totals:
|
Contributor
Benchmarks: TPC-H SF=10 on NVME (base)Verdict: No clear signal (low confidence) How to read Verdict and Engines
datafusion / vortex-file-compressed (0.981x ➖, 2↑ 1↓)
datafusion / parquet (0.991x ➖, 0↑ 0↓)
datafusion / arrow (0.997x ➖, 0↑ 0↓)
duckdb / vortex-file-compressed (1.011x ➖, 0↑ 0↓)
duckdb / parquet (0.988x ➖, 0↑ 0↓)
File Size Changes (48 files changed, -44.5% overall, 10↑ 38↓)
Totals:
|
Contributor
Benchmarks: TPC-H SF=1 on S3 (base)Verdict: No clear signal (environment too noisy confidence) How to read Verdict and Engines
datafusion / vortex-file-compressed (0.866x ➖, 6↑ 4↓)
datafusion / parquet (0.716x ➖, 12↑ 2↓)
duckdb / vortex-file-compressed (0.942x ➖, 1↑ 0↓)
duckdb / parquet (0.916x ➖, 0↑ 0↓)
|
github-actions
Bot
removed
the
action/benchmark-sql
github-actions
Bot
removed
the
action/benchmark-sql
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
5 participants
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.
Adds
ListLayout, a structured layout forDType::Listcolumns that storeselements,offsets, and (when nullable)validityas independent child layouts, each with its own configurable strategy.ListLayoutStrategycanonicalizes input toListViewArray, rebuilds it vialist_from_list_view, and writes each child concurrently.ListReaderfetchesoffsetsandvalidityconcurrently. Then usesoffsets[0]..offsets[-1]to derive a bounded elements range, rebase the offsets to start at zero, fetch only that slice ofelements, and assemble theListArray. This means partial-range reads only fetch the elements they actually need rather than the whole elements buffer.