feat: add ListLayout#8071
Open
mhk197 wants to merge 21 commits into
Open
Conversation
Merging this PR will degrade performance by 16.52%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | Simulation | chunked_varbinview_opt_canonical_into[(1000, 10)] |
187.8 µs | 225 µs | -16.52% |
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 (4256223) with develop (7a228aa)
1 task
Signed-off-by: Matthew Katz <katz@spiraldb.com> Signed-off-by: Matt Katz <mhkatz97@gmail.com>
Signed-off-by: Matthew Katz <katz@spiraldb.com> Signed-off-by: Matt Katz <mhkatz97@gmail.com>
Signed-off-by: Matthew Katz <katz@spiraldb.com> Signed-off-by: Matt Katz <mhkatz97@gmail.com>
Signed-off-by: Matt Katz <mhkatz97@gmail.com>
Signed-off-by: Matt Katz <mhkatz97@gmail.com>
Signed-off-by: Matt Katz <mhkatz97@gmail.com>
Signed-off-by: Matt Katz <mhkatz97@gmail.com>
Signed-off-by: Matt Katz <mhkatz97@gmail.com>
Signed-off-by: Matt Katz <mhkatz97@gmail.com>
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
Comment on lines
+162
to
+166
| self.offsets | ||
| .register_splits(field_mask, split_range, splits)?; | ||
| if let Some(validity) = &self.validity { | ||
| validity.register_splits(field_mask, split_range, splits)?; | ||
| } |
Contributor
There was a problem hiding this comment.
This looks odd why would they be different?
Contributor
Author
There was a problem hiding this comment.
they may be chunked differently and offsets will have one extra entry
| 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.
Signed-off-by: Matt Katz <mhkatz97@gmail.com>
Contributor
|
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 (1)
Powered by Polar Signals Cloud |
Contributor
connortsui20
left a comment
connortsui20
left a comment
There was a problem hiding this comment.
Mostly nits, didn't get a chance to read through writer.rs though
| } | ||
|
|
||
| /// Validates expected number of children based on `dtype` | ||
| #[inline] |
Contributor
There was a problem hiding this comment.
We generally dont want to inline functions with branches, and also because this is a private function the compiler will likely inline this anyways.
inline is only useful for public functions that are small
Comment on lines
+180
to
+181
| vortex_ensure!( | ||
| n_children == expected, |
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 | ||
| } |
Contributor
There was a problem hiding this comment.
This seems like a strange constructor? Why not just do Self { offsets_ptype }?
Comment on lines
+77
to
+82
| let mut n = NUM_CHILDREN_NON_NULLABLE; | ||
| if layout.dtype.is_nullable() { | ||
| n += 1; | ||
| } | ||
|
|
||
| n |
Contributor
There was a problem hiding this comment.
I feel like I would prefer reading just an if branch and then return either NUM_CHILDREN_NON_NULLABLE or NUM_CHILDREN_NON_NULLABLE + 1?
| _expr: &Expression, | ||
| _mask: MaskFuture, | ||
| ) -> VortexResult<MaskFuture> { | ||
| todo!() |
| // 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 |
Contributor
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 | ||
| }; |
Contributor
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.
Contributor
Benchmarks: PolarSignals ProfilingVortex (geomean): 0.967x ➖ How to read Verdict and Engines
datafusion / vortex-file-compressed (0.967x ➖, 2↑ 0↓)
File Size Changes (545 files changed, -98.9% overall, 0↑ 545↓)
Totals:
|
Contributor
Benchmarks: FineWeb NVMeVerdict: No clear signal (low confidence) How to read Verdict and Engines
datafusion / vortex-file-compressed (1.001x ➖, 0↑ 1↓)
datafusion / vortex-compact (1.011x ➖, 0↑ 1↓)
datafusion / parquet (1.010x ➖, 0↑ 0↓)
duckdb / vortex-file-compressed (0.994x ➖, 1↑ 0↓)
duckdb / vortex-compact (1.010x ➖, 0↑ 0↓)
duckdb / parquet (1.021x ➖, 0↑ 0↓)
File Size Changes (542 files changed, -95.1% overall, 0↑ 542↓)
Totals:
Full attributed analysis
|
Contributor
Benchmarks: TPC-H SF=1 on NVMEVerdict: No clear signal (low confidence) How to read Verdict and Engines
datafusion / vortex-file-compressed (1.022x ➖, 0↑ 1↓)
datafusion / vortex-compact (1.000x ➖, 0↑ 0↓)
datafusion / parquet (1.004x ➖, 2↑ 2↓)
datafusion / arrow (1.019x ➖, 0↑ 1↓)
duckdb / vortex-file-compressed (1.012x ➖, 0↑ 0↓)
duckdb / vortex-compact (1.013x ➖, 0↑ 0↓)
duckdb / parquet (1.026x ➖, 0↑ 1↓)
duckdb / duckdb (1.012x ➖, 0↑ 0↓)
File Size Changes (526 files changed, -99.3% overall, 0↑ 526↓)
Totals:
Full attributed analysis
|
Contributor
Benchmarks: FineWeb S3Verdict: No clear signal (environment too noisy confidence) How to read Verdict and Engines
datafusion / vortex-file-compressed (1.110x ➖, 0↑ 2↓)
datafusion / vortex-compact (1.077x ➖, 0↑ 1↓)
datafusion / parquet (1.155x ➖, 0↑ 1↓)
duckdb / vortex-file-compressed (1.074x ➖, 0↑ 2↓)
duckdb / vortex-compact (1.042x ➖, 0↑ 1↓)
duckdb / parquet (1.009x ➖, 0↑ 0↓)
Full attributed analysis
|
Contributor
Benchmarks: Statistical and Population GeneticsVerdict: No clear signal (low confidence) How to read Verdict and Engines
duckdb / vortex-file-compressed (0.979x ➖, 0↑ 0↓)
duckdb / vortex-compact (0.995x ➖, 0↑ 0↓)
duckdb / parquet (0.977x ➖, 0↑ 0↓)
File Size Changes (542 files changed, -95.3% overall, 0↑ 542↓)
Totals:
Full attributed analysis
|
Contributor
Benchmarks: Random AccessVortex (geomean): 0.968x ➖ How to read Verdict and Engines
unknown / unknown (0.998x ➖, 1↑ 3↓)
|
Contributor
Benchmarks: TPC-H SF=10 on NVMEVerdict: No clear signal (low confidence) How to read Verdict and Engines
datafusion / vortex-file-compressed (0.812x ✅, 22↑ 0↓)
datafusion / vortex-compact (0.827x ✅, 22↑ 0↓)
datafusion / parquet (0.850x ✅, 20↑ 0↓)
datafusion / arrow (0.834x ✅, 22↑ 0↓)
duckdb / vortex-file-compressed (0.866x ✅, 18↑ 0↓)
duckdb / vortex-compact (0.869x ✅, 22↑ 0↓)
duckdb / parquet (0.929x ➖, 3↑ 0↓)
duckdb / duckdb (0.900x ➖, 11↑ 0↓)
File Size Changes (496 files changed, -92.4% overall, 0↑ 496↓)
Totals:
Full attributed analysis
|
Contributor
Benchmarks: Clickbench on NVMEVerdict: No clear signal (low confidence) How to read Verdict and Engines
datafusion / vortex-file-compressed (1.052x ➖, 0↑ 6↓)
datafusion / parquet (1.050x ➖, 0↑ 2↓)
duckdb / vortex-file-compressed (1.025x ➖, 1↑ 1↓)
duckdb / parquet (1.024x ➖, 0↑ 0↓)
duckdb / duckdb (1.040x ➖, 0↑ 1↓)
File Size Changes (345 files changed, -65.8% overall, 0↑ 345↓)
Totals:
Full attributed analysis
|
Contributor
Benchmarks: Appian on NVMEVerdict: No clear signal (low confidence) How to read Verdict and Engines
datafusion / vortex-file-compressed (0.928x ➖, 1↑ 0↓)
datafusion / parquet (0.928x ➖, 1↑ 0↓)
duckdb / vortex-file-compressed (0.959x ➖, 0↑ 0↓)
duckdb / parquet (0.948x ➖, 0↑ 0↓)
duckdb / duckdb (0.957x ➖, 0↑ 0↓)
File Size Changes (527 files changed, -98.7% overall, 0↑ 527↓)
Totals:
Full attributed analysis
|
Contributor
Benchmarks: TPC-H SF=1 on S3Verdict: No clear signal (environment too noisy confidence) How to read Verdict and Engines
datafusion / vortex-file-compressed (1.065x ➖, 1↑ 4↓)
datafusion / vortex-compact (1.055x ➖, 1↑ 4↓)
datafusion / parquet (1.130x ➖, 0↑ 8↓)
duckdb / vortex-file-compressed (0.981x ➖, 0↑ 0↓)
duckdb / vortex-compact (1.058x ➖, 0↑ 0↓)
duckdb / parquet (0.980x ➖, 0↑ 0↓)
Full attributed analysis
|
Contributor
Benchmarks: CompressionVortex (geomean): 0.994x ➖ How to read Verdict and Engines
unknown / unknown (1.013x ➖, 2↑ 9↓)
|
Contributor
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.049x ➖, 0↑ 1↓)
datafusion / vortex-compact (0.990x ➖, 0↑ 2↓)
datafusion / parquet (1.185x ➖, 1↑ 8↓)
duckdb / vortex-file-compressed (0.987x ➖, 0↑ 0↓)
duckdb / vortex-compact (0.874x ➖, 0↑ 0↓)
duckdb / parquet (1.045x ➖, 0↑ 1↓)
Full attributed analysis
|
Contributor
Benchmarks: TPC-DS SF=1 on NVMEVerdict: No clear signal (low confidence) How to read Verdict and Engines
datafusion / vortex-file-compressed (0.904x ➖, 43↑ 0↓)
datafusion / vortex-compact (0.919x ➖, 26↑ 1↓)
datafusion / parquet (0.904x ➖, 39↑ 0↓)
duckdb / vortex-file-compressed (0.924x ➖, 24↑ 0↓)
duckdb / vortex-compact (0.924x ➖, 30↑ 1↓)
duckdb / parquet (0.941x ➖, 10↑ 1↓)
duckdb / duckdb (0.918x ➖, 29↑ 0↓)
File Size Changes (496 files changed, -99.2% overall, 0↑ 496↓)
Totals:
Full attributed analysis
|
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.Todos:
elementswhen we fetch most ofoffsetsto avoid sequential io