Update buffer element query functions#282
Conversation
| } | ||
|
|
||
| // [[Rcpp::export]] | ||
| XPtr<tiledb::Array> libtiledb_query_get_array(XPtr<tiledb::Query> query, XPtr<tiledb::Context> ctx) { |
There was a problem hiding this comment.
Should we document somewhere that the return value from this function cannot outlive the query? (b/c it is non-owning).
There was a problem hiding this comment.
That's a good question. Adding at least a code comment is very cheap and possibly illuminating.
You may have seen that I shied away from promoting this more, and forcing its use into the size getter (in order to answer 'are we nullable?' and kept it simpler by asking for a bool from the users. It's not obvious to me yet what the best approach is. One other thing to consider is to simply budget another pointer in the S4 class for the query and keep the array pointer there (making it also clear that we access the array via the query that is associated with it) when we construct the query object.
There was a problem hiding this comment.
I think I missed where this is being used right now? I thought it was forward-looking. In any case, this:
One other thing to consider is to simply budget another pointer in the S4 class for the query and keep the array pointer there (making it also clear that we access the array via the query that is associated with it) when we construct the query object.
sounds good to me. We do effectively the same thing in TileDB-Py, for similar reasons (I think there might be one bare call to get array on a C++ Query object, but it is a confined lifetime and not user accessible).
There was a problem hiding this comment.
(but if you prefer a code comment, that's fine too - I don't know what the intended usage is so keeping this "internal" seems fine)
There was a problem hiding this comment.
Current use is only in the test file.
eddelbuettel
force-pushed
the
de/buffer_elements
branch
from
9ceb007 to
b4c6106
Compare
|
Rebased (after merging #283) and added a (hopefully) clarifying comment. I will leave actually adding the array pointer to the query object for another PR. That still seems like the right thing to do. |
| return query; | ||
| } | ||
|
|
||
| // Note that the Array pointer returned here from a Query object is not owned and will not |
2 participants
This small PR catches up from the R side to the fact that the query result buffer estimate switched from returning a scalar to returning a pair where the first element is often (for fixed size attributes and dimensions).
As the name of the C function was already taken (and only returning a single element) a second function was added. We could, in time, deprecate the old one and then deprecate and rename the new one -- but it may not be worth the trouble.
Tests have been added as well.