-
Notifications
You must be signed in to change notification settings - Fork 1
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
Reimplementation of NestedExtensionArray #39
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #39 +/- ##
==========================================
+ Coverage 95.68% 97.35% +1.66%
==========================================
Files 14 14
Lines 626 794 +168
==========================================
+ Hits 599 773 +174
+ Misses 27 21 -6 ☔ View full report in Codecov by Sentry. |
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.
Looks good, just a couple nits/questions that you can feel free to address or ignore!
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.
This looks good to me, though I'm not very knowledgeable about arrow so there might be some aspects that I'm missing
One thing I'm curious about given the size of the change is if we can go ahead re-run @dougbrn's benchmarking notebook, but I don't think it should strictly block this PR
This reimplements
NestedExtensionArray
inheriting it directly fromExtensionArray
instead ofArrowExtensionArray
. It removes dependency on unstableArrowExtensionArray
, gives us more control over the behavior, and allows some performance optimization.If fixes #15, fixes #24
Change Description
Solution Description
Code Quality
Project-Specific Pull Request Checklists
Bug Fix Checklist
New Feature Checklist
Documentation Change Checklist
Build/CI Change Checklist
Other Change Checklist