Repository navigation
fix(index): return the Redis key as id for whole-document JSON FilterQuery results - #793
Draft
vishal-bala wants to merge 2 commits into
Draft
vishal-bala wants to merge 2 commits into
vishal-bala wants to merge 2 commits into
Conversation
…Query results A JSON FilterQuery without return_fields spread the document over the result after setting id, so a document field named id replaced the key. The key now always wins and the document's own id field is dropped, matching hash results and every query that sets return_fields.
Restore the process_aggregate_results docstring, which the previous commit overwrote with process_results wording that does not apply to it, and limit the queries.md guarantee to FT.SEARCH-based queries. Add a hermetic unit test for the JSON unpack path.
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.
Motivation
A search result's
idis the record's full Redis key in every case but one. A JSONFilterQuerywithoutreturn_fieldsreturns the whole document, andprocess_resultsbuilt that result as{"id": key, **document}, so a document with its ownidfield overwrote the key. Measured on Redis 8.4, a document loaded withindex.load([{"id": "t1", "status": "open"}], id_field="id")came back as{'id': 't1', 'status': 'open'}fromFilterQuery(Tag("status") == "open"), and as{'id': '<prefix>:t1', 'status': 'open'}oncereturn_fields=["status"]was added. Hash storage and every other search query already return the key. Code that passesresult["id"]todrop_keys(),fetch()or an update therefore worked on hash and broke on JSON, depending only on whetherreturn_fieldswas set.Changes
The key always wins as
idThe JSON unpack branch of
process_resultsinredisvl/index/index.pynow pops the document's ownidbefore merging it under the key. BothSearchIndexandAsyncSearchIndexroutequery(),paginate()andbatch_query()through that one function, so the sync and async paths change together.The document's
idfield is dropped rather than kept under another name, because that is what hash results already do. redis-py's searchResultparser runsdel fields["id"]on every returned field set, in both its RESP2 and RESP3 paths, so a hash field namedidhas never appeared in a RedisVL result. Renaming the JSON field to something like_idwould make the two storage types differ again, and would claim a field name that a user's document could also be using.Documentation and tests
docs/concepts/queries.mdgains a "Result ids" section. It states the guarantee for search queries, notes that aggregation, hybrid and SQL rows do not setidto the key, and gives three ways to read a document's ownidback.tests/integration/test_search_results.pyadds a sync and an async test that each sweep hash and JSON storage, with and withoutreturn_fields.tests/unit/test_query_types.pyadds a hermetic test of the JSON unpack branch.Notes
The guarantee covers FT.SEARCH-based queries only.
AggregationQuery,AggregateHybridQuery,HybridQueryandSQLQueryreturn rows of loaded fields throughprocess_aggregate_resultsor_convert_and_drop_empty_rows, and a field loaded under the nameidkeeps the document's value there. Those rows are projections rather than documents, so they are documented as outside the guarantee rather than changed.Next Steps
The MCP code-tools stack (#758–#761, branches
feat/mcp-code-tools/01to04) works around the old behaviour in documentation: theIndexHandle.document_iddocstring inredisvl/mcp/context.py(#758), and one sentence each indocs/concepts/mcp.mdanddocs/user_guide/how_to_guides/mcp.mdtelling users to setreturn_fieldson a JSONFilterQuery(#760). Whichever of this PR and that stack lands second should remove those caveats.Release Notes
On JSON indexes, a
FilterQuerywithoutreturn_fieldsnow returns each record's full Redis key asid, as hash indexes already did, and no longer includes the document's ownidfield, which previously replaced the key. To read that field back, add"$.id"toreturn_fields(it is returned under$.id), index it under another name such as{"name": "doc_id", "type": "tag", "path": "$.id"}, or load the document withindex.fetch().Reviewer Brief
Start here:
process_resultsinredisvl/index/index.py, theunpack_jsonbranch. It is the only behavioural change, and every sync and async search result passes through it.Where it could fail
redisvl/index/index.py:534: JSON users who readresult["id"]from a whole-documentFilterQueryand expected the document's own field now get the key; check the Release Notes recovery routes cover that case.docs/concepts/queries.md, "Result ids": the guarantee is scoped to search queries; check the wording does not promise anything for aggregation, hybrid or SQL rows.Not verified: cluster mode, and Redis versions other than 8.4. The change touches no API-key paths.
Beyond CI: both new integration tests fail on
mainat the('json', None)case and pass with the fix, and the new unit test fails without thepop; all three recovery routes in the Release Notes were run against Redis 8.4 and returnedt1.Reviewed
08d89812, fixes inc7126d5d· Claude Code (claude-opus-5-5) · lenses: correctness, security, design, tests, docs, Redis behaviour