Skip to content

Conversation

@BrianMichell
Copy link
Contributor

Supersedes #264

Removes bool open_as_void = false default in C++ function declarations and definitions.

Uses a derived DataCache implementation similar to strategy used in neuroglancer_precomputed driver for UnshardedDataCache and ShardedDataCache

@laramiel
Copy link
Collaborator

laramiel commented Jan 6, 2026

Ok, I imported this locally and am looking at a few things.

  1. Please make sure that all the tests build and pass. By removing the open_as_void = true, the spec tests fail to build.

@BrianMichell
Copy link
Contributor Author

Ok, I imported this locally and am looking at a few things.

  1. Please make sure that all the tests build and pass. By removing the open_as_void = true, the spec tests fail to build.

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!

@BrianMichell
Copy link
Contributor Author

Sorry for the delay responding to the latest round of feedback. I believe everything raised so far has been addressed.

// since we're treating the data as raw bytes regardless of the actual dtype.
// Shape is allowed to differ (handled by base class for resizing).
// Other fields like compressor, order, chunks must still match.
if (existing_metadata.dtype.bytes_per_outer_element !=
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we could just rely on the normal validate but applied to the void metadata.

@laramiel
Copy link
Collaborator

laramiel commented Jan 16, 2026

I think that this is getting pretty close. I have a few minor edits in my import which I can just add in. Getting jeremy to look over it. Thanks.

Edits:

  • some include changes. The riegeli includes are no longer needed in driver.cc.
  • rename VoidMetadataCache -> LazyVoidMetadata. Same for VoidFieldCache -> LazyVoidField.
  • If the Lazy... fields are mutable, the internals do not need to be.
  • convert Lazy... into class with private members & friend to the enclosing struct.

@copybara-service copybara-service bot merged commit b4f899d into google:master Jan 22, 2026
1 check passed
BrianMichell added a commit to BrianMichell/tensorstore that referenced this pull request Jan 26, 2026
Matches the pattern from zarr v2 driver (PR google#272). When both "field"
and "open_as_void" are specified in the spec, return an error since
these options are mutually exclusive - field selects a specific field
from a structured array, while open_as_void provides raw byte access
to the entire structure.
BrianMichell added a commit to BrianMichell/tensorstore that referenced this pull request Jan 26, 2026
The zarr3 URL syntax cannot represent field selection or void access
mode. Following the pattern from zarr v2 driver (PR google#272), ToUrl() now
returns an error when either of these options is specified instead of
silently ignoring them.
BrianMichell added a commit to BrianMichell/tensorstore that referenced this pull request Jan 26, 2026
…trip

Following the pattern from zarr v2 driver (PR google#272), override
GetBoundSpecData in ZarrDataCache to set spec.open_as_void from
ChunkCacheImpl::open_as_void_. This ensures that when you open a
store with open_as_void=true and then call spec(), the resulting
spec correctly has open_as_void=true set.

Without this fix, opening a store with open_as_void=true and then
getting its spec would lose the open_as_void flag, causing incorrect
behavior if the spec is used to re-open the store.
BrianMichell added a commit to BrianMichell/tensorstore that referenced this pull request Jan 26, 2026
Add comprehensive tests for open_as_void functionality following the
patterns from zarr v2 driver (PR google#272):

Tests that PASS:
- OpenAsVoidSimpleType: Verifies simple type arrays can be opened with
  open_as_void, gaining an extra dimension for bytes
- OpenAsVoidSpecRoundtrip: Verifies open_as_void preserved in spec JSON
- OpenAsVoidGetBoundSpecData: Verifies spec() on void store returns
  open_as_void=true (tests the GetBoundSpecData fix)
- OpenAsVoidCannotUseWithField: Verifies mutual exclusivity validation
- OpenAsVoidUrlNotSupported: Verifies ToUrl() rejects open_as_void
- FieldSelectionUrlNotSupported: Verifies ToUrl() rejects selected_field

Tests marked TODO (pending codec chain implementation):
- OpenAsVoidStructuredType
- OpenAsVoidWithCompression
- OpenAsVoidReadWrite
- OpenAsVoidWriteRoundtrip

Also fixes BUILD file: adds :metadata dependency to :chunk_cache target
to provide the dtype.h header that chunk_cache.h includes.
BrianMichell added a commit to BrianMichell/tensorstore that referenced this pull request Jan 26, 2026
The codec chain is prepared for the original dtype and chunk shape
(without the extra bytes dimension). For void access:

DecodeChunk:
- Strip the bytes dimension from grid's chunk_shape to get original shape
- Decode using the original codec shape
- Reinterpret the decoded bytes as [chunk_shape..., bytes_per_elem]

EncodeChunk:
- Input has shape [chunk_shape..., bytes_per_elem] of byte_t
- Create a view with the original chunk shape and element_size
- Encode using the original codec

This follows the pattern from zarr v2 (PR google#272) where the void metadata
has the chunk_layout computed to match encoded/decoded layouts.
BrianMichell added a commit to BrianMichell/tensorstore that referenced this pull request Jan 26, 2026
For void access, the codec handling differs between:
- Non-structured types: codec prepared for [chunk_shape] with original dtype
  Need to decode/encode then reinterpret bytes.
- Structured types: codec already prepared for [chunk_shape, bytes_per_elem]
  with byte dtype. Just decode/encode directly.

Add original_is_structured parameter to cache constructors to properly
distinguish these cases in DecodeChunk and EncodeChunk.

This follows the pattern from zarr v2 (PR google#272) where CreateVoidMetadata()
creates a modified metadata for void access.
BrianMichell added a commit to BrianMichell/tensorstore that referenced this pull request Jan 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants