test: assert the client parses execution plans, and fix a plan parser bug - #267
Merged
Conversation
The async client cannot run any query against redis >= 8.1.0:
TypeError: Redis.__init__() got an unexpected keyword argument
'himport_registry'
`Is_Cluster` copies the async pool's `connection_kwargs` and splats them
into a synchronous `redis.Redis(**kwargs)` probe. redis-py keeps internal
state in that dict alongside real connection parameters, and 8.1.0 added
six such entries (`himport_registry`, `maint_notifications_pool_handler`,
`maint_notifications_config`, `orig_host_address`, `orig_socket_timeout`,
`orig_socket_connect_timeout`). None are accepted by `Redis.__init__`.
Since the probe runs during connection setup, this fails before any
command is sent — every async query, not just cluster deployments.
Filter the kwargs against the constructor signature instead. This also
covers #235 (the same crash with `path`), and any future internal key,
without needing to enumerate them. The sync client is unaffected because
it already picks fields out by name rather than forwarding everything.
Fixes #261.
Builds a real async pool, injects an unknown key, and drives `Is_Cluster` through both branches. Only `info()` is stubbed — the real `Redis.__init__` runs, because rejecting the extra kwargs is exactly what used to raise. Patching the whole class would hide the regression, since the filter reads the constructor signature. The key is injected explicitly rather than relying on the installed redis version, so the test still guards the behaviour on redis < 8.1.0. Verified: reverting the previous commit fails this test with the reported TypeError.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughThe change adds shared execution-plan and profile validators, preserves parent nodes for sibling operations, filters unsupported cluster-probe parameters, and updates synchronous and asynchronous explain, graph, profile, and cluster tests. ChangesExecution validation and cluster detection
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #267 +/- ##
==========================================
- Coverage 92.98% 92.84% -0.15%
==========================================
Files 38 39 +1
Lines 3152 3088 -64
==========================================
- Hits 2931 2867 -64
Misses 221 221 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/test_async_graph.py (1)
581-601: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winValidate nested
Operationchildren.
Operation.__eq__compares onlynameandargs. It does not comparechildren. The assertions therefore accept any structured subtree with the expected root operation. Use a recursive assertion helper that compares each operation name, args, child count, and child operations.
tests/test_async_graph.py#L581-L601: Validate bothUnionbranches recursively.tests/test_async_graph.py#L615-L618: Validate the Cartesian Product children recursively.tests/test_graph.py#L543-L563: Validate bothUnionbranches recursively.tests/test_graph.py#L577-L580: Validate the Cartesian Product children recursively.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_async_graph.py` around lines 581 - 601, Operation equality ignores children, so the tests can accept incorrect nested query plans. Add or reuse a recursive assertion helper that checks each Operation’s name, args, child count, and descendants, then apply it to both Union branches in tests/test_async_graph.py:581-601 and tests/test_graph.py:543-563, and to the Cartesian Product children in tests/test_async_graph.py:615-618 and tests/test_graph.py:577-580.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@tests/test_async_graph.py`:
- Around line 581-601: Operation equality ignores children, so the tests can
accept incorrect nested query plans. Add or reuse a recursive assertion helper
that checks each Operation’s name, args, child count, and descendants, then
apply it to both Union branches in tests/test_async_graph.py:581-601 and
tests/test_graph.py:543-563, and to the Cartesian Product children in
tests/test_async_graph.py:615-618 and tests/test_graph.py:577-580.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6c56f699-2935-46a5-b7ad-6ddd1af5ca14
📒 Files selected for processing (6)
tests/test_async_explain.pytests/test_async_graph.pytests/test_async_profile.pytests/test_explain.pytests/test_graph.pytests/test_profile.py
When three or more operations share a nesting level, the parser pushed each
new sibling onto the stack in place of their parent, so the next sibling was
attached to the previous one instead. A plan like
Project
Cartesian Product
All Node Scan | (a)
All Node Scan | (b)
All Node Scan | (c)
parsed with (c) nested under (b). Keep the parent on the stack instead.
…gine picked The tests asserted the exact operation tree GRAPH.EXPLAIN and GRAPH.PROFILE returned, so a planner change broke them even though the client was fine. Which operations a query compiles into is the engine's business; the client is responsible for parsing the reply. Assert that instead: every line of the reply became exactly one operation, nested as deep as that line was indented, with the name and arguments split off correctly and the profile statistics parsed. These hold on both engines.
Naseem77
force-pushed
the
naseem77-align-py-tests-with-engine
branch
from
August 10, 2026 13:32
db335e8 to
49aa012
Compare
Naseem77
changed the title
…cluster-unknown-kwargs
swilly22
requested changes
Aug 12, 2026
|
|
||
| plan = await g.profile("UNWIND range(0, 3) AS x RETURN x") | ||
|
|
||
| results_op = plan.structured_plan |
Contributor
There was a problem hiding this comment.
even the number of operations might change from one implementation to another.
i prefer we use queries that have the same plan in both our C and Rust versions, instead of narrowing it down to just minimum number of operations, as this test is too weak.
…args fix: only forward accepted kwargs from Is_Cluster to redis.Redis
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/plan_helpers.py`:
- Around line 79-82: Update the records_produced assertion in the relevant test
helper to compare plan.structured_plan.records_produced directly with
records_produced, instead of taking the maximum across parsed operations.
Preserve the existing conditional behavior when records_produced is None.
In `@tests/test_async_db.py`:
- Around line 354-388: Move the synchronous
test_is_cluster_filters_unknown_connection_kwargs from tests/test_async_db.py to
the corresponding synchronous test module, preserving its assertions and setup.
Keep tests/test_async_db.py limited to pytest-asyncio tests and maintain the
existing async/sync test mirroring convention.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 28cc287b-d70f-4f3d-96bf-e4a1f11c6dc2
📒 Files selected for processing (10)
falkordb/asyncio/cluster.pyfalkordb/execution_plan.pytests/plan_helpers.pytests/test_async_db.pytests/test_async_explain.pytests/test_async_graph.pytests/test_async_profile.pytests/test_explain.pytests/test_graph.pytests/test_profile.py
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/test_profile.py
- tests/test_async_profile.py
min_operations only checked that a plan had at least N operations, which passes on almost any tree. Replace it with assert_plan_shape, which pins the operation names and their nesting against an indented literal, and optionally their arguments. The C and Rust engines put a different driver operation at the root — C wraps a read plan in Results, Rust wraps a write plan in Commit — while the plan below it is identical, so no query matches byte for byte across both. assert_plan_shape skips that root and asserts everything under it exactly. Two queries stay on assert_parsed_plan because the engines genuinely compile them differently, each with a comment naming the divergence: MERGE (Include Pending vs MergeCreate) and UNION (Union vs Join). Add a three-way cartesian product test. Three operations sharing one nesting level is the case the plan parser used to get wrong, so it guards that fix directly.
swilly22
approved these changes
Aug 13, 2026
Naseem77
added a commit
to SantoshDhaladhuli/falkordb-py
that referenced
this pull request
Aug 13, 2026
Minor rather than patch: the release narrows the redis range, so it changes which environments can install the client. Since 1.6.2: - fix: Is_Cluster() no longer forwards pool-only kwargs to redis.Redis, which broke every async client on redis 8.1 (FalkorDB#262) - fix: the execution plan parser attaches every sibling operation to its parent instead of nesting siblings under each other (FalkorDB#267) - build: redis and python-dateutil bounded to tested versions
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
3 participants
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.
Our CI runs against
falkordb/falkordb:edge, which now resolves to the Rust engine, and the test suite failed 16 tests there before reaching any client code.The tests asserted the exact operation tree that
GRAPH.EXPLAIN/GRAPH.PROFILEreturned — root opResults, thenProject, thenUnwind, and so on. That makes a planner change break the client's tests even when the client is fine, which is what happened:Resultsis gone,JoinbecameUnion,MERGEis rooted atCommitwith a newInclude Pendingop, and a traverse renders as(t)<-(r:Rider).Which operations a query compiles into is the engine's business. What the client owes us is that it issues the command and parses the reply. So that's what these tests assert now, and they pass unchanged on both engines.
falkordb:edge(Rust)falkordb:v4.20.1(C)mainWhat the tests check now
tests/plan_helpers.pyasserts, for any plan from any engine:| argshalf were split apart correctlyrecords_producedmatches the row count the query impliesThe indentation comparison is the important one — it checks the parsed tree against the raw reply, so it fails if nesting is reconstructed wrongly.
It caught a real parser bug
Writing that assertion turned up a genuine client bug, reproducible on both engines. When three or more operations share a nesting level, the parser attached the third to the second:
MATCH (a), (b), (c) RETURN *is enough to hit it. The cause: each new sibling was pushed onto the stack in place of its parent, so the next sibling popped a sibling instead. Fixed in the first commit by keeping the parent on the stack.This is why the old assertions couldn't catch it — they only ever used two-way branches, and comparing a plan to a hardcoded tree never checks the reply itself.
Commits
UNWIND range(0, 200000)runs in ~6ms on the Rust engine, below the ~10ms slowlog threshold, so nothing was recorded and the assertion indexed an empty list. Bumped to 1M rows (~25ms).Verification
Full suite against live servers on both engines, plus
ruff format --check,ruff checkandmypy falkordb/. The parser fix was checked both ways: the new assertion fails against the old parser and passes against the fixed one.Relationship to #266
#266 adapts the explain/profile tests to the Rust engine's new plans. This takes the other route — not asserting the plan at all — so it also covers the slowlog tests and the
Join/Uniondrift intest_graph.pyandtest_async_graph.py, and it keeps passing on the C engine. Happy to close whichever we don't want.Note on the codecov/project check
The repo has no
codecov.yml, so it falls back totarget: auto, threshold: 0%and any dip fails.test_query_timeoutis markedxfail(strict=False)and its 1ms server-side timeout doesn't always fire; when it doesn't, the test stops early and a few lines go unexecuted. Running the same commit five times gives 4 xfailed / 1 xpassed. Unrelated to these changes — happy to fix it separately if it's worth pinning down.Summary by CodeRabbit
Bug Fixes
Tests