Implement index_location setting for Zarr3 Datasets #7553
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
fm3
reviewed
Jan 15, 2024
...ssos-datastore/app/com/scalableminds/webknossos/datastore/datareaders/zarr3/Zarr3Array.scala
Outdated
Show resolved
Hide resolved
frcroth
force-pushed
the
index-location
branch
from
af8a201
to
8eda514
Compare
fm3
approved these changes
Jan 15, 2024
normanrz
reviewed
Jan 18, 2024
| @@ -122,8 +119,22 @@ class Zarr3Array(vaultPath: VaultPath, | |||
| parsed = parseShardIndex(shardIndexRaw) | |||
| } yield parsed | |||
|
|
|||
| private lazy val shardIndexChecksumLength = | |||
There was a problem hiding this comment.
I guess this works for most datasets, but not for all. Although, it doesn't make a lot of sense, it is spec-compliant to have multiple crc32c checksums. In the future, there might be other codecs that change the size of the shard index.
A more correct way would be to compute the size of the index based on the entire index_codecs pipeline. zarrita uses for that the compute_encoded_size methods.
There was a problem hiding this comment.
At least, there should be some assertions that only supported index configurations are usable.
fm3
reviewed
Jan 22, 2024
webknossos-datastore/app/com/scalableminds/webknossos/datastore/datareaders/zarr3/Codecs.scala
Outdated
Show resolved
Hide resolved
fm3
approved these changes
Jan 22, 2024
|
Approving this now based on normans previous feedback. @normanrz if you have other wishes, let’s add them as a follow-up issue :) that way this PR isn’t blocked |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
URL of deployed dev instance (used for testing):
Steps to test:
TODOs:
Issues:
(Please delete unneeded items, merge only when none are left open)