Skip to content

feat: add iter()/aiter() for lazy filter-based key iteration - #682

Open
Aryan-Pardeshi wants to merge 3 commits into
redis:mainfrom
Aryan-Pardeshi:feat/index-lazy-key-iteration
Open

feat: add iter()/aiter() for lazy filter-based key iteration#682
Aryan-Pardeshi wants to merge 3 commits into
redis:mainfrom
Aryan-Pardeshi:feat/index-lazy-key-iteration

Conversation

@Aryan-Pardeshi

@Aryan-Pardeshi Aryan-Pardeshi commented Aug 9, 2026

Copy link
Copy Markdown

Fixes #489

Adds iter() and aiter() for lazy, filter-based iteration over the keys in an index.

Neither existing API covers this: paginate() yields full document records rather than keys, and scan_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.

for key in index.iter(Tag("category") == "A"):
    ...

Both take an optional FilterExpression (defaulting to match-all) and a batch_size defaulting to DEFAULT_BULK_BATCH_SIZE, page through _query with return_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.py covers full iteration, filtered iteration, laziness (the first key arrives without draining the index), and a batch_size smaller 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:

iter shadows the builtin as a method name. That is what the issue asked for, but if you would rather have iter_keys/aiter_keys I will rename without argument.

I did not add __iter__/__aiter__ dunders. Making a SearchIndex directly iterable would mean list(index) silently issues a full paged scan against Redis, which felt like a surprising thing to attach to a plain for loop. 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() on SearchIndex and aiter() on AsyncSearchIndex so callers can walk document Redis keys (not full records) with an optional FilterExpression and configurable batch_size, defaulting to match-all when the filter is omitted.

Both methods are thin wrappers around the existing _iter_keys_by_filter path (FT.AGGREGATE + WITHCURSOR), so iteration avoids MAXSEARCHRESULTS caps that affect FT.SEARCH + LIMIT, with the same de-duplication and memory characteristics documented on that helper.

Integration tests in test_index_iteration.py cover full and filtered scans, lazy consumption, and paging when batch_size is 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.

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.

Fix All in Cursor

Reviewed by Cursor Bugbot for commit 441a51d. Configure here.

Comment thread redisvl/index/index.py Outdated
…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).
@Aryan-Pardeshi

Copy link
Copy Markdown
Author

Good catch — pushed a fix. iter()/aiter() now delegate to _iter_keys_by_filter(), which pages with FT.AGGREGATE ... WITHCURSOR instead of FT.SEARCH + LIMIT, so this is no longer subject to MAXSEARCHRESULTS. Tests still pass (7/7), full unit suite green (1292 passed).

@vishal-bala vishal-bala self-assigned this Aug 13, 2026
@vishal-bala vishal-bala removed their assignment Sep 3, 2026
@vishal-bala
vishal-bala self-requested a review September 3, 2026 09:07

@vishal-bala vishal-bala left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.

Comment thread redisvl/index/index.py
Comment on lines +3300 to +3302
async for batch in self._iter_keys_by_filter(filter_expr, batch_size):
for key in batch:
yield key

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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 key

I 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.

Comment thread redisvl/index/index.py
Comment on lines +2027 to +2029
filter_expr = (
FilterExpression("*") if filter_expression is None else filter_expression
)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.

Comment thread redisvl/index/index.py
def iter(
self,
filter_expression: str | FilterExpression | None = None,
batch_size: int = DEFAULT_BULK_BATCH_SIZE,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.

Comment thread redisvl/index/index.py
Comment on lines +2013 to +2017
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.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.

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.

Add iter()/aiter() for filter-based lazy iteration

2 participants