-
Notifications
You must be signed in to change notification settings - Fork 670
REFACTOR-#2035: move getitem_array to the backend #2036
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
REFACTOR-#2035: move getitem_array to the backend #2036
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2036 +/- ##
==========================================
- Coverage 82.11% 82.02% -0.10%
==========================================
Files 80 80
Lines 9611 9617 +6
==========================================
- Hits 7892 7888 -4
- Misses 1719 1729 +10
Continue to review full report at Codecov.
|
|
Thank you for your review! I pushed fixes for all issues you found. |
devin-petersohn
left a comment
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.
I am comfortable with this approach. There are multiple ways we can make the implementation for these indexers faster, we need to create issues for them after this is merged.
|
@ienkovich , LGTM! @devin-petersohn , are there any comments? |
|
|
devin-petersohn
left a comment
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.
One minor nit. Naming is okay with me!
|
@devin-petersohn Thanks for your review! I've pushed the final fix |
Signed-off-by: ienkovich <ilya.enkovich@intel.com>
|
@devin-petersohn is it ok now? |
devin-petersohn
left a comment
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.
Thanks for making the changes @ienkovich, LGTM!
…-project#2036) Signed-off-by: ienkovich <ilya.enkovich@intel.com>
What do these changes do?
This patch moves
getitem_arrayimplementation from API level to the backend. This allows OmniSci backend to have its own implementation with a lazy bool indexer support.flake8 modinblack --check modingit commit -s