Repository navigation
feat(executor): parse RESP3 map replies for FT.SEARCH and FT.AGGREGATE - #41
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
sql-redis sends FT.* commands with client.execute_command and reads the
raw reply. redis-py registers no FT.SEARCH / FT.AGGREGATE response
callback on the base client (Search.search() calls execute_command and
then its own _parse_results), so the executor receives the wire shape
verbatim: a flat array on RESP2, a map on RESP3.
The parser assumed the array, so on a RESP3 connection its positional
strides walked into integer keys that do not exist. FT.SEARCH raised
KeyError: 2 and FT.AGGREGATE raised KeyError: slice(1, None, None).
Since redis-py 8 defaults to RESP3, this broke on upgrade without the
caller passing protocol at all.
_parse_reply now dispatches on the shape of the reply, and
_resp3_to_resp2 folds a map back into the array shape so one parser
handles both protocols. The four branch behaviours (score-alias
resolution over the union of row field names, the collision loop, nil
field-array tolerance, count extraction) move into _parse_array_reply
unchanged, which is what keeps protocol=2 output identical. Both
execute() bodies lose their duplicated parse block.
Shape dispatch rather than a protocol check on the connection: sniffing
connection_kwargs is wrong for cluster and sentinel clients, and cannot
distinguish redis-py 8's four callback modes. Routing through
client.ft(index) does not help either, since redis-py 7.4 and earlier
return the raw map from _parse_results under RESP3, and redis-py 8's
protocol=3 callback table has no FT.AGGREGATE entry.
Structural map keys arrive as bytes when decode_responses=False, so
_map_get probes both key types; reading only str keys fails silently
with zero rows, which is the bug redis-py shipped in 8.0.0 (#4107).
Document field keys are passed through untouched, as before.
FT.HYBRID keeps its own parser: measured against Redis 8.4.6, its RESP3
rows are flat field maps, not the {id, extra_attributes} documents
FT.SEARCH returns. FT.INFO and FT._LIST were already shape-tolerant.
Verified against Redis 8.4.6 across redis-py {6.3.0, 7.4.0, 8.1.0} and
client modes {protocol=2, unset, protocol=3, legacy_responses=False,
protocol=3 with decode_responses=False}, sync and async: rows and counts
are identical to the protocol=2 baseline in all 30 cells. RESP3
total_results matches the RESP2 leading count for FT.AGGREGATE too.
The one accepted divergence is the score's type, float on RESP3 and str
on RESP2, since Redis sends a double there. It is passed through rather
than normalised; float() is the protocol-safe conversion at the call
site and is what the existing tests already use.
…patch
Six-perspective review of the RESP3 change. Addresses what it found.
Measured, not assumed: RESP2 and RESP3 do NOT agree on QueryResult.count
for an FT.AGGREGATE without GROUPBY. A computed-field projection over 5
documents reports count=1 on RESP2, the placeholder the Redis docs call
"not a valid value", and count=5 on RESP3. With a GROUP BY the two do
agree. The previous docs asserted agreement unconditionally, and the
tests happened to use a GROUP BY whose group count equalled its row
count, so nothing caught it. Neither value can be derived from the other
without changing protocol=2 output, so both are surfaced as Redis sent
them, now documented and pinned by tests on both the stub and live
paths. The pre-existing claim that the aggregate count equals the row
count after LIMIT was also false and is corrected.
Reply dispatch now rejects what it cannot parse. A map without a
'results' key (a cluster node-keyed reply, an FT.PROFILE reply) folded
to zero rows, and a non-array non-map reply reached positional indexing.
Both were silent wrong answers of exactly the kind the new FAILURE_MODES
entry warns about; both now raise with the offending shape named.
The reply layout is derived once. _fold_map_to_array and
_parse_array_reply each re-derived with_scores and has_fields from the
translation, and had to agree or the parser would stride over slots the
fold never emitted, mis-attributing one document's data to another. A
frozen _ReplyLayout is now computed once in _parse_reply and passed to
both. Unreachable today, since RETURN 0 is only ever emitted alongside
WITHSCORES, but the invariant protecting it lived three modules away.
count no longer differs from the hybrid parser when total_results is
present but nil, and _field_array accepts the flat form its FT.HYBRID
counterpart already accepted.
_ScoreParseMixin is gone. Every method was a staticmethod called through
the explicit class name, so it offered the syntax of inheritance with
none of its semantics, and only two of eight methods concerned scores.
The parsers are module-level functions, which is what the new helpers
already were; the mixed self./class-name style went with it.
Tests: count provenance is now asserted where total_results differs from
the row count, which a surviving mutant showed nothing checked. Added
the aggregate count divergence, shape rejection, the error re-wrapping
paths (the async copy was the last real coverage hole, 16 lines), the
aggregate missing-fields branch, and a canary for what an unset protocol
negotiates per redis-py major, which a doc claim rested on. Removed two
tests subsumed by exact-equality assertions and replaced a bookkeeping
denylist with a column allowlist, so a new RESP3 result key fails a test
instead of appearing in every row. Unordered aggregate rows are compared
by group key rather than positionally, and score parity uses approx.
executor.py coverage 90% on main, now 99%.
The redis-py-compat job selects by a registered 'protocol' marker rather
than a filename list, so a new protocol-sensitive test is picked up
without editing the workflow.
Docs: two result-shape examples showed a bytes key with a str value,
which no client produces; RETURN 0 keeps its Factor 4 heading so the
published anchor still resolves, with the protocol material demoted to a
note; a stale pointer into Executor.execute and the schema docstrings
that conflated redis-py 8 with RESP3 are fixed; the paragraph triplicated
across FAILURE_MODES, a docstring and a test module now lives in one
place.
Verified again after the restructure: 30/30 harness cells across redis-py
{6.3.0, 7.4.0, 8.1.0} and five client modes still match the protocol=2
baseline. Suite 677 passed, 3 skipped. Marked subset: 148 on 6.3.0 and
7.4.0, 151 on 8.1.0.
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
sql-redisraisesKeyError: 2on anyFT.SEARCHorFT.AGGREGATEquery over a RESP3 connection. The executor sends commands withclient.execute_commandand reads the raw reply, and redis-py registers no response callback for either command on the base client:Search.search()callsexecute_commandand then its own_parse_results. So the executor receives the wire shape verbatim, a flat array on RESP2 and a map on RESP3, and its parser assumed the array.The failure needs no opt-in: redis-py 8 defaults to RESP3, so a caller who upgrades redis-py hits it without ever passing
protocol. Measured on Redis 8.4.6 with redis-py 8.1.0, every RESP3 client mode fails:protocolunset,protocol=3andlegacy_responses=False. Only an explicitprotocol=2works, which is why RedisVL pins its factory-created clients to it (redis/redis-vl-python#695). This PR removes the need for that pin.Changes
Shape dispatch, with a RESP3 map folded into the array shape
_parse_replydispatches on the shape of the reply rather than on the protocol the connection negotiated. A map goes through_fold_map_to_array, which rewrites it into the flat array RESP2 sends, and both shapes then reach the same row parser,_parse_array_reply.Folding rather than writing a second parser keeps the four existing branch behaviours in one place: score-alias resolution over the union of every row's field names, the collision loop, nil field-array tolerance, and count extraction.
_parse_array_replyis the pre-existing parser moved unchanged apart from one dead assignment, which is what keepsprotocol=2output identical. The approach mirrors RedisVL's own_parse_batch_search_resultin redis/redis-vl-python#695.Shape dispatch was chosen over two alternatives. Routing through
client.ft(index)fixes nothing across the declaredredis>=5.0.0range: on redis-py 7.4.0 and earlier_parse_resultsreturns the raw map under RESP3, and redis-py 8.1.0'sprotocol=3callback table has noFT.AGGREGATEentry. Readingconnection_kwargs["protocol"]is wrong for cluster and sentinel clients and cannot tell redis-py 8's four callback modes apart.Layout derived once, unrecognised replies rejected
The parser strides over the array, so the fold and the parser must agree on which slots each document occupies. A frozen
_ReplyLayoutis derived once per query and passed to both, so they cannot drift apart.A reply that is neither shape now raises a
ValueErrornaming what arrived, instead of parsing into empty rows. A map without aresultskey (a cluster node-keyed reply, anFT.PROFILEreply) and a non-array non-map would otherwise look like an empty result set.Bytes structural keys
_map_getlooks up structural keys (total_results,results,id,score,extra_attributes) asstrand then asbytes, because they arrive as bytes whendecode_responses=False. Reading onlystrkeys returns zero rows without raising, which is the bug redis-py shipped in 8.0.0 (redis/redis-py#4107). Document field keys are passed through untouched, as before.Executor structure
_ScoreParseMixinis removed. Every method was a@staticmethodcalled through the explicit class name, and only two of eight concerned scores, so the reply parsers are now module-level functions shared byExecutorandAsyncExecutor. Bothexecute()methods lose their duplicated 65-line parse block.Tests and CI
tests/test_reply_shapes.py: canned replies through both executors, every literal captured from Redis 8.4.6 rather than written by hand.tests/test_protocol_matrix.py: the same behaviour against a real server across all four client modes, plus a canary asserting each protocol really sends the shape assumed.redis-py-compatCI job runs the tests markedprotocolagainst redis-py 6.3.0, 7.4.0 and 8.1.0, after checking the overlay installed the requested version.tests/conftest.py, andtests/test_nil_field_array.pyuses them.docs/concepts/result-shape.mdcovers the protocol-dependent values, anddocs/for-ais-only/FAILURE_MODES.mdrecords why dispatch is by shape.Notes
The score's type differs by protocol and is deliberately not normalised. RESP3 sends a double, so callers get a
float; RESP2 sends text, so they get astrorbytes. Stringifying the double cannot reproduce the text Redis would have sent on RESP2, so normalising would produce a third form rather than parity.float()is the protocol-safe conversion and works on both.countfor anFT.AGGREGATEwithout aGROUP BYalso differs by protocol. Measured on Redis 8.4.6 over 5 documents,SELECT price * 2 AS dblreportscount=1on RESP2, a placeholder the Redis docs call "not a valid value", andcount=5on RESP3. With aGROUP BYthe two agree. Neither value can be derived from the other without changingprotocol=2output, so both are reported as Redis sent them, and tests pin the difference so it is not later "fixed" by derivingcountfrom the row count.The dependency range stays at
redis>=5.0.0. RESP3 has been reachable throughprotocol=3since redis-py 5.0, so the fix is needed at the floor too. RedisVL excludes redis-py 8.0.0 because of redis/redis-py#4107, but that defect lives inResult.from_resp3, whichsql-redisnever calls.The evidence for the RESP3 reply layout comes from one server version. Redis 8.4.6 standalone was measured across redis-py 6.3.0, 7.4.0 and 8.1.0 and five client modes, sync and async, and rows matched the
protocol=2baseline in all 30 cells. No cluster deployment was exercised, and redis.io documents only the top-level keys of the RESP3 map, not the per-document layout the fold relies on.Next Steps
warningkey. On a query timeout RediSearch returns partial results with a warning, and the fold discards it, so a truncated result set looks complete. RESP2 has no equivalent channel, which is why it was left out of this change.execute()bodies, which are still near-identical above the_parse_replycall. Both error re-wrapping paths are now tested, so the refactor has a safety net.Release Notes
sql-redisnow works over RESP3 connections, including redis-py 8's default. On RESP3,score()columns are returned asfloatrather than text, andcountfor an aggregation withoutGROUP BYreports the real total where RESP2 reports1.