Feature: add listview rebuild + cleanup #4819
Merged
Conversation
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
connortsui20
added
changelog/feature
CodSpeed Performance ReportMerging #4819 will degrade performances by 12.49%Comparing Summary
Benchmarks breakdown
|
Codecov Report❌ Patch coverage is ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| ); | ||
|
|
||
| // Validate the `offsets` and `sizes` arrays. | ||
| match_each_integer_ptype!(offset_ptype, |O| { |
Contributor
There was a problem hiding this comment.
can we add a todo to validate this without a canoncalize?
Contributor
Author
There was a problem hiding this comment.
I think it is basically always more efficient to validate after canonicalizing? Or put another way, if we want to add offsets + sizes arrays then we basically always need to canonicalize the bitpacked arrays
Contributor
There was a problem hiding this comment.
You could look at the specific encoding used (e.g. Delta) and do less work?
Comment on lines
+89
to
+125
| if referenced_elements | ||
| .iter() | ||
| .all(|&is_referenced| is_referenced) | ||
| { | ||
| return self.clone(); | ||
| } |
Contributor
There was a problem hiding this comment.
can you merge this with the previous loop?
Contributor
Author
There was a problem hiding this comment.
Don't think you can do that efficiently? Unless you used some sort of heap data structure...
Contributor
There was a problem hiding this comment.
Is it worth even checking this? I would assume its non-trival to compute and you are being asked to compact anyway?
Comment on lines
+55
to
+97
| fn rebuild_remove_gaps(&self) -> ListViewArray { | ||
| if self.is_empty() { | ||
| return self.clone(); | ||
| } | ||
|
|
||
| let offset_ptype = self.offsets().dtype().as_ptype(); | ||
| let size_ptype = self.sizes().dtype().as_ptype(); | ||
|
|
||
| match_each_integer_ptype!(offset_ptype, |O| { | ||
| match_each_integer_ptype!(size_ptype, |S| { self.rebuild_remove_gaps_inner::<O, S>() }) | ||
| }) | ||
| } |
Contributor
There was a problem hiding this comment.
Did you benchmark this is seems very slow
Contributor
Author
There was a problem hiding this comment.
It probably is slow, but the issue is that the alternatives are generally data dependent or much more complex:
- We could track ranges in a segment-tree-like data structure, but that is only good when the gaps are large
- We could do a union find thing for ranges but that has the same problem as the previous one
- We could use a bitset, which would definitely be more efficient if we implement a complicated prefix summation of a bitvector, and I'm not actually sure it would be much faster (but definitely several orders of magnitude more complicated)
- Roaring bitmaps? Even more complicated...
|
|
||
| // TODO(connor)[ListView]: Figure out a better heuristic. | ||
| if selection_mask.density() < REBUILD_DENSITY_THRESHOLD { | ||
| new_array = new_array.rebuild(ListViewRebuildMode::RemoveGaps); |
Contributor
There was a problem hiding this comment.
Do you think just adding everything to a builder (the other rebuild might be faster)?
Contributor
Author
There was a problem hiding this comment.
Yeah that is the MakeZeroCopyToList mode. You're probably right (though for this PR I'm remove this)
Comment on lines
+97
to
+146
| // For example, if elements [0, 2, 3] are used, the prefix sum maps: 0->0, 1->1, 2->1, 3->2. | ||
| let mut prefix_sum = vec![0; self.elements().len()]; | ||
| let mut cumulative_sum = 0; | ||
| for i in 0..referenced_elements.len() { | ||
| prefix_sum[i] = cumulative_sum; | ||
| if referenced_elements[i] { | ||
| cumulative_sum += 1; | ||
| } | ||
| } | ||
|
|
||
| // Copy only the referenced elements into a new compacted elements array. | ||
| let mut elements_builder = builder_with_capacity(self.elements().dtype(), cumulative_sum); | ||
| for (i, &is_referenced) in referenced_elements.iter().enumerate() { | ||
| if is_referenced { | ||
| elements_builder | ||
| .append_scalar(&self.elements().scalar_at(i)) | ||
| .vortex_expect("append scalar"); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Can we merge these loops? Do you think the compiler would in-fact fuse?
Contributor
Author
There was a problem hiding this comment.
It wouldn't fuse because the group is dependent on cumulative_sum becoming the capacity of the builder. Maybe we could make the capacity of the builder 0 since it can keep growing? Personally I would just let the compiler figure this out.
joseph-isaacs
added
the
action/benchmark
github-actions
bot
removed
the
action/benchmark
Contributor
Benchmarks: Random AccessSummary
|
Contributor
Benchmarks: TPC-H SF=1 on 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: CompressionSummary
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
|
Contributor
Benchmarks: TPC-DS SF=1 on NVMESummary
Detailed Results Table
|
connortsui20
force-pushed
the
ct/list-view-rebuild
branch
from
8b7e9f9 to
55bd04c
Compare
Contributor
There was a problem hiding this comment.
can we add benchmarks for these?
Contributor
Author
There was a problem hiding this comment.
I think I'm going to leave it to the next PR
joseph-isaacs
approved these changes
Oct 2, 2025
Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
connortsui20
force-pushed
the
ct/list-view-rebuild
branch
from
55bd04c to
d0cd099
Compare
7 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Tracking Issue: #4699
Adds a
rebuildmethod onListViewArraythat allows us to rebuild the array to specific shapes that we want (via aListViewRebuildModeenum).This will make it easier to implement an optimization (that we are tentatively calling
IsZeroCopyToList) that tells us theListViewArrayhas no gaps nor overlaps from the views into theelementsbuffer, and that the views are all sorted and in order.I also added an unimplemented
OverlapCompressionrebuild mode if we ever want to do that.Also does some cleanup of other
ListViewfunctionality.