perf: only encode actual list values in RowConverter (16-26 times faster for small sliced list)
#7996
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
Waiting for: - apache#7994 to be merged first Closes apache#7993
rluvaton
commented
Jul 24, 2025
Comment on lines
523
to
529
| let first_offset = list_array.offsets()[0] as usize; | ||
| let last_offset = | ||
| list_array.offsets()[list_array.offsets().len() - 1] as usize; | ||
|
|
||
| list_array | ||
| .values() | ||
| .slice(first_offset, last_offset - first_offset) |
Member
Author
There was a problem hiding this comment.
this can be changed to get_values_sliced() or something after we resolve:
Contributor
There was a problem hiding this comment.
Could you also please add some comments explaining the context of this code (that as you have said, is non obvious) for future readers
For example, something like
// values can include more data than referenced in the ListArray, only encode
// the referenced values. 
alamb
reviewed
Jul 26, 2025
Contributor
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
Thanks @rluvaton -- this code looks good to me
I think we need to add some tests as well prior to merging this PR
For example:
- A round trip encoded sliced list (if it doesn't already exist)
- A test specifically for encoding a smaller part of the list -- for example a test that created a ListArray with 2 rows: a single element list
[1], and a list with 1000 values,[1,2,3,4...1000], sliced to only include the single element list. Then encode it and ensure the encodde size is reasonable
Comment on lines
523
to
529
| let first_offset = list_array.offsets()[0] as usize; | ||
| let last_offset = | ||
| list_array.offsets()[list_array.offsets().len() - 1] as usize; | ||
|
|
||
| list_array | ||
| .values() | ||
| .slice(first_offset, last_offset - first_offset) |
Contributor
There was a problem hiding this comment.
Could you also please add some comments explaining the context of this code (that as you have said, is non obvious) for future readers
For example, something like
// values can include more data than referenced in the ListArray, only encode
// the referenced values. 
Contributor
|
Ooops, sorry -- I didn't see #7994 -- looking at that one |
alamb
reviewed
Jul 27, 2025
Contributor
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
Thanks @rluvaton -- I still worry that this PR has no tests and thus we run the risk of potentially breaking / reverting it in some future PR.
Would it be possible to add some coverage, either in the form of the "verify the size of the output rows" or a performance benchmark?
Member
Author
|
Pr: Added the tests. Round trip is included in the fuzz test. Number of output rows: already covered in the existing tests The output rows size test will pass on both main and this branch as list encode the entire children as intermediate data, it would later only pick visible list values I can add a performance benchmark that would show how bad for performance it was before |
Member
Author
Member
Author
|
Run the benchmark against this and: these are the results locally: it shows 16 to 26 times faster |
rluvaton
changed the title
RowConverterRowConverter (16-26 times faster for small sliced list)
alamb
pushed a commit
that referenced
this pull request
Jul 28, 2025
#8008) # Which issue does this PR close? N/A # Rationale for this change #7996 (review) # What changes are included in this PR? added to the row format conversion list/large list and sliced list/large list cases # Are these changes tested? Not needed # Are there any user-facing changes? Nope ---- Related to: - #7996
Contributor
|
🤖 |
alamb
approved these changes
Jul 28, 2025
Contributor
|
🤖: Benchmark completed Details
|
Member
Author
|
@alamb the benchmark results does not include the new benchmarks |
Contributor
Yeah I probably need to merge this PR up from main (so |
Contributor
|
🤖 |
Contributor
|
🤖: Benchmark completed Details
|
Contributor
|
Thank you @rluvaton |
Sign up for free
to 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.
Which issue does this PR close?
RowConverteron list should only encode the sliced list values and not the entire data #7993.Rationale for this change
See issue
What changes are included in this PR?
only encode sliced list values and change shift the offset in encoding
Are these changes tested?
Yes in:
Waiting for it to be merged first
Are there any user-facing changes?
only perf