refactor(rust): Trim sliced-out memory from ListArrays in list arithmetic #19276
Conversation
This file contains 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
github-actions
bot
added
internal
nameexhaustion
requested review from
ritchie46,
orlp and
c-peters
as code owners
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #19276 +/- ##
==========================================
+ Coverage 80.11% 80.13% +0.01%
==========================================
Files 1526 1526
Lines 209338 209359 +21
Branches 2418 2418
==========================================
+ Hits 167707 167761 +54
+ Misses 41081 41048 -33
Partials 550 550 ☔ View full report in Codecov by Sentry. |
ritchie46
reviewed
Oct 17, 2024
| @@ -58,10 +58,12 @@ impl NumericListOp { | |||
| rhs.len(), | |||
| { | |||
| let (a, b) = lhs.list_offsets_and_validities_recursive(); | |||
| assert!(a.iter().all(|x| *x.first() as usize == 0)); | |||
There was a problem hiding this comment.
Can we make this debug_assert?
nameexhaustion
commented
Oct 17, 2024
| /// Returns an iterator over the offsets of this chunked array. | ||
| /// | ||
| /// The offsets are returned as though the array consisted of a single chunk. | ||
| pub fn iter_offsets(&self) -> impl Iterator<Item = i64> + '_ { |
There was a problem hiding this comment.
remove unused code
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.
For example, given this ListArray:
Currently, the list arithmetic kernel would:
10_000values, when we only need5002 - 5000 = 2valuesArithmeticKernelon all 10,000 values, when we only need to call it on a slicevalues[5000..5002]of 2 values.This can be a performance footgun if one performs list arithmetic across sliced chunks of a DataFrame - we would end up performing allocation/compute on the entire DataFrame for every chunk.
This PR trims the sliced-out memory from the ListArrays so that we don't over-allocate / perform compute on sliced-out memory.