fix(queries): match :EDGE and filter on kind in typed traversals - #70
Open
r0h1tb wants to merge 1 commit into
Open
fix(queries): match :EDGE and filter on kind in typed traversals#70r0h1tb wants to merge 1 commit into
r0h1tb wants to merge 1 commit into
Conversation
batch_upsert_edges writes every edge as a single untyped :EDGE
relationship and keeps the semantic kind as a property:
MERGE (a)-[r:EDGE {id: e.id}]->(b)
SET r += e
Eight read sites instead matched relationship types that no writer
creates (:INHERITS, :EXTENDS, :IMPLEMENTS, :OVERRIDES, :TYPES,
:CAPTURES). Those queries are valid Cypher over an empty set: they
compile, they run, and they return nothing, for ever, with no error.
That silently emptied find_subclasses, find_superclasses, find_overrides
and the type-usage and inheritance counts in the node-detail and impact
paths.
This is option A from #61, as chosen on the issue: align the readers to
the writer. Each site now matches [r:EDGE] and filters with
`WHERE r.kind IN $...`, the shape #50 already moved the call path to.
Variable-length traversals use `all(rel IN rels WHERE rel.kind IN $...)`,
matching find_callers/find_callees.
Option B — emitting real typed relationships — keeps traversals natural
and lets Neo4j use relationship-type indexes for the *1..N inheritance
walks, but needs a dynamic write and a re-index. Documented in a comment
next to the new kind groups; worth doing deliberately rather than as a
bug fix.
Two details worth calling out:
- The paginated inheritance query returned `type(r) as rel_type`, which
would now always be "EDGE". It returns `r.kind` instead, so the
reported reference type still says INHERITS/EXTENDS/IMPLEMENTS.
- The :CAPTURES site is corrected for consistency but still returns
nothing, for a different reason: the parsing layer emits no CAPTURES
edges or Variable nodes yet. EdgeKind.CAPTURES is declared so the kind
is no longer a bare undeclared string.
The regression test is static, for the same reason test_query_parameters_bound
is: proving a traversal returns rows needs a populated graph with real
inheritance and override edges. It collects the relationship types the
writers create and the ones the readers match, and asserts the second set
is a subset of the first — so a future typed writer makes its matching
readers pass on their own.
Fixes #61
7 tasks
Owner
|
@good pr, may merge |
10 tasks
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.
Description
Eight read sites matched relationship types that no writer ever creates, so
they could only ever return empty — silently, with no error.
batch_upsert_edgeswrites every edge as a single untyped:EDGErelationship and keeps the semantic kind as a property:
The only other relationship types created anywhere are
:RELATESand:CONTAINS_BLOCK, both of which have matching readers and are fine.This is option A, as you chose on the issue — align the readers to the
writer. Each site now matches
[r:EDGE]and filters withWHERE r.kind IN $...,the shape #50 already moved the call path to. Variable-length traversals use
all(rel IN rels WHERE rel.kind IN $...), matchingfind_callers/find_callees.Related Issue
Fixes #61
Type of Change
What was broken
api/ast_rag_api.py:470[:INHERITS|EXTENDS|IMPLEMENTS*1..N][rels:EDGE*1..N]+all(rel IN rels …)api/ast_rag_api.py:486api/ast_rag_api.py:520[:OVERRIDES*1..N][rels:EDGE*1..N]+all(rel IN rels …)api/ast_rag_api.py:916[r:TYPES][r:EDGE]+r.kind IN $type_kindsapi/ast_rag_api.py:921[r:INHERITS|EXTENDS|IMPLEMENTS][r:EDGE]+r.kind IN $inheritance_kindsapi/ast_rag_api.py:995[r:TYPES][r:EDGE]+r.kind IN $type_kindsapi/ast_rag_api.py:1019[r:INHERITS|EXTENDS|IMPLEMENTS][r:EDGE]+r.kind IN $inheritance_kindsservices/search_service.py:499[:CAPTURES][r:EDGE]+r.kind IN $capture_kindsUser-visible effect:
find_subclasses,find_superclasses,find_overridesand the type-usage / inheritance counts in the node-detail and impact paths
returned nothing regardless of the graph.
Two details worth flagging yourself
1.
type(r)would have silently changed meaning. The paginated inheritancequery returned
type(r) as rel_type, which after this change is always"EDGE".It returns
r.kindinstead, so the reported reference type still saysINHERITS/EXTENDS/IMPLEMENTSrather than degrading toEDGE. A naiverewrite of the eight sites would have broken that field.
2.
:CAPTURESis corrected but still returns nothing — for a differentreason, which I want to be explicit about rather than imply it now works. The
parsing layer emits no
CAPTURESedges and noVariablenodes at all, sofind_lambdas(with_captured_vars=True)reportscaptured_vars: []either way.The query shape is fixed so it starts working the moment the extractor lands,
and
EdgeKind.CAPTURESis declared so the kind is no longer a bare undeclaredstring. Happy to drop that hunk if you would rather keep this PR strictly to
the sites where edges actually exist.
Option B
Left undone deliberately, and documented in a comment next to the new kind
groups. Emitting real typed relationships keeps traversals natural and lets
Neo4j use relationship-type indexes for the
*1..Ninheritance walks, but itneeds a dynamic write (APOC or a per-kind
MERGEbranch) and is a breakinggraph-schema change requiring a re-index. Worth doing deliberately rather than
as a bug fix.
Checklist
pytest tests/ -v)ast-rag evaluate --all— not run, see Testing belowTesting
The regression test is static, for the same reason
test_query_parameters_boundis: proving a traversal returns rows needs a populated graph with real
inheritance and override edges. It collects the relationship types the writers
create and the ones the readers match, and asserts the second is a subset of the
first. It is derived from the source rather than hard-coded, so if a writer is
later changed to emit typed relationships (option B), the matching readers stop
failing it on their own. A second test guards the guard — if the scan finds no
:EDGEwriter, the subset check proves nothing and it says so.On this branch with the source changes reverted and the test kept:
Full suite, Python 3.12 on macOS:
main@ 41e48af)Exactly +2, the two new tests. No pre-existing failures on either side.
test_query_parameters_boundstill passes, which matters here — it is whatwould catch a rewritten query whose new
$inheritance_kinds/$type_kinds/$capture_kindsparameter was not actually bound at somesession.runsite.ast-rag evaluate --allwas not run. It needs a live Neo4j + Qdrant, andthe committed
ast_rag_config.jsonpoints at a private LAN (#62), so I have noway to run it from here. Nothing in this change touches indexing, embeddings or
scoring — it only rewrites read-side Cypher — but flagging it rather than
ticking the box.
Additional Notes
Ping me if you would prefer this split per query family, or if you want the
:CAPTUREShunk dropped.