Uh oh!
There was an error while loading. Please reload this page.
feat: write nested struct layouts - #4942
Merged
Merged
Conversation
a10y
commented
Oct 14, 2025
Comment on lines
+288
to
+291
| vortex_array::compute::mask( | ||
| projection.as_ref(), | ||
| &Mask::from_buffer(!validity.to_bool().boolean_buffer()), | ||
| ) |
ContributorAuthor
There was a problem hiding this comment.
this is only safe when we know that the projection has the same length as the struct itself. If we're projecting an UNNEST(a) that will not hold. We don't have UNNEST but i suspect we'll want to add it so we can pushdown into list elements
2 tasks
robert3005
approved these changes
Oct 16, 2025
robert3005
left a comment
Contributor
There was a problem hiding this comment.
I think this makes sense. Some of the code comments are not correct though
| chunks.push(chunk); | ||
| } | ||
| let collected = ChunkedArray::try_new(chunks, _dtype)?.to_array(); |
Contributor
There was a problem hiding this comment.
maybe we canonicalize here instead of writing a flat chunked array?
ContributorAuthor
There was a problem hiding this comment.
In practice that ends up happening b/c the child here is the CompressStrategy, so it will canonicalize + compress internally.
In general I tried to mirror this on the RepartitionStrategy which yields ChunkedArray as well. I think the LayoutStrategies are a bit weird tbh and there are a few unexpected things like this.
Contributor
There was a problem hiding this comment.
I thought repartition strategy will have a chunked strategy as a child so it will flatten the chunked array, in this case I think we go straight to flat.
Uh oh!
There was an error while loading. Please reload this page.
a10y added a commit
that referenced
this pull request
Oct 16, 2025
GithubArchive is an archival dataset of Github event logs, hosted at https://www.gharchive.org/. It's also distributed as part of RealNest from CWI. This is our first benchmark to test nested data access with Vortex. I want to get this in before we merge #4942 or any other follow on work for nested data support. TODOs: - [x] Add more data files. Currently queries finish way too fast - [x] Add a few more queries --------- Signed-off-by: Andrew Duffy <andrew@a10y.dev>
Contributor
Benchmarks: FineWeb S3Summary
Detailed Results Table
|
Contributor
Benchmarks: FineWeb NVMeSummary
Detailed Results Table
|
Contributor
Benchmarks: TPC-H SF=1 on S3Summary
Detailed Results Table
|
Contributor
Benchmarks: TPC-H SF=10 on NVMESummary
Detailed Results Table
|
Contributor
Benchmarks: Statistical and Population GeneticsSummary
Detailed Results Table
|
Contributor
Benchmarks: TPC-H SF=10 on S3Summary
Detailed Results Table
|
Contributor
Benchmarks: Clickbench on NVMESummary
Detailed Results Table
|
Signed-off-by: Andrew Duffy <andrew@a10y.dev>
ContributorAuthor
The latest few commits remove the contention in It's a bit bittersweet, but the reason the other queries seem to have gotten slower is because...we're compressing better. We now have 773 leaf StructLayouts, most of which encode string data. Now that those are all being shredded out, we're compressing them with Dict+FSST much more frequently, which makes decoding slower:   So I think that the StructLayout is fine, it's actually the way we read back chunks from a DictLayout that may be problematic. For Query 3 from gharchive:   This means that to read just one single chunk with splits of 8192 rows, require 36 split tasks. Each split task returns a DictArray wrapping 8192 codes, and the entire 54,000 item FSST-encoded dictionary. Calling to_arrow on each of these DictArray chunks will in turn decode the entire FSST values dictionary. So in effect, it's not that FSST is too slow, but rather that we're decoding the same FSST array 36 times. |
a10y
commented
Oct 28, 2025
ContributorAuthor
I suppose this is something that Operator work is supposed to help with? Though that really seems more focused on memoizing decodes in a single split task, rather than across split tasks? |
robert3005
commented
Oct 28, 2025
Contributor
If we only get 8192 codes we should only decode those codes. I know duckdb have a decompression cache on the fsst array to avoid any double decompression which is similar to us caching canonicalisations. I would still try first to avoid canonicalising values unconditionally and performing the take first |
Signed-off-by: Andrew Duffy <andrew@a10y.dev>
a10y
commented
Oct 28, 2025
ContributorAuthor
This is about values not codes though. The issue is that every chunk goes through ToArrow, which canonicalizes. The entire values array gets decompressed for each chunk in that case. As a test, when I switch the DictReader to instead eagerly canonicalize the values, Q3 goes from being 40% slower to nearly 2x faster than baseline. I'm going to revert that commit for now, but I think this is a problem that is independent of nested struct splitting and is something we'll have to handle more fundamentally with DictLayouts. |
This reverts commit 570d987. Signed-off-by: Andrew Duffy <andrew@a10y.dev>
robert3005
approved these changes
Oct 28, 2025
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
2 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.
Part of #4889
To better support writing nested data, here we update our builtin StructLayout in a backwards-compatible fashion.
StructStrategywill shred struct fields into their own StructLayout recurseivelyI use the
RealNestdataset to evaluate, which contains a copy of ~200k github pull request webhook events. Nested struct layout reduces file size over the previous strategy by about ~10%, and also makes pushdown into the nested columns possible.Some open questions
UNNESTor something else that increases the result size, it is hard to map the validity buffer onto the projection_eval result