-
Notifications
You must be signed in to change notification settings - Fork 159
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[FEAT] Add basic list aggregations #2032
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2032 +/- ##
==========================================
+ Coverage 84.70% 85.26% +0.55%
==========================================
Files 62 68 +6
Lines 6768 7268 +500
==========================================
+ Hits 5733 6197 +464
- Misses 1035 1071 +36
|
Updated the code to just slice the series and use the Series aggregations on each list. Much cleaner code and consolidates the logic to one point which I like, but unsure if there are performance issues with doing this. From what I can tell there is nothing that will require significant additional runtime or copies. |
.iter() | ||
.map(|s| s.unwrap_or(Series::empty("", self.child_data_type()))) | ||
.map(|s| op(&s)) | ||
.collect::<DaftResult<Vec<_>>>()?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we probably want to have a concat that takes in an iterator of Series for this. materializing all the Series then concating it is going to be slow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tabled until we create an ArrayBuilder so that we don't have to fully materialize the Series objects to concat
As requested in #1977
List aggregations: