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

zipstore delitems via override #1184

Closed
Show file tree
Hide file tree
Changes from 23 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
96d1183
zipstore delitems via override
I-am-Emmanuel Oct 13, 2022
a83bff2
test-for delete and pop item
I-am-Emmanuel Oct 14, 2022
41017c9
Merge branch 'main' of https://github.com/zarr-developers/zarr-python…
I-am-Emmanuel Oct 14, 2022
0144f95
i-am-emmanuel-zip-del_item
I-am-Emmanuel Oct 14, 2022
609186d
Merge branch 'main' of https://github.com/zarr-developers/zarr-python…
I-am-Emmanuel Oct 20, 2022
16fb300
modified delete_item stored item
I-am-Emmanuel Oct 20, 2022
bf18397
modified delete_item stored item
I-am-Emmanuel Oct 20, 2022
d2d49de
modified delete_item stored item
I-am-Emmanuel Oct 20, 2022
60f9f61
modified delete_item stored item
I-am-Emmanuel Oct 20, 2022
6366a2c
modified delete_item stored_item
I-am-Emmanuel Oct 20, 2022
a65d383
Cleanup blank spaces
jakirkham Oct 20, 2022
a41af48
Fix lints
jakirkham Oct 20, 2022
ab52106
Fix one more lint
jakirkham Oct 20, 2022
425a744
Inline `value`
jakirkham Oct 20, 2022
8544da2
Merge branch 'main' of https://github.com/zarr-developers/zarr-python…
I-am-Emmanuel Oct 25, 2022
57ce061
Modified zip_store del_item
I-am-Emmanuel Oct 25, 2022
15c783c
Merge branch 'i-am-emmanuel-zipstore-delitem' of https://github.com/I…
I-am-Emmanuel Oct 25, 2022
040da5f
Modified Zip_del_item
I-am-Emmanuel Oct 25, 2022
5f8b815
Dropping extra spaces (for linter)
jakirkham Oct 25, 2022
489390a
Remodified zip_del_item
I-am-Emmanuel Oct 26, 2022
736e27e
Remodified zip_del_item
I-am-Emmanuel Oct 26, 2022
458d31d
Merge branch 'main' of https://github.com/zarr-developers/zarr-python…
I-am-Emmanuel Nov 2, 2022
dcce2c3
refactor __contains__ and keylist to reflect deleted key in zipstore
I-am-Emmanuel Nov 2, 2022
d305642
Merge branch 'main' into i-am-emmanuel-zipstore-delitem
I-am-Emmanuel Mar 14, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 30 additions & 4 deletions zarr/storage.py
Original file line number Diff line number Diff line change
Expand Up @@ -1755,7 +1755,12 @@ def __exit__(self, *args):
def __getitem__(self, key):
with self.mutex:
with self.zf.open(key) as f: # will raise KeyError
return f.read()
data = f.read()

if data:
return data
else:
raise KeyError("Key not found")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
raise KeyError("Key not found")
raise KeyError(key)


def __setitem__(self, key, value):
if self.mode == 'r':
Expand All @@ -1777,7 +1782,23 @@ def __setitem__(self, key, value):
self.zf.writestr(keyinfo, value)

def __delitem__(self, key):
raise NotImplementedError
with self.mutex:
try:
self.zf.getinfo(key)
except KeyError:
raise KeyError("Cannot delete a non-existent key")
keyinfo = zipfile.ZipInfo(
filename=key,
date_time=time.localtime(time.time())[:6]
)
keyinfo.compress_type = self.compression
if keyinfo.filename[-1] == os.sep:
keyinfo.external_attr = 0o40775 << 16 # drwxrwxr-x
keyinfo.external_attr |= 0x10 # MS-DOS directory flag
else:
keyinfo.external_attr = 0o644 << 16 # ?rw-r--r--

self.zf.writestr(keyinfo, b"")

def __eq__(self, other):
return (
Expand All @@ -1789,7 +1810,8 @@ def __eq__(self, other):

def keylist(self):
with self.mutex:
return sorted(self.zf.namelist())
namelist = [key for key in self.zf.namelist() if self.zf.getinfo(key) != b""]
Copy link
Member

Choose a reason for hiding this comment

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

Would suggest passing namelist through a set since there may be duplicated keys (thanks to overwrites).

Also with getinfo, this provides metadata about the file. Think we can just check the file size is non-zero (so no need to read the file).

Otherwise this seems reasonable.

Suggested change
namelist = [key for key in self.zf.namelist() if self.zf.getinfo(key) != b""]
namelist = [key for key in set(self.zf.namelist()) if self.zf.getinfo(key).file_size]

return sorted(namelist)

def keys(self):
yield from self.keylist()
Expand All @@ -1803,7 +1825,11 @@ def __len__(self):
def __contains__(self, key):
try:
with self.mutex:
self.zf.getinfo(key)
value = self.zf.getinfo(key)

if value == b"":
raise KeyError
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
raise KeyError
raise KeyError(key)


except KeyError:
return False
else:
Expand Down
22 changes: 19 additions & 3 deletions zarr/tests/test_storage.py
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,21 @@ def test_pop(self):

store.close()

def test_delitem(self):
store = self.create_store()
foo = self.root + 'foo'
store[foo] = b'bar'

del store[foo]

with pytest.raises(KeyError):
store[foo]
# if foo in store:
# if isinstance(store, ZipStore):
# assert store[foo] == b''
# else:
# assert foo not in store

def test_popitem(self):
store = self.create_store()
store[self.root + 'foo'] = b'bar'
Expand Down Expand Up @@ -1792,7 +1807,6 @@ def test_flush(self):
store.flush()
assert store[self.root + 'foo'] == b'bar'
store.close()

store = self.ZipStoreClass('data/store.zip', mode='r')
store.flush() # no-op

Expand All @@ -1806,8 +1820,10 @@ def test_pop(self):
# override because not implemented
store = self.create_store()
store[self.root + 'foo'] = b'bar'
with pytest.raises(NotImplementedError):
store.pop(self.root + 'foo')

store.pop(self.root + 'foo')

assert store[self.root + 'foo'] == b""

def test_popitem(self):
# override because not implemented
Copy link
Member

Choose a reason for hiding this comment

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

Would also update this test

Copy link
Author

Choose a reason for hiding this comment

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

Okay.

Expand Down