feat: add iter()/aiter() for lazy filter-based key iteration - #682
feat: add iter()/aiter() for lazy filter-based key iteration#682Aryan-Pardeshi wants to merge 3 commits into
Conversation
…ted __iter__/__aiter__, restore conftest
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
Reviewed by Cursor Bugbot for commit 441a51d. Configure here.
…H+LIMIT FT.SEARCH + LIMIT is capped by MAXSEARCHRESULTS and non-deterministic without a unique sort, which is exactly the large-index case this API targets. The repo already has _iter_keys_by_filter for this reason -- it pages with FT.AGGREGATE ... WITHCURSOR and always releases the cursor. Delegate to it instead of reimplementing offset-based paging. Caught by Cursor Bugbot on review, verified against the existing _iter_keys_by_filter docstring and callers (drop_by_filter, update_by_filter).
|
Good catch — pushed a fix. |
vishal-bala
left a comment
There was a problem hiding this comment.
Thanks for this. The mechanics are right and the docstrings are candid about the trade-offs, which made it straightforward to review. I ran it against Redis 8.2.7 and iteration returned every key, filtered correctly with both a Tag builder and a raw "@category:{A}" string, and paged properly at batch_size=2. Five inline comments, plus two things here.
Please rename both methods to iter_keys, on SearchIndex and AsyncSearchIndex alike. Every other paired method in the file keeps one name across the two classes, paginate included, so porting a loop by swapping the class currently raises AttributeError. iter_keys also says what gets yielded and reads well next to drop_keys. Leaving out the __iter__/__aiter__ dunders was the right call for the reason you gave.
The description still describes your first commit, with _query, return_fields=["id"], and keys yielded "without ever building the full list". Both changed in the FT.AGGREGATE rewrite. Please update it.
| """Iteration must stream: the first key arrives without draining the index.""" | ||
| iterator = sample_index.iter() | ||
|
|
||
| assert next(iterator) is not None |
There was a problem hiding this comment.
This assertion can't fail. Keys are strings, so is not None holds for anything the generator yields, and an empty index would surface as an uncaught StopIteration rather than a failed assertion.
I checked by swapping in an iter() that drains the whole index into a list before yielding anything. The test still passed, so it can't tell a streaming implementation from an eager one, which is the one property it's named for.
What would work is counting FT.AGGREGATE round trips before the first key arrives. tests/unit/test_bulk_cursor_dedup.py already has a _ReplayCursor harness that fakes client.ft(name), so this can be a hermetic unit test with no server and no fixture. While you're there, the async method has no laziness test at all.
| async for batch in self._iter_keys_by_filter(filter_expr, batch_size): | ||
| for key in batch: | ||
| yield key |
There was a problem hiding this comment.
The server-side cursor can outlive this generator. Measured on Redis 8.2.7: break out of async for key in index.aiter() early, then await index.disconnect(), and FT.INFO ... cursor_stats shows index_total climbing 1, 2, 3 across three asyncio.run lifecycles. The cursor is never released. The event loop's async-generator finalisation tries to send FT.CURSOR DEL after the client has gone, and the except RedisError: pass in the helper swallows the failure. Each leaked cursor is held for MAXIDLE 300 seconds, against a per-shard capacity of 128.
Two parts. Wrapping the inner generator makes an explicit close deterministic:
async with contextlib.aclosing(
self._iter_keys_by_filter(filter_expr, batch_size)
) as batches:
async for batch in batches:
for key in batch:
yield keyI measured the cursor released the instant aclose() returns with that in place, versus still open without it. The abandon-and-never-close case can't be fixed from in here, so the docstring should tell callers who break out early to wrap the iterator in contextlib.aclosing().
The sync path needs nothing. I watched index_total drop back to 0 on close(), on del plus a collection, and on break.
| filter_expr = ( | ||
| FilterExpression("*") if filter_expression is None else filter_expression | ||
| ) |
There was a problem hiding this comment.
This wrapper is a no-op, and bypassing the existing helper introduces a divergence. str(FilterExpression("*")) is just "*", and _iter_keys_by_filter already accepts a plain str.
_is_match_all_filter at index.py:236 is what drop_by_filter and update_by_filter use for this normalisation, and it treats "" and whitespace as match-all as well as None. Measured: iter("") and iter(" ") return zero keys here, while drop_by_filter("") matches the whole index. So a filter that arrives empty from config means nothing to one method and everything to its neighbour. iter(FilterExpression()) also raises the bare ValueError("Improperly initialized FilterExpression") that the helper exists to absorb.
Routing through it fixes all three cases and deletes these three lines.
| def iter( | ||
| self, | ||
| filter_expression: str | FilterExpression | None = None, | ||
| batch_size: int = DEFAULT_BULK_BATCH_SIZE, |
There was a problem hiding this comment.
batch_size wants the same guards paginate has fifteen lines up: TypeError for a non-int, ValueError for anything below 1.
Measured on Redis 8.2.7. batch_size=0 silently returns every key, because redis-py drops a falsy COUNT and the server picks its own page size, which makes "Defaults to 500" in the docstring untrue. batch_size="5" sails straight through. batch_size=-1 surfaces the raw server text Bad arguments for COUNT: Value is outside acceptable bounds inside a RedisSearchError.
Matching paginate is enough. Worth knowing that neither will raise at call time, since validation inside a generator function doesn't run until the first next(). If you'd rather it fail eagerly, the checks have to live in a non-generator wrapper that returns the generator. Your call.
| Delegates to :meth:`_iter_keys_by_filter`, which pages with | ||
| ``FT.AGGREGATE ... WITHCURSOR`` rather than ``FT.SEARCH`` + ``LIMIT``, so | ||
| this is not subject to the ``MAXSEARCHRESULTS`` limit. See that method's | ||
| docstring for why keys are de-duplicated and why memory is | ||
| ``O(match count)`` rather than truly streaming. |
There was a problem hiding this comment.
These caveats point at something the published docs don't contain. docs/api/searchindex.rst uses autoclass ... :members: with no :private-members:, so iter appears on docs.redisvl.com but _iter_keys_by_filter doesn't. The :meth: reference is dangling and the memory and completeness caveats are invisible to exactly the reader who needs them. The async method is a longer chain still, because the async helper's body is "See the sync counterpart".
Please inline the load-bearing sentences: memory is proportional to the match count, so very large scans should partition the filter; a batch can come back smaller than batch_size; and a drained cursor isn't proof every match was seen. That last one matters most, because a method named for iteration reads as exhaustive. Keep the seen set as it is — dropping it would hand callers the same document twice, and since RediSearch reindexes an updated document under a new, higher id, writing during iteration then provokes further repeats.
Two more worth adding while you're in here. drop_by_filter and update_by_filter carry the only "never string-concatenate untrusted input into a filter" warning in the package, and these accept the same parameter type without it. And the cursor's idle timeout only resets when a page is read, so a caller doing per-key embedding or network work can have its cursor reaped after keys have already been yielded, which fails mid-stream rather than up front.
One softening, too. The MAXSEARCHRESULTS framing is accurate but reads scarier than it is. I measured the default at 1,000,000 on Redis 8.2.7, so it only bites above a million matches. Naming the figure would help a reader judge whether it applies to them.

Fixes #489
Adds
iter()andaiter()for lazy, filter-based iteration over the keys in an index.Neither existing API covers this:
paginate()yields full document records rather than keys, andscan_by_pattern()works on raw Redis key patterns and materialises a list. For index maintenance over a large index you want keys only, streamed, and selectable by filter expression.Both take an optional
FilterExpression(defaulting to match-all) and abatch_sizedefaulting toDEFAULT_BULK_BATCH_SIZE, page through_querywithreturn_fields=["id"], and yield keys one at a time without ever building the full list. The async version mirrors the sync one exactly.tests/integration/test_index_iteration.pycovers full iteration, filtered iteration, laziness (the first key arrives without draining the index), and abatch_sizesmaller than the document count so the paging loop is actually exercised rather than short-circuiting on a single batch. 7 passed against Redis in Docker.Two things I want to flag rather than have you find:
itershadows the builtin as a method name. That is what the issue asked for, but if you would rather haveiter_keys/aiter_keysI will rename without argument.I did not add
__iter__/__aiter__dunders. Making aSearchIndexdirectly iterable would meanlist(index)silently issues a full paged scan against Redis, which felt like a surprising thing to attach to a plainforloop. Easy to add if you want it.Note
Low Risk
Additive public API on top of an existing bulk helper; no changes to auth, persistence, or bulk mutation semantics beyond new iteration entry points.
Overview
Adds
iter()onSearchIndexandaiter()onAsyncSearchIndexso callers can walk document Redis keys (not full records) with an optionalFilterExpressionand configurablebatch_size, defaulting to match-all when the filter is omitted.Both methods are thin wrappers around the existing
_iter_keys_by_filterpath (FT.AGGREGATE+WITHCURSOR), so iteration avoidsMAXSEARCHRESULTScaps that affectFT.SEARCH+LIMIT, with the same de-duplication and memory characteristics documented on that helper.Integration tests in
test_index_iteration.pycover full and filtered scans, lazy consumption, and paging whenbatch_sizeis smaller than the index size for sync and async.Reviewed by Cursor Bugbot for commit 69dd491. Bugbot is set up for automated code reviews on this repo. Configure here.