Skip to content

Remove test that stores empty file#2560

Closed
maxrjones wants to merge 1 commit intozarr-developers:mainfrom
maxrjones:remove-test
Closed

Remove test that stores empty file#2560
maxrjones wants to merge 1 commit intozarr-developers:mainfrom
maxrjones:remove-test

Conversation

@maxrjones
Copy link
Member

This test parameterization was causing failures in #1661 because it would create a file with no bytes stored, which would lead to failures when trying to read specific bytes. @jhamman recommended removing the test and opening a PR for input from @d-v-b.

@d-v-b
Copy link
Contributor

d-v-b commented Dec 15, 2024

I don't think we use the "write no bytes" feature anywhere in zarr, but to me it's a bit suspicious that our stores pass this test but a different store does not -- that suggests some fundamental differences between the stores that I would like to understand better before we weaken the tests

@maxrjones
Copy link
Member Author

I don't think we use the "write no bytes" feature anywhere in zarr

Do you think a chunk could compress to zero bytes if it's only zeros or fill values depending on the compressor?

but to me it's a bit suspicious that our stores pass this test but a different store does not -- that suggests some fundamental differences between the stores that I would like to understand better before we weaken the tests

That makes sense. There are two cases related to a chunk with zero bytes that are tested here:

  1. Test get for an entire persisted chunk with zero bytes - this should return an empty byte string and does for all the stores
  2. Test get with a byte range specified for a persisted chunk with zero bytes - fsspec seems to return an empty byte string while obstore errors with Generic LocalFileSystem error: Requested range was invalid. Which do you think is more appropriate?

@dstansby dstansby added the needs release notes Automatically applied to PRs which haven't added release notes label Jan 9, 2025
@dstansby dstansby removed the needs release notes Automatically applied to PRs which haven't added release notes label Jan 20, 2025
@kylebarron
Copy link
Contributor

kylebarron commented Feb 5, 2025

I'm not knowledgeable enough to say whether these specific tests should be removed, but I would argue it's incorrect to expect that a range request that is wholly outside the bounds of the resource should not error. For reference, the HTTP spec says that a range request that starts after the end of the file should error with 416 Requested Range Not Satisfiable.

@d-v-b
Copy link
Contributor

d-v-b commented Feb 5, 2025

Do you think a chunk could compress to zero bytes if it's only zeros or fill values depending on the compressor?

I don't think it's possible to rule this out categorically, but I don't think any of our current compressors do this.

but it seems like there's no way to express a valid HTTP range request against a 0-length file, so maybe we should not require that stores support this (related discussion: golang/go#47021)

@maxrjones maxrjones closed this Apr 11, 2025
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.

4 participants