Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support a list of codecs in place of a single compressor field #153

Merged
merged 1 commit into from
Nov 7, 2022

Conversation

jbms
Copy link
Contributor

@jbms jbms commented Aug 5, 2022

Per what seemed to be the consensus reached at the zarr meeting on 2022-07-27, this replaces the compressor field in the array metadata with a codecs field that can specify a list of codecs.

@martindurant
Copy link
Member

I am very much in favour of this idea

@rabernat
Copy link
Contributor

This makes sense to me too.

@martindurant
Copy link
Member

There is an implicit assumption here, that if the last codec in an encoding chain still produces an array as opposed to bytes (or the list is empty), then the output to storage is the bytes (memory buffer) of that array. That's like the "null" or "dummy" codec mentioned elsewhere. I don't think we need that codes if we are explicit about it here.

@jbms
Copy link
Contributor Author

jbms commented Aug 24, 2022

There is an implicit assumption here, that if the last codec in an encoding chain still produces an array as opposed to bytes (or the list is empty), then the output to storage is the bytes (memory buffer) of that array. That's like the "null" or "dummy" codec mentioned elsewhere. I don't think we need that codes if we are explicit about it here.

See here: https://github.com/zarr-developers/zarr-specs/pull/153/files#diff-90d755c4d7549e4ab133911db1e21cda80aeba463eb25c00a76501c2ab89a5d2R836

@martindurant
Copy link
Member

Perfect, thanks @jbms

@jstriebel
Copy link
Member

LGTM as well. Is there missing anything for this to be merged? cc @joshmoore @MSanKeys963 @alimanfoo @jakirkham We can cherry-pick the change onto the ZEP1-review branch then as well.

Copy link
Member

@jstriebel jstriebel left a comment

Choose a reason for hiding this comment

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

LGTM as well 👍

@joshmoore
Copy link
Member

Merging per all the 👍s. One thing this does make me aware of though, is that we should probably start maintaining a list of explicit changes now before the list becomes too long to be applied to the existing V3 implementations.

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.

5 participants