v2 spec: add optional dimension_separator (see #707)

[joshmoore]

Various implementations allow for defining the separator between the
dimension indexes when writing chunks:

• n5-zarr defines a dimensionSeparator parameter;
• zarr-python's NestedDirectoryStore does so by default
• and FSStore provides a key_separator parameter;
• tensorstore has a key_encoding parameter; and
• jzarr is looking to add the same functionality.

When writing an array, it is straight-forward to set this separator
and have arrays properly configured. Consumers of such arrays,
however, must either know a priori if their arrays use a
non-default separator or must loop through all possible chunks keys
searching for the right one.

By defining adding an optional metadata key to the .zarray, we:

• preserve the efficient configuration of arrays
• while keeping the v2 spec backwards compatible.

The primary downsides are that this will be the first optional metadata
value in the v2 spec and therefore we don't have a strong understanding of how that will play out, and datasets which were previously written with non-default separators will need updating in order to enable the detection though that is no worse than the current situation.

cc: @alimanfoo @chris-allan @SabineEmbacher @jbms @axtimwalde @manzt @gzuidhof @haileyajohnson @DennisHeimbigner @zarr-developers/core-devs

(If I've missed any Zarr v2 implementors, please add them)

[martindurant]

So presumably you would be able to still pass the separator argument OR look for the metadata key. The first would take priority and would remain the recommendation for the current and older versions of zarr and for datasets that were written by them.

[joshmoore]

@martindurant: exactly. Wasn't sure if that was too much for this paragraph, but I can look for a spot to go into more details. I imagine if someone passes a separator that doesn't match the expected one we could warn in mode="r". For mode="a|w", I'd personally favor an exception but an alternative would be to update the metadata, effectively hiding the existing chunks.

[martindurant]

It would be reasonable to supply a helper function somewhere to convert an existing array from one style of metadata to another.

[jbms]

I'm happy with this and can add support to tensorstore and neuroglancer. Is this the confirmed metadata format or are we waiting on additional discussion? I'd rather not add this only to then have to also support something else in the future.

[martindurant]

I think we are hoping for more +1 from the other maintainers.
I think I'd be OK with merging if there is no opposition within, say, a week.

[DennisHeimbigner]

The change in the spec says this:

The most common non-default value is "/", which leads to nested keys of the form "0/0" that in most implementations produces a directory structure.

I think the last part "that in most implementations produces a directory structure" should be removed since it is inserting implementation claims into what should be an implementation free document.

[martindurant]

"that in most implementations produces a directory structure" should be removed

Would different wording help, like "in some implementation is a special character used to mark directories" or "the directory delimiter in posix-like storage", or "commonly used as a directory delimiter" ?

[DennisHeimbigner]

Why is it needed at all?

[shoyer]

I like including an example of a non-default value because provides a succinct motivation for why this feature exists. The Zarr spec already has a number of such examples for explaining other features.

[DennisHeimbigner]

Reasonable. Then ignore this suggested change. Thanks.

[joshmoore]

Note: 5 days have passed. https://github.com/zarr-developers/governance/blob/master/GOVERNANCE.md states

Changes to the specification require a dedicated issue on our issue tracker and even more time than an API change. The appropriate length of time is currently under discussion.

Considering this is an optional addition, I'd propose that we leave this call for objections open until the end of the week and then start moving forward? (Any objections to that plan today please. 😉 )

[DennisHeimbigner]

Can the separator be a UTF-8 character => multi-byte?

[axtimwalde]

I think that's a valid question. I vote for UTF-8 arbitrary length string. ASCII encouraged.

[jbms]

For simplicity it might be best to limit it to one of a small set of characters for now, e.g. {"." and "/"}, and possibly expand that later if there is a need for it.

On the implementation side it is not terribly difficult to just concatenate whatever arbitrary string is specified, but there are a few ways things can go wrong:

• If the string starts or ends with a digit, then it is possible that a given key could correspond to more than one chunk.
• If the string is length 0 obviously that leads to ambiguity.
• Currently the allowed characters are [0-9./], which is pretty widely supported by various underlying storage systems. If you allow arbitrary unicode or even arbitrary ascii you can quickly run into characters not supported by the storage system (e.g. local filesystem), or which may have special behavior when opened --- e.g. on MS Windows there are a bunch of reserved names that cause problems when opened, and they aply to every directory, not just the root.
• Some filesystems do various forms of normalization to unicode characters.

While in general the user should be free to make a bad choice and suffer the consequences, in some cases software that processes zarr may be intended to handle an untrusted zarr volume and then care must be taken to handle these issues.

Unless there is currently a use case for a separator other than {"." and "/"} it seems like it would be reasonable to restrict it to those options for now.

[DennisHeimbigner]

Also by putting it into .zarray, it implies that the separator can differ for each
array. I would think it better if the dimension separator was constrained
to be the same for all arrays in the dataset.

[jbms]

Putting the separator into .zarray seems helpful since currently .zarray stands on its own. If you have to look in parent directory .zattr files that makes it more complicated.

As I imagine implementing this, and generally in what seems like the most natural implementation, there wouldn't be any problem with different separators within the same hierarchy. However I see that having different separators within the same hierarchy would not work with the existing implementation of NestedDirectoryStore. However I would suppose that once this feature is implemented NestedDirectoryStore would no longer be needed except for API compatibility.

[axtimwalde]

I like that it can be different for each dataset, it's simpler to parse and may be useful.

[DennisHeimbigner]

I would suggest for version 3 that you introduce a single "superblock" object, ".zarr" say,
to hold metadata that applies to the whole dataset. This could also hold
extension information and the dimension separator could be put there as well.
p.s. I use the term "dataset" to refer to the whole collection of arrays and metadata.
Is there a preferred term for that collection?

[DennisHeimbigner]

This is somewhat off-topic, but it sounds like the real unit here is the .zarray+.zattr+chunks.
There really is no notion of a "dataset" as a collection of arrays and groups have no
semantics at all; they are purely for namespaces.
Under that model, then indeed, everything has to be attached to .zarray, including dimension separators and extensions.

[axtimwalde]

I used the term dataset for 'array' as in HDF5 and N5, sorry for the confusion. I like the term 'container' for the entirety of groups and arrays that belong to the same root. That would be the equivalent of an HDF5 file.

[SabineEmbacher]

Also by putting it into .zarray, it implies that the separator can differ for each
array. I would think it better if the dimension separator was constrained
to be the same for all arrays in the dataset.

The separator should be individually adjustable for each array. A zarr data set can thus also be compiled manually from already existing data.
For example, if already existing data from different processing years are to be merged into a time series data set. Or if subsets of already existing different satellite sensors are merged to one data set, because these extracts from different sensors are needed for certain climate models or other calculations.

[chris-allan]

Having some experience dealing with arbitrary unicode both from an implementation and security perspective I would largely parrot @jbms. Many of us are using, or have the desire to use, Zarr in a trusted environments and in particular on large shared filesystems. Allowing arbitrary unicode dimension separators sets up a whole series of cross-language programming safety, path normalization, and subpath containment scenarios that must be dealt with to be safe in such an environment. Having a defined set of allowed ASCII separator characters is both easier for implementers and safer.

[jbms]

Currently tensorstore does fail if .zarray contains extra fields. However I will remove that check at the same time that I implement this feature.

[DennisHeimbigner]

That restriction should probably be removed from the spec and replaced
with one that says that unrecognized keys should be ignored.

[joshmoore]

Thanks for the suggestion, @DennisHeimbigner. I've pushed:

-Other keys MUST NOT be present within the metadata object.
+Other keys SHOULD NOT be present within the metadata object and SHOULD be
+ignored by implementations.

[joshmoore]

The zarr-python implementation (#716) is finally green. I'd propose we try to get these both merged this week, unless anyone else who is working on an implementation needs more time.

[joshmoore]

Thanks for the review, @Carreau. Considering that there are now several implementations based on this (nczarr, tensorstore, and zarr.js minimally) I'm going to merge. Adding comment to #716 and then the big question will be when and as what to release. (My assumption is 2.8.0)

[joshmoore]

Some additional transcribed notes from @alimanfoo:

• We discussed whether or not this should actually lead to a version number bump. However v3 is already defined and there's no process for a version in between.
• The primary concern would be that existing filesets would become corrupted by older implementations that do not understand the new array key.
• However, since that's already the case today without the metadata key, this should be considered a "fix rather than a [breaking] change"