Repository navigation
Conversation
Collaborator
|
Hi, thanks for your contribution! Can you rebase onto the latest |
ningyixi
force-pushed
the
fix/load-iterator-keys
branch
from
October 22, 2026 15:15
5a4abe7 to
972b789
Compare
Author
|
Rebased onto latest |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
SearchIndex.load()andAsyncSearchIndex.load()accept iterable data and keys, but passing an iterator for either argument with explicit keys currently raisesTypeErrorbecause storage callslen()before materializing the inputs. Explicit empty keys also bypass the length check and silently generate different keys for nonempty data.Materialize each iterable once before checking lengths in both storage write paths, and distinguish omitted keys from an explicit empty iterable. This preserves the existing validation error and rejects mismatches before any pipeline writes.
The integration tests use Redis 8.4 via the repository's Testcontainers setup, exercise HASH and JSON storage, sync and async APIs, all list/iterator combinations, and shorter/longer/empty keys with no partial writes.
Validation (Python 3.12.14):
make format,make check-types(120 source files),make check-format check-sort-imports,uv lock --check, and pre-commit on both changed files pass.make testwas attempted with the default locked development dependencies: 1,892 passed, 153 skipped, 2 xfailed; 195 failures and 154 setup errors reference missing optional packages (sentence_transformers,sql_redis,google,voyageai,pydantic_settings). This is not a passing full-suite result; provider/model tests were not run with credentials.pre-commit run --all-filespasses the code-quality hook but Codespell rewrites an existingUnparseablecomment in untouchedredisvl/redis/connection.py. That unrelated rewrite was reverted; the changed-file hooks pass.Suggested release label:
auto:patch.Implemented and tested with OpenAI Codex assistance.
Review record
Reviewed at
972b789by Grok Bot withpre-pr-review.Lenses: correctness (always; 12-line storage fix plus sync/async twin); tests (adds integration coverage for iterators and key-length mismatches). Skipped: security (no query-building, MCP, or destructive paths); design (bugfix, no API shape change); docs (behaviour already documented as Iterable); Redis behaviour (no new Redis commands or reply-shape assumptions).
_get_keysstill consumes a single-pass keys iterable twice (storage.py:185) (dead code — never called; out of scope);_preprocess_and_validate_objectsstill uses truthiness for keys (storage.py:308) (write/awrite already reject empty keys vs nonempty data before calling it); string mistaken for Iterable[str] would split into characters (pre-existing contract, not introduced here).Verified:
uv run black/isort --profile blackon the two changed files (unchanged);uv run mypy redisvl/index/storage.py(no issues).Not checked: integration tests in
tests/integration/test_load_iterables.py(no Docker on this machine); cluster mode; fullmake test.Note
Low Risk
Localized change to load/write input handling and validation order; behavior fix for iterables and explicit empty keys, covered by new integration tests.
Overview
Fixes
SearchIndex.load/AsyncSearchIndex.loadwhendataor explicitkeysare single-pass iterators: storage now materializes both to lists once before length checks and preprocessing, solen()no longer raisesTypeErrorand iterators are not consumed inconsistently.The keys-vs-objects check now uses
keys is not Noneinstead of truthiness, so an explicit emptykeysiterable is validated against nonempty data instead of being treated like omitted keys and auto-generating different keys.The same logic applies to sync
writeand asyncawrite. New integration tests cover HASH/JSON, sync/async, list/iterator combinations, and mismatched key counts with no partial Redis writes before the error.Reviewed by Cursor Bugbot for commit 5a4abe7. Bugbot is set up for automated code reviews on this repo. Configure here.