fix: Phase 3 WITH scoping, triadic OPTIONAL MATCH, multi-CREATE, rel-type disjunction (#362, #363)#364
Merged
Merged
Conversation
…T extensions (#362) - WITH WHERE now evaluates against pre-projection context (merged with new aliases), fixing KeyError when WHERE references variables not projected (e.g. UNWIND i,j then WITH types[i] AS lhs, types[j] AS rhs WHERE i<>j) - MATCH/ExpandEdges skip rows where bound variable is CypherNull (forwarded from OPTIONAL MATCH), producing correct empty results instead of crashing - CypherValue.equals/less_than guard against non-CypherValue others (NodeRef/EdgeRef), returning null for incompatible cross-type comparisons - Chained property access: grammar allows a.b.c (property_access as base), column-name generation handles nested PropertyAccess - SET n = {map}: replace all properties from map expression - SET n += {map}: augment/merge properties - SET n:Label: add labels to node (updates label index via add_node) - SET (n).prop = expr: parenthesized node target - Net TCK gain: +36 (3456 → 3492 passing, 392 → 356 failing) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…d signatures (#362) - Grammar: allow create_clause+ in write_query so multi-CREATE scripts share variable scope (binary-tree fixture: nodes + rels in same query) - Executor: when dst_var is already bound, OptionalExpandEdges filters to only edges reaching that bound node (correct triadic selection semantics) - Types: relax equals/less_than signatures to Any to allow cross-type guards without mypy 'unreachable' false positives Fixes 17/19 triadic TCK tests; 2 remaining need [:TYPE1|TYPE2] grammar support. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Support [:TYPE1|TYPE2] form in addition to [:TYPE1|:TYPE2]. Simplify rel_types grammar rule — inlines rel_type token into parent and handles optional colon on subsequent alternatives. Closes #363. Fixes remaining 2/19 triadic TCK scenarios. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
WalkthroughLoosens SET item shapes to accept 2- or 3-tuples, extends SET semantics (map replace/merge and label additions), fixes WITH WHERE evaluation order and optional-expand semantics, improves nested PropertyAccess stringification, relaxes CypherValue comparison signatures, and refactors in-memory statistics handling and create planning. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as rgba(40,116,166,0.5)
participant Parser as rgba(90,155,68,0.5)
participant Transformer as rgba(204,120,50,0.5)
participant Planner as rgba(120,60,180,0.5)
participant Executor as rgba(200,40,80,0.5)
participant Storage as rgba(30,144,255,0.5)
Client->>Parser: submit Cypher text
Parser->>Transformer: produce AST (SET items normalized, rel_types)
Transformer->>Planner: emit operators (aggregated CREATE, Set items 2/3-tuple)
Planner->>Executor: execution plan
Executor->>Executor: evaluate WITH (merged context for WHERE)
Executor->>Storage: perform traversal, apply SET (replace/merge/labels)
Storage-->>Executor: rows/edges (CypherNull forwarding)
Executor-->>Client: result set
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/graphforge/executor/executor.py`:
- Around line 2986-3000: The SET :+ branch creates a new NodeRef and only
updates ctx.bindings[var_name], leaving other bindings that alias the same node
id stale; locate the block handling op == ":+", NodeRef, self.graph.add_node and
ctx.bindings, compute the updated NodeRef as you already do, then iterate over
ctx.bindings items and replace any binding whose value is a NodeRef with the
same id with the updated NodeRef (also update ctx.bindings[var_name] as before)
so all aliases reference the new object; ensure you do this update only for
NodeRef instances and preserve other binding types.
- Around line 784-786: The aggregation fast-path in _execute_expand() still
dereferences src_node.id via the agg_hint path and can dereference a CypherNull;
modify the agg_hint branch so it checks for isinstance(src_node, CypherNull) (or
equivalent null check) before accessing src_node.id and handle that case by
forwarding a NULL binding into _execute_expand_with_aggregation() (i.e., treat
the group/key as NULL per three-valued logic rather than accessing attributes),
ensuring _execute_expand_with_aggregation() receives and handles CypherNull
inputs safely.
- Around line 3003-3031: The code assumes the bound value for var_name has a
.properties attribute and directly calls element.properties.clear()/assign,
which raises AttributeError for scalars/lists; before mutating properties (the
branch handling op == "=" and op == "+="), validate the bound element from
ctx.bindings[var_name] actually exposes properties (e.g. hasattr(element,
"properties") and isinstance(element.properties, dict) or if your model has
Node/Relationship classes use isinstance(element, Node|Relationship)); if the
check fails, raise the appropriate Cypher type error (match the project’s
existing error type, e.g. TypeError or CypherTypeError) with a clear message
referencing SET and var_name so callers get a proper Cypher type error instead
of a raw AttributeError. Ensure this validation is placed before any call to
element.properties.clear() or element.properties[...] and keep references to
var_name, ctx.bindings, evaluate_expression, CypherMap and CypherNull unchanged.
In `@src/graphforge/parser/cypher.lark`:
- Around line 113-117: The grammar currently allows arbitrary recursive
property_access in set_item which permits LHS like n.a.b or foo()[0].x that the
executor cannot handle; restrict set_item in cypher.lark to only the forms the
executor supports (variable "=" expression, variable "+=" expression, variable
label+, and variable "." IDENTIFIER = expression — including the parenthesized
variant "(" variable ")" "." IDENTIFIER "=" expression) so that rules map to
set_node_map, set_node_map_augment, set_labels, set_property_paren and
set_property only for direct variable.property targets; then verify/update
parser.py's transformer/_execute_set_items to only expect those rule names
(set_property, set_property_paren, set_node_map, set_node_map_augment,
set_labels) and reject or raise a clear error for any other LHS shapes.
- Around line 55-58: The write_query rule currently only allows create_clause+
when part of match/merge sequences, making standalone CREATE ... SET ...
unreachable; update the write_query alternatives to include a branch that
permits create_clause followed by an optional set_clause (and optional
return/order/skip/limit as appropriate) so forms like "CREATE (n) SET n:Person"
and "CREATE (n) SET n += {...}" parse — specifically add or change productions
in the write_query rule to include "create_clause set_clause? return_clause?
order_by_clause? skip_clause? limit_clause?" (and/or allow "create_clause+
set_clause? ..." if you want to keep multi-CREATE support) while keeping
existing match/merge variants intact so create_clause and set_clause are
reachable for create-only queries.
In `@src/graphforge/parser/parser.py`:
- Around line 302-310: The parenthesized SET transformer set_property_paren
currently uses str(items[1]) which preserves backticks; change it to normalize
the token via the parser's _get_token_value helper (same approach used by other
property/label transformers) so the property name becomes the unquoted value;
specifically, in set_property_paren use _get_token_value(items[1]) to compute
prop_name before creating the PropertyAccess (variable from items[0].name,
property=prop_name) and return (prop_access, expr).
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cd089e4a-c83a-4d90-880b-25dbeb276e05
📒 Files selected for processing (6)
src/graphforge/ast/clause.pysrc/graphforge/executor/executor.pysrc/graphforge/parser/cypher.larksrc/graphforge/parser/parser.pysrc/graphforge/planner/operators.pysrc/graphforge/types/values.py
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #364 +/- ##
==========================================
- Coverage 89.02% 88.83% -0.20%
==========================================
Files 39 39
Lines 11814 11898 +84
Branches 2576 2611 +35
==========================================
+ Hits 10518 10569 +51
- Misses 889 904 +15
- Partials 407 425 +18
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Sentry.
|
… extensions (#364) Cover new code paths introduced in the Phase 3 PR: - OptionalExpandEdges with pre-bound dst_var (triadic selection) - Multi-CREATE variable scoping across consecutive CREATE clauses - Relationship type disjunction [:TYPE1|TYPE2] parser forms - SET n = {map} (replace), SET n += {map} (augment), SET n:Label Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- _execute_expand_with_aggregation: add CypherNull guard (same as
non-agg path) to prevent crash on NULL source nodes
- SET n:Label: update all sibling bindings pointing at same node id
so aliases like WITH n AS a, n AS b see consistent labels
- SET n = / SET n +=: skip silently when element is CypherNull
(openCypher: SET null += {map} is a no-op), raise TypeError for
non-node/rel scalars
- Grammar: restrict set_item property_access to variable.IDENTIFIER
only (prevents SET n.a.b = 1 and other unsupported nested forms)
- Grammar: add order_by/skip/limit to create_clause+ write_query rule
so CREATE (n) RETURN n LIMIT 0 parses correctly
- Parser: use _get_token_value() in set_property and set_property_paren
for consistent backtick-stripping
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Replace single TCK job with a 4-shard matrix using pytest-shard. Each shard runs on its own ubuntu-latest runner with -n auto, giving ~8 effective cores total. Wall time drops from ~90 min to ~25 min at no extra billing cost. A tck-report job runs after all shards complete, merges the JUnit XML files, and runs tck_metrics.py on the combined results. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
tests/unit/executor/test_set_extensions.py (1)
1-144: Move these cases out of the unit suite.They exercise the full
GraphForge.execute()pipeline, so they belong intests/integration/or need to be rewritten as isolated executor unit tests. As written, this file mixes parser/planner/executor coverage in atests/unit/location.As per coding guidelines, "Unit tests must test a single component in isolation using fixtures like empty_graph and assertions on AST/operator types".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/executor/test_set_extensions.py` around lines 1 - 144, These tests (classes TestSetNodeMapReplace, TestSetNodeMapAugment, TestSetLabels) exercise the full GraphForge.execute() pipeline and should not live in unit tests; either move the entire file to tests/integration/ or refactor each test to isolate the executor by using the empty_graph fixture and asserting on parsed AST/operator nodes instead of calling GraphForge.execute(); locate uses of GraphForge.execute in these tests and replace with executor-level fixtures/calls or relocate the file to integration tests so parser/planner coverage resides outside unit suite.tests/unit/executor/test_optional_expand_bound_dst.py (1)
1-188: This module is integration-style, not unit-style.It exercises multiple layers through
GraphForge.execute()and mixes executor, CREATE scoping, and parser/grammar coverage in atests/unit/path. Please move it to integration/tck coverage or split it into focused unit tests.As per coding guidelines, "Unit tests must test a single component in isolation using fixtures like empty_graph and assertions on AST/operator types".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/executor/test_optional_expand_bound_dst.py` around lines 1 - 188, These tests are integration-style (they exercise multiple layers via GraphForge.execute) but live under tests/unit; either move this module (classes TestOptionalExpandBoundDst, TestMultiCreateVariableScoping, TestRelationshipTypeDisjunction and their test_* methods such as test_optional_match_bound_dst_matching_edge) to the integration/TCK test suite, or refactor each test to be a true unit test by removing GraphForge.execute usage and instead using unit fixtures (e.g. empty_graph) and direct assertions on the parser/AST or executor operators (asserting types/results of the AST/operator methods) so they target a single component in isolation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/graphforge/executor/executor.py`:
- Around line 597-603: The executor currently treats any non-CypherNull binding
as a node and dereferences .labels/.id, causing crashes for scalars/lists/maps;
update the branches that inspect bound_node (the checks that call
_node_matches_labels or access .id/.labels) to first verify the binding is an
actual node type (e.g., isinstance(bound_node, NodeRef)) and, if not, treat it
as no match (continue) just like CypherNull; apply this guard in the three spots
referenced (the block using _node_matches_labels with op.labels, the block that
checks node id, and the other label/id check locations noted) so only NodeRef
instances are dereferenced.
- Around line 3172-3217: The OPTIONAL-MATCH fast path must respect an
already-bound relationship variable: if op.edge_var is present in ctx.bindings,
fetch the existing binding (ctx.get(op.edge_var)) and (a) if it's an EdgeRef,
filter matching_edges further to only that exact edge (compare by id or object
identity) so you never return a different edge, and (b) if it's CypherNull or a
non-edge, treat as no-match and preserve the NULL binding; when building new_ctx
do not overwrite an existing op.edge_var binding—copy ctx.bindings and only bind
op.edge_var when it was not already bound. Apply these changes in the dst_bound
branch where matching_edges and new_ctx are created.
In `@tests/unit/executor/test_optional_expand_bound_dst.py`:
- Around line 121-142: The test test_three_create_clauses_chain currently only
asserts len(result) == 1 which doesn't prove the first CREATE's identifier `a`
was reused; update the assertions after running MATCH to check the returned
row(s) contain the expected node identities/names to prove shared scope: for
example, assert the single returned row's x.name == 'a' and y.name == 'b' (and
keep the single-row assertion). Locate this in the
test_three_create_clauses_chain function where GraphForge() is used and
gf.execute(...) is called to validate the exact returned values rather than only
the row count.
---
Nitpick comments:
In `@tests/unit/executor/test_optional_expand_bound_dst.py`:
- Around line 1-188: These tests are integration-style (they exercise multiple
layers via GraphForge.execute) but live under tests/unit; either move this
module (classes TestOptionalExpandBoundDst, TestMultiCreateVariableScoping,
TestRelationshipTypeDisjunction and their test_* methods such as
test_optional_match_bound_dst_matching_edge) to the integration/TCK test suite,
or refactor each test to be a true unit test by removing GraphForge.execute
usage and instead using unit fixtures (e.g. empty_graph) and direct assertions
on the parser/AST or executor operators (asserting types/results of the
AST/operator methods) so they target a single component in isolation.
In `@tests/unit/executor/test_set_extensions.py`:
- Around line 1-144: These tests (classes TestSetNodeMapReplace,
TestSetNodeMapAugment, TestSetLabels) exercise the full GraphForge.execute()
pipeline and should not live in unit tests; either move the entire file to
tests/integration/ or refactor each test to isolate the executor by using the
empty_graph fixture and asserting on parsed AST/operator nodes instead of
calling GraphForge.execute(); locate uses of GraphForge.execute in these tests
and replace with executor-level fixtures/calls or relocate the file to
integration tests so parser/planner coverage resides outside unit suite.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: dc6caf73-ef49-47ff-aa12-bed67bf4fab0
⛔ Files ignored due to path filters (2)
.github/workflows/test.ymlis excluded by!**/.github/**uv.lockis excluded by!**/*.lock,!**/*.lock
📒 Files selected for processing (6)
pyproject.tomlsrc/graphforge/executor/executor.pysrc/graphforge/parser/cypher.larksrc/graphforge/parser/parser.pytests/unit/executor/test_optional_expand_bound_dst.pytests/unit/executor/test_set_extensions.py
✅ Files skipped from review due to trivial changes (2)
- pyproject.toml
- src/graphforge/parser/cypher.lark
🚧 Files skipped from review as they are similar to previous changes (1)
- src/graphforge/parser/parser.py
Comment on lines
+597
to
603
| # Null node from OPTIONAL MATCH forwarding → no match (produce no rows) | ||
| if isinstance(bound_node, CypherNull): | ||
| continue | ||
|
|
||
| # Check if bound node has required labels | ||
| if op.labels: | ||
| if self._node_matches_labels(bound_node, op.labels): |
Contributor
There was a problem hiding this comment.
Guard traversal inputs with NodeRef, not just CypherNull.
These branches still dereference .labels / .id on any non-null binding, so a scalar/list/map binding will still crash instead of behaving as “no match.” Please validate the bound value is a node before traversing, in all three paths.
As per coding guidelines, "Always verify that variables are bound before use in executor operations".
Also applies to: 784-790, 908-915
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/graphforge/executor/executor.py` around lines 597 - 603, The executor
currently treats any non-CypherNull binding as a node and dereferences
.labels/.id, causing crashes for scalars/lists/maps; update the branches that
inspect bound_node (the checks that call _node_matches_labels or access
.id/.labels) to first verify the binding is an actual node type (e.g.,
isinstance(bound_node, NodeRef)) and, if not, treat it as no match (continue)
just like CypherNull; apply this guard in the three spots referenced (the block
using _node_matches_labels with op.labels, the block that checks node id, and
the other label/id check locations noted) so only NodeRef instances are
dereferenced.
Comment on lines
+3172
to
+3217
| # If dst_var is already bound (from preceding MATCH), filter edges to | ||
| # only those reaching that specific destination node — LEFT JOIN semantics. | ||
| dst_bound = op.dst_var in ctx.bindings | ||
| if dst_bound: | ||
| from graphforge.types.graph import NodeRef as _NodeRef | ||
|
|
||
| bound_dst = ctx.get(op.dst_var) | ||
| if isinstance(bound_dst, _NodeRef): | ||
| bound_dst_id = bound_dst.id | ||
|
|
||
| from graphforge.types.graph import EdgeRef as _EdgeRef | ||
|
|
||
| def _edge_dst_id(e: _EdgeRef, direction: str, src_id: object) -> object: | ||
| if direction == "OUT": | ||
| return e.dst.id | ||
| elif direction == "IN": | ||
| return e.src.id | ||
| return e.dst.id if e.src.id == src_id else e.src.id | ||
|
|
||
| matching_edges = [ | ||
| e | ||
| for e in edges | ||
| if _edge_dst_id(e, op.direction, src_node.id) == bound_dst_id | ||
| ] | ||
| if matching_edges: | ||
| for edge in matching_edges: | ||
| new_ctx = ExecutionContext() | ||
| new_ctx.bindings = dict(ctx.bindings) | ||
| if op.edge_var: | ||
| new_ctx.bind(op.edge_var, edge) | ||
| result.append(new_ctx) | ||
| else: | ||
| # No matching edge — OPTIONAL: preserve row with NULL edge | ||
| new_ctx = ExecutionContext() | ||
| new_ctx.bindings = dict(ctx.bindings) | ||
| if op.edge_var: | ||
| new_ctx.bind(op.edge_var, CypherNull()) | ||
| result.append(new_ctx) | ||
| else: | ||
| # Bound dst is not a node (e.g. NULL) — treat as no match | ||
| new_ctx = ExecutionContext() | ||
| new_ctx.bindings = dict(ctx.bindings) | ||
| if op.edge_var: | ||
| new_ctx.bind(op.edge_var, CypherNull()) | ||
| result.append(new_ctx) | ||
| continue |
Contributor
There was a problem hiding this comment.
Preserve pre-bound relationship constraints in the OPTIONAL MATCH fast path.
This branch filters by the bound destination, but it never re-applies an existing op.edge_var binding. If the relationship variable is already in scope, this can return a different edge than the one already bound.
Suggested fix
matching_edges = [
e
for e in edges
if _edge_dst_id(e, op.direction, src_node.id) == bound_dst_id
]
+ if op.edge_var and op.edge_var in ctx.bindings:
+ bound_edge = ctx.get(op.edge_var)
+ matching_edges = [
+ e
+ for e in matching_edges
+ if hasattr(bound_edge, "id") and e.id == bound_edge.id
+ ]
if matching_edges:
for edge in matching_edges:
new_ctx = ExecutionContext()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/graphforge/executor/executor.py` around lines 3172 - 3217, The
OPTIONAL-MATCH fast path must respect an already-bound relationship variable: if
op.edge_var is present in ctx.bindings, fetch the existing binding
(ctx.get(op.edge_var)) and (a) if it's an EdgeRef, filter matching_edges further
to only that exact edge (compare by id or object identity) so you never return a
different edge, and (b) if it's CypherNull or a non-edge, treat as no-match and
preserve the NULL binding; when building new_ctx do not overwrite an existing
op.edge_var binding—copy ctx.bindings and only bind op.edge_var when it was not
already bound. Apply these changes in the dst_bound branch where matching_edges
and new_ctx are created.
Comment on lines
+121
to
+142
| def test_three_create_clauses_chain(self): | ||
| """Three CREATE clauses all share scope.""" | ||
| gf = GraphForge() | ||
| gf.execute(""" | ||
| CREATE (a:A {name: 'a'}) | ||
| CREATE (b:B {name: 'b'}) | ||
| CREATE (a)-[:LINK]->(b) | ||
| """) | ||
|
|
||
| result = gf.execute("MATCH (x:A)-[:LINK]->(y:B) RETURN x.name, y.name") | ||
| assert len(result) == 1 | ||
|
|
||
| def test_multi_create_with_trailing_semicolon(self): | ||
| """Multi-CREATE script ending with semicolon parses correctly.""" | ||
| gf = GraphForge() | ||
| gf.execute(""" | ||
| CREATE (a:Node {val: 1}), (b:Node {val: 2}) | ||
| CREATE (a)-[:EDGE]->(b); | ||
| """) | ||
|
|
||
| result = gf.execute("MATCH ()-[:EDGE]->() RETURN count(*) AS c") | ||
| assert result[0]["c"].value == 1 |
Contributor
There was a problem hiding this comment.
Strengthen the three-CREATE scope assertion.
assert len(result) == 1 only proves that one LINK edge exists. It would still pass if the third CREATE accidentally rebound a to a fresh node, so this test does not fully validate shared scope across all three clauses.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/unit/executor/test_optional_expand_bound_dst.py` around lines 121 -
142, The test test_three_create_clauses_chain currently only asserts len(result)
== 1 which doesn't prove the first CREATE's identifier `a` was reused; update
the assertions after running MATCH to check the returned row(s) contain the
expected node identities/names to prove shared scope: for example, assert the
single returned row's x.name == 'a' and y.name == 'b' (and keep the single-row
assertion). Locate this in the test_three_create_clauses_chain function where
GraphForge() is used and gf.execute(...) is called to validate the exact
returned values rather than only the row count.
- Replace pytest-shard (0-indexed, no balancing) with pytest-split (1-indexed groups 1-4, duration_based_chunks algorithm) - TCK: 4 shards collect exactly 25% each (967 tests/shard) - TCK: store/restore .test_durations via Actions cache so balance improves automatically after the first run - Coverage: new coverage-shard job runs tests/unit + tests/integration across 4 parallel runners with --cov, then coverage job merges XML reports and combines .coverage files for the threshold check Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- test.yml: top-level permissions: contents: read (applies to all jobs) tck-report job gets actions: write for cache/save step - changelog-check.yml: contents: read + pull-requests: write (needs to post PR comments) Follows GitHub security best practice of least-privilege GITHUB_TOKEN. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Adds 3.14 on all three OS with continue-on-error: true — visible in CI but non-blocking until the ecosystem stabilises. Also declares 3.14 support in pyproject.toml classifiers. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Multiple CREATE clauses in one query (e.g. the 971-clause movie graph fixture) were emitting N separate Create operators, each copying the full execution context. Merging them into one operator reduces this to a single pass over all patterns. Cuts the movie-graph TCK test from ~10 minutes to seconds. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- memory.py: replace per-insert Pydantic model_copy() with mutable counters; add _unique_sources_by_type for O(1) edge degree tracking instead of O(n) full set scan; get_statistics() now builds GraphStatistics lazily on demand - parser.py: _split_create_sequence() pre-splits long CREATE-only queries into batches of 5 before Earley parsing — reduces TCK movie graph (971 CREATEs) from 27 minutes to ~26 seconds; school graph from ~45 min to ~72 seconds - Add scripts/tck_perf_report.py to track timing regressions in CI - Add 'Report TCK performance metrics' step to tck-report CI job - docs: parser-performance-analysis.md documents root cause, benchmarks, and LALR(1) migration plan (issue #365) - tests: update test_snapshot_includes_statistics for new counter-based API Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/graphforge/storage/memory.py (1)
218-245:⚠️ Potential issue | 🟠 MajorRecompute per-type degree stats when replacing an edge.
The replacement path decrements
old_edge.type, but it never removes the old source from_unique_sources_by_typeor recomputesavg_degree_by_typefor the old type unless the count drops to zero. Replacing an edge with a different type or source can therefore leave a phantom denominator behind and skew later optimizer estimates. A regression test that replaces an edge across bothtypeandsrc.idwould catch this.Possible fix
+ def _recompute_edge_type_stats(self, edge_type: str) -> None: + edge_ids = self._type_index.get(edge_type, set()) + if not edge_ids: + self._unique_sources_by_type.pop(edge_type, None) + self._avg_degree_by_type.pop(edge_type, None) + return + sources = {self._edges[edge_id].src.id for edge_id in edge_ids} + self._unique_sources_by_type[edge_type] = sources + self._avg_degree_by_type[edge_type] = self._edge_counts_by_type[edge_type] / len(sources) ... if not is_new_edge: old_edge = self._edges[edge.id] self._outgoing[old_edge.src.id].remove(old_edge) self._incoming[old_edge.dst.id].remove(old_edge) self._type_index[old_edge.type].discard(edge.id) count = self._edge_counts_by_type.get(old_edge.type, 0) if count > 1: self._edge_counts_by_type[old_edge.type] = count - 1 elif old_edge.type in self._edge_counts_by_type: del self._edge_counts_by_type[old_edge.type] - self._avg_degree_by_type.pop(old_edge.type, None) + self._recompute_edge_type_stats(old_edge.type) ... else: # Replacement: add new edge stats (total_edges unchanged) self._unique_sources_by_type[edge.type].add(edge.src.id) self._edge_counts_by_type[edge.type] = self._edge_counts_by_type.get(edge.type, 0) + 1 - unique_sources = len(self._unique_sources_by_type[edge.type]) - self._avg_degree_by_type[edge.type] = self._edge_counts_by_type[edge.type] / max( - unique_sources, 1 - ) + self._recompute_edge_type_stats(edge.type)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/graphforge/storage/memory.py` around lines 218 - 245, When replacing an existing edge (variables old_edge and edge) you currently decrement _edge_counts_by_type for old_edge.type but fail to remove old_edge.src.id from _unique_sources_by_type[old_edge.type] when that source no longer has any remaining edges of that type and you do not recompute _avg_degree_by_type for the old type; update the replacement path to: check whether old_edge.src.id still has any outgoing/incoming edges of old_edge.type (inspect _outgoing/_incoming entries or type index for that src), if not remove it from _unique_sources_by_type[old_edge.type], then recompute _avg_degree_by_type[old_edge.type] = _edge_counts_by_type.get(old_edge.type,0) / max(len(_unique_sources_by_type.get(old_edge.type, set())),1); also handle the case where edge.type differs from old_edge.type so both old and new type counters/avg are updated consistently.
🧹 Nitpick comments (2)
src/graphforge/storage/memory.py (1)
166-172: Preservelast_updatedacross reads.
GraphStatistics()gets a fresh default timestamp every timeget_statistics()is called, solast_updatedno longer means “last mutation”. If anything persists or compares that field, reads now look like writes. Consider storing a dedicated_stats_last_updatedvalue and only bumping it from the mutation paths.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/graphforge/storage/memory.py` around lines 166 - 172, get_statistics currently constructs a new GraphStatistics with a fresh timestamp on every read; introduce a stored _stats_last_updated attribute on the Memory backend and use that when creating GraphStatistics in get_statistics instead of generating a new timestamp, and ensure all mutation methods (e.g., add_node, remove_node, add_edge, remove_edge or any method that changes _total_nodes/_total_edges/_node_counts_by_label/_edge_counts_by_type/_avg_degree_by_type) update _stats_last_updated when they change state so last_updated truly reflects the last mutation.src/graphforge/api.py (1)
784-793: Centralize statistics copy/hydration behindGraph.These two blocks now duplicate the private counter layout of
Graph, and they are already slightly asymmetric (clone()copies_unique_sources_by_type, backend load relies onadd_edge()side effects). A smallGraphhelper for copying/restoring statistics state would keep these paths in sync and make the next stats-field change less brittle.Also applies to: 846-854
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/graphforge/api.py` around lines 784 - 793, The current code duplicates Graph's private stats layout when copying counters (seen in clone() restoring cloned.graph._total_nodes/_total_edges/_node_counts_by_label/_edge_counts_by_type/_avg_degree_by_type/_unique_sources_by_type) and this duplicates logic elsewhere (also at the other block), so add a Graph-level helper (e.g., Graph.copy_stats_from(source: Graph) or Graph._hydrate_stats(state_dict)) that encapsulates copying/restoring all private counters and collections, then replace the direct assignments in clone() and the backend-load path with a single call to that helper; ensure the helper performs deep copies for mutable structures (dicts, defaultdicts/sets) to preserve original isolation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/tck_perf_report.py`:
- Around line 53-54: The status logic for test cases currently only checks
tc.find("failure") and thus treats <error> and <skipped> as PASS; update the
dict construction that sets "status" (the expression using tc.find(...)) to: if
tc.find("failure") or tc.find("error") then "FAIL", elif tc.find("skipped") then
"SKIPPED", else "PASS"; apply the same fix to the other identical occurrence
around the second block (the other dict at lines ~73-74) so both places use the
combined checks on tc for "failure", "error", and "skipped".
- Around line 100-113: The report currently formats the threshold with
"{threshold:.0f}" which rounds fractional seconds away; change both occurrences
in the strings that reference threshold (the header "Tests exceeding
{threshold:.0f}s threshold:" and the footer "No tests exceeded the
{threshold:.0f}s threshold.") to use a one-decimal format like
"{threshold:.1f}s" so fractional thresholds (e.g., 0.5s) are displayed
correctly; update the f-strings that include the variable threshold accordingly.
- Around line 30-35: The function find_testsuite currently returns only a single
ET.Element for a testsuite which drops additional suites when the XML root is
<testsuites>; update find_testsuite to return all suites: if root.tag is
"testsuite" return a list containing root, if root.tag is "testsuites" return
root.findall("testsuite") (or an iterable/list of those elements), and update
callers that expect a single ET.Element (e.g., functions using the returned
value) to iterate over the returned list/iterable so every <testsuite> is
processed.
In `@src/graphforge/parser/parser.py`:
- Around line 303-307: In set_labels(), replace the current string conversion
(labels = [str(label) for label in items[1:]]) with the parser-normalization
used elsewhere so backtick-quoted labels are stripped; use
self._get_token_value(...) for each label (same approach as in set_property,
set_property_paren, and label_conjunction) so labels =
[self._get_token_value(label) for label in items[1:]] and return the normalized
labels tuple as before.
- Around line 1350-1367: The backtick identifier branch in _KEYWORD_RE uses
`(?:[^`])*` which fails to accept escaped backticks (two consecutive backticks
inside a backtick-quoted identifier); update the backtick alternative in
_KEYWORD_RE to allow either a non-backtick character or an escaped backtick
sequence (two backticks) repeatedly so identifiers like `foo``bar` are matched
as a single token; locate _KEYWORD_RE in parser.py and replace the backtick
alternative with a pattern that permits (non-backtick | ``) repeated before the
closing backtick, preserving the surrounding anchors and re.VERBOSE usage.
---
Outside diff comments:
In `@src/graphforge/storage/memory.py`:
- Around line 218-245: When replacing an existing edge (variables old_edge and
edge) you currently decrement _edge_counts_by_type for old_edge.type but fail to
remove old_edge.src.id from _unique_sources_by_type[old_edge.type] when that
source no longer has any remaining edges of that type and you do not recompute
_avg_degree_by_type for the old type; update the replacement path to: check
whether old_edge.src.id still has any outgoing/incoming edges of old_edge.type
(inspect _outgoing/_incoming entries or type index for that src), if not remove
it from _unique_sources_by_type[old_edge.type], then recompute
_avg_degree_by_type[old_edge.type] = _edge_counts_by_type.get(old_edge.type,0) /
max(len(_unique_sources_by_type.get(old_edge.type, set())),1); also handle the
case where edge.type differs from old_edge.type so both old and new type
counters/avg are updated consistently.
---
Nitpick comments:
In `@src/graphforge/api.py`:
- Around line 784-793: The current code duplicates Graph's private stats layout
when copying counters (seen in clone() restoring
cloned.graph._total_nodes/_total_edges/_node_counts_by_label/_edge_counts_by_type/_avg_degree_by_type/_unique_sources_by_type)
and this duplicates logic elsewhere (also at the other block), so add a
Graph-level helper (e.g., Graph.copy_stats_from(source: Graph) or
Graph._hydrate_stats(state_dict)) that encapsulates copying/restoring all
private counters and collections, then replace the direct assignments in clone()
and the backend-load path with a single call to that helper; ensure the helper
performs deep copies for mutable structures (dicts, defaultdicts/sets) to
preserve original isolation.
In `@src/graphforge/storage/memory.py`:
- Around line 166-172: get_statistics currently constructs a new GraphStatistics
with a fresh timestamp on every read; introduce a stored _stats_last_updated
attribute on the Memory backend and use that when creating GraphStatistics in
get_statistics instead of generating a new timestamp, and ensure all mutation
methods (e.g., add_node, remove_node, add_edge, remove_edge or any method that
changes
_total_nodes/_total_edges/_node_counts_by_label/_edge_counts_by_type/_avg_degree_by_type)
update _stats_last_updated when they change state so last_updated truly reflects
the last mutation.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7ef60c92-3a6d-40c3-adf0-a6cecc34db0a
⛔ Files ignored due to path filters (5)
.github/workflows/changelog-check.ymlis excluded by!**/.github/**.github/workflows/test.ymlis excluded by!**/.github/**CHANGELOG.mdis excluded by!**/*.mddocs/development/parser-performance-analysis.mdis excluded by!**/*.md,!**/docs/**uv.lockis excluded by!**/*.lock,!**/*.lock
📒 Files selected for processing (7)
pyproject.tomlscripts/tck_perf_report.pysrc/graphforge/api.pysrc/graphforge/parser/parser.pysrc/graphforge/planner/planner.pysrc/graphforge/storage/memory.pytests/unit/optimizer/test_statistics.py
✅ Files skipped from review due to trivial changes (2)
- pyproject.toml
- tests/unit/optimizer/test_statistics.py
tck_perf_report.py:
- find_testsuite -> find_testsuites, returns all <testsuite> elements
- _tc_status helper: checks failure/error/skipped not just failure
- threshold formatting: {:.0f} -> {:.1f} for fractional second display
parser.py:
- set_labels: use _get_token_value() instead of str() for backtick stripping
- _KEYWORD_RE: fix backtick pattern to allow escaped backticks (foo``bar)
memory.py:
- edge replacement: clean up _unique_sources_by_type for old type when src
no longer has edges of that type; recompute _avg_degree_by_type for old type
- add _stats_last_updated timestamp, updated on every mutation, used in
get_statistics() instead of generating a fresh time.time() on each read
- add copy_stats_from(source: Graph) and hydrate_stats(stats: GraphStatistics)
helpers to encapsulate private stats layout
api.py:
- clone(): replace 6-line stats copy with cloned.graph.copy_stats_from(self.graph)
- backend load: replace 5-line restore with graph.hydrate_stats(loaded_stats)
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This was referenced Apr 28, 2026
5 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
2 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.
Closes #362
Closes #363
Closes #366
Summary
[:KNOWS|FOLLOWS](pipe without colon) now parses correctlyPerformance
model_copy()calls and full-edge-set scan; replaced with mutable counters and incremental_unique_sources_by_typetracking_split_create_sequence()splits queries like the TCK movie graph (971 CREATEs) into batches of 5 — reduces parse time from 27 minutes to ~26 secondsdocs/development/parser-performance-analysis.md; LALR(1) migration tracked as issue perf: migrate parser from xearley to LALR(1) for O(n) parsing #365CI
pytest-splitduration balancingscripts/tck_perf_report.py+ CI step to track per-test timing regressionspermissionsblocks on all workflow jobs