fix(falkordb): route single-group reads onto the group's graph#1572
Open
yimtheppariyapol wants to merge 1 commit into
Open
fix(falkordb): route single-group reads onto the group's graph#1572yimtheppariyapol wants to merge 1 commit into
yimtheppariyapol wants to merge 1 commit into
Conversation
Contributor
|
All contributors have signed the CLA ✍️ ✅ |
Author
|
I have read the CLA Document and I hereby sign the CLA behalf on myself, e-mail: [email protected] |
Author
|
I have read the CLA Document and I hereby sign the CLA behalf on myself, e-mail: [email protected] |
FalkorDB stores each group_id in its own graph, so a read must clone the driver onto that graph. `handle_multiple_group_ids` only did this when `len(group_ids) > 1`, so a single-group read fell through to the base driver (pinned at default_db) and returned nothing — even though the data was physically present. The write path mutates `self.driver` onto the group graph, which masked the bug in same-process write-then-read tests. Replace the `len > 1` gate with a blank-filtered `routable_group_ids` (mirrors search()'s `!= ['']` convention), so a single real group routes and a `['']`/`[None]` group no longer routes onto an empty graph name. Also memoize `FalkorDriver.clone()` per database. `__init__` schedules build_indices_and_constraints() on every construction; that is load-bearing exactly once per group but pure churn on every subsequent read now that single-group reads clone. Caching keeps the once-per-group index build while removing the per-call constructor cost. Follow-up (not in this PR): the write-path `self.driver` mutation in graphiti.py could be made non-mutating so reads and writes share one routing mechanism.
1998dcb to
a881246
Compare
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.
Summary
Single-group reads against FalkorDB return nothing even when the data is physically present in the group's graph.
handle_multiple_group_idsonly re-routed the driver onto the group graph whenlen(group_ids) > 1, so a single-group read fell through to the base driver (pinned atdefault_db) and queried the wrong graph.Root cause
FalkorDB stores each
group_idin its own graph, so a read must clone the driver onto that graph. The decorator gated that routing onlen(group_ids) > 1. The write path mutatesself.driveronto the group graph, which is why same-process write-then-read tests masked the bug — the reader inherited the mutated driver. A fresh reader process with a singlegroup_idquerieddefault_dband got 0 results.Reproduced:
GRAPH.QUERY <group>showed episodes + facts present; a single-groupget_episodes([group])returned 0; a two-group[group, other]search returned everything (becauselen > 1routed).What changed
decorators.py— replace thelen > 1gate with a blank-filteredroutable_group_ids(mirrorssearch()'s!= ['']convention). A single real group now routes; a['']/[None]group no longer routes onto an empty graph name.falkordb_driver.py— memoizeclone()per database.__init__schedulesbuild_indices_and_constraints()on every construction — load-bearing exactly once per group, but pure churn on every subsequent read now that single-group reads clone. Caching keeps the once-per-group index build while removing the per-call constructor cost.Verification
get_episodes([group])now returns the group's facts against a live FalkorDB (previously 0).clone(): repeated clone of the same db returns the same object (no churn);default_group_idmaps todefault_db; self-clone returns self; a new group is constructed once then cached, and the new group's graph still gets its indices built.Follow-up (not in this PR)
The write-path
self.drivermutation ingraphiti.pycould be made non-mutating so reads and writes share one routing mechanism, removing the asymmetry that masked this bug.