Add Group support (for TileDB 2.8)#388
Conversation
|
This pull request has been linked to Shortcut Story #16120: Add support for new Group API. |
eddelbuettel
force-pushed
the
de/sc-16120/groups_api
branch
from
eafb136 to
f5fa12b
Compare
aaronwolen
left a comment
aaronwolen
left a comment
There was a problem hiding this comment.
I'm still working my way through all of the new functionality but here a few minor comments so far.
| return R_NilValue; | ||
| } | ||
| RObject vec = _metadata_to_sexp(v_type, v_num, v); | ||
| vec.attr("key") = Rcpp::CharacterVector::create(key); |
There was a problem hiding this comment.
I like storing the key name in an attribute. Will the array metadata getter adopt the same behavior?
There was a problem hiding this comment.
Possibly. I like it better too.
We should talk about that because I need to refine what I do over there in terms of single values and vectors. This currently does not really support vectors. Would we ever want or need them?
There was a problem hiding this comment.
In terms of storing vectors as metadata ( tiledb_put_metadata(arr, "foo", c(1.1, 2.2, 3.3)))? Or retrieving metadata for multiple keys (tiledb_get_metadata(arr, key = c("foo", "vec")))?
There was a problem hiding this comment.
The former. Whether it works cross-language, whether it works cross-language cross-type (i.e. int, double, string), ... I suggest we do this after we first achieve closure on this initial scope of work in this PR.
Multi-key is out of scope AFAICT.
There was a problem hiding this comment.
@eddelbuettel / @aaronwolen we have a long standing need to add list datatypes in core. Which is most impactful for list of strings. We could implement this to nicely support this cross-platform.
There was a problem hiding this comment.
Makes sense. But I think it's worth updating the array metadata getters to attach the same "key" attribute in this PR.
There was a problem hiding this comment.
I suggest to back off and be a little more careful with that as it is technically a change to an existing interface. Feel free to open and issue / sc ticket and we see about making that change without impact, but I think it does not belong in this PR.
There was a problem hiding this comment.
technically a change to an existing interface
Fair point.
It's a minor thing but my suggestion is either add the attribute in both places or neither. Unless there's a reason it makes sense for groups but not arrays?
There was a problem hiding this comment.
I think I want to change it over at attributes too, I am simply suggesting to be careful about rather than rush it.
It's all an extra as the spec really is [key, value] and we do extract the value given the key.
| ##' @param recursive A logical value indicating whether a recursive dump is desired | ||
| ##' @return A character string | ||
| ##' @export | ||
| tiledb_group_member_dump <- function(grp, recursive) { |
There was a problem hiding this comment.
If I create an empty group and then run tiledb_group_member_dump() the output includes the name of the group itself. Is that expected? e.g.,
library(tiledb)
uri <- tempfile()
chk <- tiledb_group_create(uri)
grp <- tiledb_group(uri, type = "READ")
tiledb_group_member_count(grp)
tiledb_group_member_dump(grp, recursive = FALSE)
produces
[1] 0
[1] "file71e162efd8c GROUP\n"
There was a problem hiding this comment.
I think so. I also noted that if we add a member, and then call dump we get INVALID as property.
I don;t have a comparison or spec and I basically act as middle man to the Core API here.
There was a problem hiding this comment.
Maybe @Shelnutt2 can clarify? My assumption was the output would be empty when no group members are present.
aaronwolen
left a comment
aaronwolen
left a comment
There was a problem hiding this comment.
Fundamentals look good. I still have a few questions but need to do a little more due diligence on my part.
3 participants
This PR adds support for Group objects as added in TileDB 2.8. The PR is (at least for now) labeled draft as we may want to refine a UI aspect or two, but should be feature complete with all member functions of the C++ class implemented at the C++ and R level, and tests added.