Closed johnkerl closed 1 month ago
All modified and coverable lines are covered by tests :white_check_mark:
Project coverage is 90.02%. Comparing base (
08193f2
) to head (3517a1e
). Report is 1 commits behind head on main.
A more general comment orthogonal to this PR: we really should think about making more use of nanoarrow (as I have) to remove most if not all of this arrow hand-holding boilerplate code (ie the bits that have to set each element of the struct explicitly).
Thanks @eddelbuettel . That's a good idea for another PR. At the moment I want to separate refactors / dependency changes from the careful sequencing of steps necesary for confident rollout of the current-domain-backed new-shape feature. Introducing nanoarrow here would be too many variables: a dependent-package change, along with the current-domain support. As follow-on -- separately -- yes, I am behind this. I'll file an issue to track this great idea.
Issue and/or context: As tracked on issue #2407 / [sc-51048]. The intended Python and R API changes are all agreed on and finalized as described in #2407.
Changes:
create_arrow_schema
andcreate_index_column_info
take flags for status quo (use_current_domain=false
) as well as new-shape (use_current_domain=true
-- #2407) . These flags will allow for a reasoned, well-tested transition to new-shape functionality at the R and Python levels, implemented on upcoming PRs.libtiledbsoma
level will validate correctness of the libtiledbsoma-to-core contracts, thereby increasing confidence of the correctness of the libtiledbsoma-level implementation.resize
logic tolibtiledbsoma
, and unit-test its correctness. However, this PR is already more than big enough as-is.Notes for Reviewer:
As of 2024-08-18, this is stacked on top of #2910. #2910 should be approved and merged first, then, this should be rebased on the new
main
. (What should not happen is for this PR to be merged into #2910.)As of 2024-08-18 this incorporates #2915 without being stacked on top of it. Here, too, #2915 should be approved and merged first, then, this should be rebased on the new
main
.