Skip to content

Enhancements to File, LMDB, Zip#95

Merged
crusaderky merged 1 commit into
dask:mainfrom
crusaderky:invalid_dtypes
Apr 3, 2023
Merged

Enhancements to File, LMDB, Zip#95
crusaderky merged 1 commit into
dask:mainfrom
crusaderky:invalid_dtypes

Conversation

@crusaderky
Copy link
Copy Markdown
Collaborator

  • Robustly check key/value types in File, LMDB, Zip
  • Thoroughly check for file descriptor leaks
  • Zip.__contains__ no longer reads the value from disk
  • Zip.__setitem__ will now raise when updating an already-existing key instead of quietly corrupting the mapping

@crusaderky crusaderky self-assigned this Mar 29, 2023
Comment thread zict/lmdb.py
try:
v = self._mapping[key]
except (KeyError, AttributeError):
except KeyError:
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Why not AttributeError anymore?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

See LMDB.__getitem__. Before, non-string keys would raise AttributeError (from _encode_key); now they raise KeyError.

Comment thread zict/tests/utils_test.py
"""z does not accept any Python object as values.
Test that it reacts correctly when confronted with an invalid value type.
"""
bad = object()
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Why can't value be a Python object?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Such is specifically the signature of LMDB, Zip, and File. They're not MutableMapping[KT, VT]; they're MutableMapping[str, bytes].

Copy link
Copy Markdown

@j-bennet j-bennet left a comment

Choose a reason for hiding this comment

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

Looks good, I just had a couple of questions. 👍

@crusaderky crusaderky merged commit af20f02 into dask:main Apr 3, 2023
@crusaderky crusaderky deleted the invalid_dtypes branch April 3, 2023 22:45
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.

2 participants