Add support for opening structured dtypes as void for zarr driver
#272
Conversation
|
Ok, I imported this locally and am looking at a few things.
|
…ussion_r2663351949` and extend test coverage
…ussion_r2663353574` and extend test coverage
My bad, I don't know how I missed the compilation issue for the spec test. I've fixed the issue and resolved your comments. Thanks for taking a look and getting back to this so fast! |
…ges/BASE..a42b6f511375dc1bd402ad525519f57210546735#r2666111665` Enforce schema validation for one-of `field` and `open_as_void`
…ussion_r2666195572` Use synthesized open_as_void field
| } | ||
| metadata->dtype = *partial_metadata.dtype; | ||
| } else { | ||
| if (!selected_field.empty()) { |
There was a problem hiding this comment.
This should fail in open_as_void.
if (open_as_void) {
return absl::InvalidArgumentError(
"\"dtype\" must be specified in \"metadata\" if \"open_as_void\" is specified");
}
| if (partial_metadata.dtype) { | ||
| // If a zarr dtype is specified explicitly, determine the field index. If a | ||
| // multi-field zarr dtype is desired, it must be specified explicitly. | ||
| TENSORSTORE_ASSIGN_OR_RETURN( |
There was a problem hiding this comment.
This seems a little roundabout. I'd need to look at where GetFieldIndex is used, but it seeems like it might be more direct to do something like (spitballing here):
if (open_as_void) {
metadata->dtype = ... open_as_void dtype with a single dtype.field set ...
} else {
metadata->dtype = *partial_metadata.dtype;
}
|
|
||
| std::string DataCache::GetBaseKvstorePath() { return key_prefix_; } | ||
|
|
||
| // VoidDataCache implementation |
There was a problem hiding this comment.
Once we compose a ZarrMetadata with an open as void field I think that most of these can just be the base implementation with the above metadata ptr.
| Must be specified if the `.metadata.dtype` specified in the array | ||
| metadata has more than one field. | ||
| default: null | ||
| oneOf: |
There was a problem hiding this comment.
This isn't a valid schema --- but also I don't think we actually want the oneOf constraint since specifying open_as_void=false along with non-empty field name should be allowed
There was a problem hiding this comment.
I was thinking about field vs. open_as_void as a variant.
3 participants
Supersedes #264
Removes
bool open_as_void = falsedefault in C++ function declarations and definitions.Uses a derived DataCache implementation similar to strategy used in
neuroglancer_precomputeddriver forUnshardedDataCacheandShardedDataCache