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

[r] Add $reopen() to TileDBObject #2372

Merged
merged 6 commits into from
Apr 5, 2024
Merged

[r] Add $reopen() to TileDBObject #2372

merged 6 commits into from
Apr 5, 2024

Conversation

mojaveazure
Copy link
Member

@mojaveazure mojaveazure commented Apr 3, 2024

Expose $reopen() as a public API for all TileDBObjects (groups and arrays); as R does not have context managers, there's no native way to do the Python construct

cls = type(arr)
with cls.open(arr.uri, "r") as readarr:
    # do a read operation even though the array is open in write

This PR exposes $reopen() to reopen an array or collection in a new mode

Modified SOMA methods:

  • TileDBObject$reopen(): pushed down from TileDBArray and now public; includes new default for reopening: if arr$mode() is READ, will automatically reopen in WRITE and vice versa
  • TileDBObject$is_open(): now uses TileDBObject$mode() to determine if the array is open as TileDBObject$mode() already processes TileDBObject$private$.mode

Context: #921

Copy link

codecov bot commented Apr 3, 2024

Codecov Report

Merging #2372 (9e510b8) into main (f1fb30b) will increase coverage by 5.19%.
Report is 1 commits behind head on main.
The diff coverage is 100.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2372      +/-   ##
==========================================
+ Coverage   66.07%   71.26%   +5.19%     
==========================================
  Files         144       54      -90     
  Lines       13002     4695    -8307     
  Branches      510        0     -510     
==========================================
- Hits         8591     3346    -5245     
+ Misses       4311     1349    -2962     
+ Partials      100        0     -100     
Flag Coverage Δ
libtiledbsoma ?
python ?
r 71.26% <100.00%> (+0.03%) ⬆️

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

Components Coverage Δ
python_api ∅ <ø> (∅)
libtiledbsoma ∅ <ø> (∅)

reopen = function(mode = NULL) {
modes <- c(READ = 'WRITE', WRITE = 'READ')
oldmode <- self$mode()
mode <- mode %||% modes[oldmode]
Copy link
Contributor

@eddelbuettel eddelbuettel Apr 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The flipping read/write is clever but maybe too hard to read?

Also what happens if mode were "RESUME" ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per conversation with @mojaveazure mode and ingest_mode are distinct in Python, and must be in R as well.

Copy link
Member Author

@mojaveazure mojaveazure Apr 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For $open() (and therefore $reopen()); mode cannot be "RESUME"

On the Python side, resume-mode exists only in tiledbsoma.io; for parity, this means that resume-mode exists/will exist only in the write_soma() methods and is/will be handled there, but the mode of the array itself cannot be "RESUME"

apis/r/R/TileDBArray.R Outdated Show resolved Hide resolved
Copy link
Contributor

@eddelbuettel eddelbuettel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That will be good to have exposed.

@johnkerl
Copy link
Member

johnkerl commented Apr 3, 2024

@mlin @bkmartinjr @pablo-gar I'd love to get your feedback

Expose `TileDBArray$reopen()` as a public API; as R does not have
context managers, there's no native way to do the Python construct
```python
cls = type(arr)
with cls.open(arr.uri, "r") as readarr:
    # do a read operation even though the array is open in write
```
This PR exposes `$reopen()` to reopen an array in a new mode

Modified SOMA methods:

 - `TileDBArray$reopen()`: now public and includes new default for
   reopening: if `arr$mode()` is `READ`, will automatically reopen in
   `WRITE` and vice versa
 - `TileDBObject$is_open()`: now uses `TileDBObject$mode()` to determine
   if the array is open as `TileDBObject$mode()` already processes
   `TileDBObject$private$.mode`
Bump develop version
@mojaveazure mojaveazure changed the title [r] Make TileDBArray$reopen() public [r] Add $reopen() to TileDBObject Apr 5, 2024
Copy link
Member

@johnkerl johnkerl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔥

@mojaveazure mojaveazure merged commit 95e3156 into main Apr 5, 2024
6 checks passed
@mojaveazure mojaveazure deleted the ph/feat/reopen branch April 5, 2024 18:38
@mojaveazure
Copy link
Member Author

mojaveazure commented May 30, 2024

[sc-48522]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants