Repository: wbolster/plyvel (language: python)

Issue:
Request:  __contains__ and __getitem__ interfaces
Assuming db is instance of plyvel.DBInterface, it would be useful to do something like:

db['key']

or 

if 'key' in db



Discussion / hints:
I'm not really sure what that would add over the current interface. Yes, a LevelDB database is a mapping, but it's not the same as a dictionary.

The `key in db` encourages constructs like `if key in db: do_something(db[key])`, which is actually bad practice: LevelDB doesn't provide a way to check for a key's existence other than retrieving the value, so this does two lookups whereas one would have sufficed.

If you really want such an interface, you can easily use something like this (untested):

``` python
class PlyvelDictLike(object):
    def __init__(self, db):
        self.db = db

    def __getitem__(self, key):
        value = self.db.get(key)
        if value is not None:
            return value
        raise KeyError("no such key: %r" % key)

    def __setitem__(self, key, value):
        self.db.put(key, value)

    def __delitem__(self, key):
        self.db.delete(key)
```

Thanks.  I didn't realize that about checking for a key's existence.  In my
particular case, I'm trying to come up with a fast on-disk equivalent of
python's set type, so I am only interested in checking existence.

On Sun, Dec 8, 2013 at 8:18 AM, Wouter Bolsterlee
notifications@github.comwrote:

> I'm not really sure what that would add over the current interface. Yes, a
> LevelDB database is a mapping, but it's not the same as a dictionary.
> 
> The key in db encourages constructs like if key in db:
> do_something(db[key]), which is actually bad practice: LevelDB doesn't
> provide a way to check for a key's existence other than retrieving the
> value, so this does two lookups whereas one would have sufficed.
> 
> If you really want such an interface, you can easily use something like
> this (untested):
> 
> class PlyvelDict(object):
>     def **init**(self, db):
>         self.db = db
> 
> ```
> def __getitem__(self, key):
>     value = self.db.get(key)
>     if value is not None:
>         return value
>     raise KeyError("no such key: %r" % key)
> 
> def __setitem__(self, key, value):
>     self.db.put(key, value)
> 
> def __delitem__(self, key):
>     self.db.delete(key)
> ```
> 
> —
> Reply to this email directly or view it on GitHubhttps://github.com/wbolster/plyvel/issues/26#issuecomment-30081357
> .

In that case you can try something like this (again, untested):

``` python
from collections.abc import MutableSet

class PlyvelSet(MutableSet):
    def __init__(self, db):
        self.db = db

    def __contains__(self, key):
        return self.db.get(key) is not None

    def __iter__(self):
        return self.db.iterator(include_value=False)

    def __len__(self):
        raise NotImplementedError(
            "Cannot efficiently determine size of LevelDB databases")

    def add(self, item):
        self.db.put(item, b"")

    def discard(self, item):
        self.db.delete(item)
```

I just lost an hour because I did not expect that the syntax `key in db` doesn't work the same as with dictionaries. To me this was so unexpected, that it was the last thing that I looked at when debugging my code. The argument that this check promotes bad practice (`if key in db: do_something(db[key])`) is debatable: this equally applies to dictionaries.
ouch, at first i thought that your comment didn't make any sense, since `thing in db` is not supposed to work at all since plyvel does _not_ offer a dict-like interface…

… until it occurred to me that ‘dict-like’ involves more than `__contains__`, `__getitem__`, `__setitem__`, and `__delitem__`, etc.

notably, the ‘in’ operator has fallback behaviour for objects that do not specify `__contains__`, see the [section about the `in` operator in the language reference](https://docs.python.org/3/reference/expressions.html#membership-test-details):

- `__contains__` ← not present on `plyvel.DB`
- `__iter__` ← present on `plyvel.DB` 
- `__getitem__` ← not present on `plyvel.DB`

the `in` operator _accidentally_ triggers the `__iter__` behaviour!

this means that `thing in db` actually iterates over the db, which yields `(key, value)` pairs, none of which will match `thing`. for comparison, `(key, value) in db` actually works… _accidentally_! for non-matches such as an innocent looking key lookup the whole database is iterated (extremely slow for larger databases), and will always return `False` in the end.

not good 😨

that said, i stand by my 10+ year old opinion that `plyvel.DB` (and iterators, and prefixed `DB`, etc.) should _not_ offer `__contains__` and `__getitem__` to suggest a dict-like interface. it's not just the performance anti-pattern, but also other expectations like `.keys()`, `thing in db`, `iter(db)` yielding just keys (not the case in plyvel!), etc.

my proposed solution is to explicitly break `thing in db` by adding a `__contains__` method to each relevant class that will simply raise `TypeError`.

wdyt? @wiegerw 

Fix patch:
diff --git a/plyvel/_plyvel.pyx b/plyvel/_plyvel.pyx
index 877a040..728cfbc 100644
--- a/plyvel/_plyvel.pyx
+++ b/plyvel/_plyvel.pyx
@@ -213,6 +213,10 @@ cdef int parse_options(Options *options, c_bool create_if_missing,
             comparator_name, comparator)
 
 
+def contains_is_unsupported():
+    raise TypeError("__contains__ is not supported ('in' and 'not in' operators)")
+
+
 #
 # Database
 #
@@ -349,6 +353,9 @@ cdef class DB:
 
         return WriteBatch(self, None, transaction, sync)
 
+    def __contains__(self, key):
+        contains_is_unsupported()
+
     def __iter__(self):
         if self._db is NULL:
             raise RuntimeError("Database is closed")
@@ -488,6 +495,9 @@ cdef class PrefixedDB:
     def write_batch(self, *, transaction=False, bool sync=False):
         return WriteBatch(self.db, self.prefix, transaction, sync)
 
+    def __contains__(self, key):
+        contains_is_unsupported()
+
     def __iter__(self):
         return self.iterator()
 
@@ -708,6 +718,9 @@ cdef class BaseIterator:
     def __dealloc__(self):
         self.close()
 
+    def __contains__(self, key):
+        contains_is_unsupported()
+
     def __enter__(self):
         return self
 
@@ -1165,6 +1178,9 @@ cdef class Snapshot:
 
         return db_get(self.db, key, default, read_options)
 
+    def __contains__(self, key):
+        contains_is_unsupported()
+
     def __iter__(self):
         return self.iterator()
 


Test patch:
diff --git a/test/test_plyvel.py b/test/test_plyvel.py
index c91713e..0b76086 100644
--- a/test/test_plyvel.py
+++ b/test/test_plyvel.py
@@ -2,6 +2,7 @@
 
 from __future__ import unicode_literals
 
+import functools
 import itertools
 import os
 import random
@@ -1174,3 +1175,33 @@ def test_context_manager(db_dir):
         assert db.get(key) == value
 
     assert db.closed
+
+
+def test_in_operator(db):
+    """The ‘in’ and ‘not in’ operators should not work."""
+    raises = functools.partial(
+        pytest.raises, TypeError, match="__contains__ is not supported"
+    )
+    key = b"key"
+    with raises():
+        key in db
+    with raises():
+        key not in db
+
+    snapshot = db.snapshot()
+    with raises():
+        key in snapshot
+    with raises():
+        key not in snapshot
+
+    prefixed_db = db.prefixed_db(b"k")
+    with raises():
+        key in prefixed_db
+    with raises():
+        key not in prefixed_db
+
+    iterator = iter(db)
+    with raises():
+        key in iterator
+    with raises():
+        key not in iterator