Address review: clean error messages and validate stored hash segments

Collapse backslash line-continuations inside ValueError message
literals (13 sites in minhash.py, lean_minhash.py, b_bit_minhash.py) --
they embedded the source indentation into user-facing error text.

Validate segment bytes in MinHashLSHForest.get_minhash_hashvalues
before inferring the value width: the segment length must be a whole
number of 4- or 8-byte values and all segments must agree, so corrupted
or truncated storage raises ValueError instead of being silently
reinterpreted with the wrong dtype. Add a corrupt-segment test.

Author: ekzhu

datasketch/b_bit_minhash.py

@@ -83,18 +83,15 @@ def jaccard(self, other):
         """
         if self.b != other.b:
             raise ValueError(
-                "Cannot compare two b-bit MinHashes with different\
-                    b values"
+                "Cannot compare two b-bit MinHashes with different b values"
             )
         if self.scheme != other.scheme:
             raise ValueError(
-                "Cannot compare two b-bit MinHashes with different\
-                    permutation schemes"
+                "Cannot compare two b-bit MinHashes with different permutation schemes"
             )
         if self.seed != other.seed:
             raise ValueError(
-                "Cannot compare two b-bit MinHashes with different\
-                    set of permutations"
+                "Cannot compare two b-bit MinHashes with different set of permutations"
             )
         intersection = np.count_nonzero(self.hashvalues == other.hashvalues)
         raw_est = float(intersection) / float(self.hashvalues.size)

datasketch/lean_minhash.py

@@ -248,8 +248,7 @@ def serialize(self, buf, byteorder="@") -> None:
         """
         if len(buf) < self.bytesize(byteorder):
             raise ValueError(
-                "The buffer does not have enough space\
-                    for holding this MinHash."
+                "The buffer does not have enough space for holding this MinHash."
             )
         if self.scheme == _SCHEME_LEGACY:
             fmt = "%sqi%dI" % (byteorder, len(self))
@@ -351,8 +350,7 @@ def union(cls, *lmhs: LeanMinHash) -> LeanMinHash:
         scheme = lmhs[0].scheme
         if any((seed != m.seed or num_perm != len(m) or scheme != m.scheme) for m in lmhs):
             raise ValueError(
-                "The unioning MinHash must have the\
-                    same seed, number of permutation functions and scheme."
+                "The unioning MinHash must have the same seed, number of permutation functions and scheme."
             )
         hashvalues = np.minimum.reduce([m.hashvalues for m in lmhs])
 

datasketch/lshforest.py

@@ -150,10 +150,20 @@ def get_minhash_hashvalues(self, key: Hashable) -> np.ndarray:
                 f"The provided key does not exist in the LSHForest: {key}")
         # Each stored segment packs self.k hash values; derive the value byte
         # width from the segment size (4 for the "affine32" MinHash scheme,
-        # 8 for "affine64" and "legacy").
-        bytes_per_value = len(byteslist[0]) // self.k
+        # 8 for "affine64" and "legacy"). Validate before trusting it, so
+        # corrupted or truncated storage raises instead of being silently
+        # reinterpreted with the wrong width.
+        segment_size = len(byteslist[0])
+        bytes_per_value, remainder = divmod(segment_size, self.k)
+        if remainder != 0 or bytes_per_value not in (4, 8):
+            raise ValueError(
+                "Stored hash segment of %d bytes is not %d values of a "
+                "supported width (4 or 8 bytes)" % (segment_size, self.k)
+            )
+        if any(len(item) != segment_size for item in byteslist):
+            raise ValueError("Stored hash segments have inconsistent sizes")
         dtype = np.uint32 if bytes_per_value == 4 else np.uint64
-        values_per_segment = len(byteslist[0]) // np.dtype(dtype).itemsize
+        values_per_segment = segment_size // np.dtype(dtype).itemsize
         hashvalues = np.empty(
             len(byteslist) * values_per_segment, dtype=dtype)
         for index, item in enumerate(byteslist):

datasketch/minhash.py

@@ -279,8 +279,7 @@ def __init__(
             # Because 1) we don't want the size to be too large, and
             # 2) we are using 4 bytes to store the size value
             raise ValueError(
-                "Cannot have more than %d number of\
-                    permutation functions"
+                "Cannot have more than %d number of permutation functions"
                 % _hash_range
             )
         self.seed = seed
@@ -556,18 +555,15 @@ def jaccard(self, other: MinHash) -> float:
         """
         if getattr(other, "scheme", None) != self.scheme:
             raise ValueError(
-                "Cannot compute Jaccard given MinHash with\
-                    different permutation schemes"
+                "Cannot compute Jaccard given MinHash with different permutation schemes"
             )
         if other.seed != self.seed:
             raise ValueError(
-                "Cannot compute Jaccard given MinHash with\
-                    different seeds"
+                "Cannot compute Jaccard given MinHash with different seeds"
             )
         if len(self) != len(other):
             raise ValueError(
-                "Cannot compute Jaccard given MinHash with\
-                    different numbers of permutation functions"
+                "Cannot compute Jaccard given MinHash with different numbers of permutation functions"
             )
         return float(np.count_nonzero(self.hashvalues == other.hashvalues)) / float(len(self))
 
@@ -597,18 +593,15 @@ def merge(self, other: MinHash) -> None:
         """
         if getattr(other, "scheme", None) != self.scheme:
             raise ValueError(
-                "Cannot merge MinHash with\
-                    different permutation schemes"
+                "Cannot merge MinHash with different permutation schemes"
             )
         if other.seed != self.seed:
             raise ValueError(
-                "Cannot merge MinHash with\
-                    different seeds"
+                "Cannot merge MinHash with different seeds"
             )
         if len(self) != len(other):
             raise ValueError(
-                "Cannot merge MinHash with\
-                    different numbers of permutation functions"
+                "Cannot merge MinHash with different numbers of permutation functions"
             )
         self.hashvalues = np.minimum(other.hashvalues, self.hashvalues)
 
@@ -707,8 +700,7 @@ def union(cls, *mhs: MinHash) -> MinHash:
         scheme = mhs[0].scheme
         if any((seed != m.seed or num_perm != len(m) or scheme != m.scheme) for m in mhs):
             raise ValueError(
-                "The unioning MinHash must have the\
-                    same seed, number of permutation functions and scheme"
+                "The unioning MinHash must have the same seed, number of permutation functions and scheme"
             )
         hashvalues = np.minimum.reduce([m.hashvalues for m in mhs])
         permutations = mhs[0].permutations

test/test_lshforest.py

@@ -75,6 +75,21 @@ def test_get_minhash_hashvalues(self):
             for i in range(len(retrieved_hashvalues)):
                 self.assertEqual(hashvalues[i], retrieved_hashvalues[i])
 
+    def test_get_minhash_hashvalues_rejects_corrupt_segments(self):
+        forest, data = self._setup()
+        key = next(iter(data))
+        segments = forest.keys[key]
+        # Truncated first segment: no longer a whole number of hash values
+        # of a supported width, must not be silently reinterpreted.
+        forest.keys[key] = [segments[0][:-3], *segments[1:]]
+        with self.assertRaises(ValueError):
+            forest.get_minhash_hashvalues(key)
+        # First segment intact but a later one truncated: inconsistent sizes.
+        forest.keys[key] = [*segments[:-1], segments[-1][: len(segments[-1]) // 2]]
+        with self.assertRaises(ValueError):
+            forest.get_minhash_hashvalues(key)
+        forest.keys[key] = segments
+
     def test_pickle(self):
         forest, _ = self._setup()
         forest2 = pickle.loads(pickle.dumps(forest))