fix(deduplicator): reset hash_tables between process() calls - #1043
Conversation
hash_tables was initialized once in __init__ but never cleared when process() was called again on the same operator instance. A prior call (e.g. probe/preview) would leave stale entries that pollute subsequent deduplication, causing false-positive merges via UnionFind. Part of #137
| # Reset hash tables to avoid stale state from previous calls | ||
| self.hash_tables = [defaultdict(set) for _ in range(self.num_bands)] | ||
|
|
There was a problem hiding this comment.
If process() is called with an empty or single-sample dataset, hash_tables from the previous call remains populated on the instance. The next large call does reset before use, so output is currently safe, but this breaks the stated invariant ("reset at the top of process()") and leaves stale state observable after a small/empty call.
Please move the reset before the early return, ideally with a shared helper:
def _reset_hash_tables(self):
self.hash_tables = [defaultdict(set) for _ in range(self.num_bands)]
and call self._reset_hash_tables() as the first statement in both process() implementations.
| unittest.main() | ||
|
|
||
|
|
||
| class DocumentMinhashDeduplicatorRepeatedCallTest(DataJuicerTestCaseBase): |
There was a problem hiding this comment.
The new test class is defined after the main guard:
if __name__ == '__main__':
unittest.main()
class DocumentMinhashDeduplicatorRepeatedCallTest(...):
...python test_document_minhash_deduplicator.py will run unittest.main() and exit before the class definition is ever executed, so the new tests are not discoverable when the file is run directly. Please move the class above line 978, or move the if __name__ == '__main__' guard to the end of the file.
|
|
||
| # Reset hash tables to avoid stale state from previous calls | ||
| self.hash_tables = [defaultdict(set) for _ in range(self.num_bands)] | ||
|
|
There was a problem hiding this comment.
Same issue as above for DocumentMinhashDeduplicatorWithUid.process(): reset at line 391 should occur before the if len(dataset) <= 1 early return at lines 388–389. If you add _reset_hash_tables() to the base class, both overrides can simply call it first.
Reproduction
The same 3-sample dataset processed with the same parameters gives different deduplication results depending on whether the operator was previously used on other data.
Before fix
Same input, same parameters — 1 sample falsely deduplicated because stale hash-table entries from the warmup run caused a spurious UnionFind merge.
After fix
Results are identical regardless of operator history.
Root cause
self.hash_tables(list ofdefaultdict(set)) is created once in__init__and populated with sample indices insideprocess(), but never cleared between calls. Index entries from a priorprocess()call persist and participate in clustering on subsequent calls, merging unrelated samples.Fix
Reset
self.hash_tablesat the top ofprocess():Applied to both
DocumentMinhashDeduplicatorandDocumentMinhashDeduplicatorWithUid.Tests added
DocumentMinhashDeduplicatorRepeatedCallTest— callsprocess()twice on the same operator instance with disjoint datasets, asserts no cross-contamination. Covers both the base class and the UID variant.