[r,c++] Use SOMA Context object throughout#2997
Conversation
johnkerl
left a comment
johnkerl
left a comment
There was a problem hiding this comment.
This is awesome! Just a few minor notes at this point.
Also minor cleanup from code review for one indentation and a few comments, and one spurious extra call
| tiledb_timestamp = NULL, internal_use_only = NULL, | ||
| soma_context = NULL) { |
There was a problem hiding this comment.
I thought we had agreed on not exposing soma_context() via $new() at this point in time #2988 (comment)
There was a problem hiding this comment.
Yes. This gives the object the option to also set one here but it is not a requirement. Having the entry point is good for testing, methinks. See the comment a few lines futher down as well as the need to rework / let go of tiledbsoma_ctx. I think this is all best addressed in a next PR.
TileDB-SOMA/apis/r/R/TileDBObject.R
Lines 34 to 51 in 091e109
| sr <- sr_setup( | ||
| uri = x$uri, | ||
| config = as.character(tiledb::config(x$tiledbsoma_ctx$context())), | ||
| soma_context(), |
There was a problem hiding this comment.
Note for future work: this should be replaced with fetching the context from x; currently available with
| soma_context(), | |
| x$.__enclos_env__$private$.soma_context |
There was a problem hiding this comment.
Fair point but not urgent and on the fence as whether we should use opaque internals. "Future work", as you say.
3 participants
Issue and/or context:
SOMA Context objects are used throughout the C++ library part of SOMA. Where we previously serialized context and configuration to vector, maps or lists of key-value pairs, we now hold on to an allocated object for longer.
Changes:
This PR changes how we provide such a context from the R package by allocating one as a shared pointer and passing it around as an external pointer throuch which the SOMA context object can be accessed. Where needed, the SOMA context also contains a (core) TileDB context object.
We currently cache the most-recently created object at the package level and provide it if no new one is demanded. This scheme is exactly what tiledb-r does, and acts reliably and robust via a single access point
soma_context(). One unit test show how to create a different configuration without caching, using it and then reverting to the default.SOMA classes can obtain their initial default object via the instantiating factory methods. Access is then provided via a member function, allowing both for simple re-use as well variations (object by object) as needed. How to split the core object values off the
platform_configobject is left for a subsequent PR that will refine the interface.All tests pass.
Notes for Reviewer:
SC 55146 #2899