Skip to content
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

[python,c++] Improve error handling in IntIndexer #3441

Merged
merged 9 commits into from
Dec 16, 2024

Conversation

bkmartinjr
Copy link
Member

@bkmartinjr bkmartinjr commented Dec 14, 2024

Issue and/or context:

IntIndexer would do strange things when given malformed inputs. See for example sc-60689 and sc-60690

IntIndexer also incorrectly handled any arrow array with an offset, and would return incorrect outputs. See sc-60795

Changes:

  1. More error checking in pybind-ings, plus tests. The desired parts of the implicit casting are moved up into the Python code where they are more easily managed.
  2. Minimal fix to correctly use array offsets (note: this code needs refactoring to use nanoarrow interfaces)

Copy link

codecov bot commented Dec 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.29%. Comparing base (b30dd60) to head (b6c5785).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3441      +/-   ##
==========================================
+ Coverage   86.22%   86.29%   +0.06%     
==========================================
  Files          55       55              
  Lines        6295     6305      +10     
==========================================
+ Hits         5428     5441      +13     
+ Misses        867      864       -3     
Flag Coverage Δ
python 86.29% <100.00%> (+0.06%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
python_api 86.29% <100.00%> (+0.06%) ⬆️
libtiledbsoma ∅ <ø> (∅)

@johnkerl johnkerl changed the title [python,C++] improve error handling in IntIndexer [python,c++] improve error handling in IntIndexer Dec 14, 2024
@johnkerl johnkerl changed the title [python,c++] improve error handling in IntIndexer [python,c++] Improve error handling in IntIndexer Dec 14, 2024
@bkmartinjr bkmartinjr marked this pull request as ready for review December 14, 2024 16:57
@bkmartinjr bkmartinjr marked this pull request as draft December 15, 2024 16:20
@bkmartinjr bkmartinjr marked this pull request as ready for review December 15, 2024 20:27
@nguyenv
Copy link
Member

nguyenv commented Dec 16, 2024

What is the purpose of adding noconvert?

@bkmartinjr
Copy link
Member Author

What is the purpose of adding noconvert?

It tells pybind to not try and cast, which removes all kinds of unsupported things (i.e., likely errors) from being done implicitly. Pybind will try to cast almost any sequence to an ndarray. If we don't explicitly need that behavior, it is better to just disable it.

@bkmartinjr bkmartinjr merged commit e474425 into main Dec 16, 2024
21 checks passed
@bkmartinjr bkmartinjr deleted the bkmartinjr/reindexer-cleanup branch December 16, 2024 15:53
github-actions bot pushed a commit that referenced this pull request Dec 16, 2024
* improve error handling in reindexer/IntIndexr

* improve error handling in reindexer/IntIndexr

* refine exceptions

* lint

* lint

* fix mishandling of arrow array input

* add test
johnkerl pushed a commit that referenced this pull request Dec 16, 2024
* improve error handling in reindexer/IntIndexr

* improve error handling in reindexer/IntIndexr

* refine exceptions

* lint

* lint

* fix mishandling of arrow array input

* add test

Co-authored-by: Bruce Martin <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants