fix: shortestPath() / allShortestPaths() parse and raise NotImplementedError#497
Merged
Conversation
…edError (#468) - Add SHORTEST_PATH_KW / ALL_SHORTEST_PATHS_KW terminals to cypher.lark - Add path_function grammar rule and path_function_binding pattern alternative - Add ShortestPathExpression AST node (frozen Pydantic model) - Add transformer methods: shortest_path_func, all_shortest_paths_func, path_function_binding - Add _reject_shortest_path() planner validation; raises NotImplementedError with BFS workaround: MATCH path = (a)-[*1..N]-(b) RETURN length(path) ORDER BY length(path) LIMIT 1 - 5 parser unit tests + 5 integration tests (10 total); pre-push green Closes #468 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #497 +/- ##
==========================================
+ Coverage 88.02% 88.05% +0.02%
==========================================
Files 40 40
Lines 14470 14513 +43
Branches 3434 3439 +5
==========================================
+ Hits 12737 12779 +42
+ Misses 1142 1141 -1
- Partials 591 593 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Sentry.
|
Contributor
|
Caution Review failedPull request was closed or merged during review WalkthroughAdded parser support and an AST node for ChangesShortest Path Graceful Degradation
Sequence DiagramsequenceDiagram
participant Client
participant Parser
participant AST
participant Planner
Client->>Parser: submit query with shortestPath()/allShortestPaths()
Parser->>AST: transform parse tree -> ShortestPathExpression
Client->>Planner: plan(query AST)
Planner->>AST: scan MATCH clauses for path_function
AST-->>Planner: ShortestPathExpression found
Planner-->>Client: raise NotImplementedError (includes BFS workaround hint)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 3
🧹 Nitpick comments (2)
tests/unit/parser/test_shortest_path_parser.py (1)
12-32: ⚡ Quick winParametrize the two parser happy-path cases.
test_shortest_path_parsesandtest_all_shortest_paths_parsesare the same test shape with different inputs and expectedallvalues, so they should be one parametrized test.As per coding guidelines, "Use pytest parametrization (
@pytest.mark.parametrize) when testing the same logic with different inputs to avoid code duplication".🤖 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/unit/parser/test_shortest_path_parser.py` around lines 12 - 32, Combine the two tests test_shortest_path_parses and test_all_shortest_paths_parses into one parametrized pytest function using `@pytest.mark.parametrize`; feed it two cases with the Cypher strings "MATCH p = shortestPath((a)-[*]-(b)) RETURN p" and "MATCH p = allShortestPaths((a)-[*]-(b)) RETURN p" and expected values for expr.all (False, True), then inside the single test call parse_cypher, retrieve match = ast.clauses[0], pattern = match.patterns[0], expr = pattern["path_function"], assert isinstance(expr, ShortestPathExpression) and assert expr.all equals the parameterized expected value so you remove duplicate logic in test_shortest_path_parses and test_all_shortest_paths_parses.tests/integration/test_shortest_path.py (1)
17-42: ⚡ Quick winParametrize the shortest/all-shortest-path exception cases.
These four tests differ only by function name and assertion target, so a small
@pytest.mark.parametrizetable would remove duplication and make future message-contract additions easier to keep in sync.As per coding guidelines, "Use pytest parametrization (
@pytest.mark.parametrize) when testing the same logic with different inputs to avoid code duplication".🤖 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/integration/test_shortest_path.py` around lines 17 - 42, The four tests test_shortest_path_raises_not_implemented, test_shortest_path_message_contains_workaround, test_all_shortest_paths_raises_not_implemented and test_all_shortest_paths_message_contains_workaround duplicate the same logic; refactor them using pytest.mark.parametrize to drive both function-name variants ("shortestPath" and "allShortestPaths") and the two assertion types (match string vs checking _WORKAROUND_HINT in exception text). Replace the duplicated GraphForge() setup and gf.execute(...) calls with a single parametrized test that calls GraphForge().execute with the appropriate Cypher (use the function name to build the query) and asserts either pytest.raises(..., match=...) for the function-name message or captures the exception and asserts _WORKAROUND_HINT in str(exc.value); reference GraphForge.execute and _WORKAROUND_HINT in the new test.
🤖 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 `@src/graphforge/parser/cypher.lark`:
- Around line 354-355: The two new terminals SHORTEST_PATH_KW.2 and
ALL_SHORTEST_PATHS_KW.2 are currently case-sensitive; update their regexes to be
case-insensitive (so variants like SHORTESTPATH or allshortestpaths match) by
adding an inline case-insensitive flag to each pattern (apply to
SHORTEST_PATH_KW.2 and ALL_SHORTEST_PATHS_KW.2) while preserving the negative
lookahead for alphanumeric/underscore to avoid partial matches.
In `@src/graphforge/planner/planner.py`:
- Around line 3416-3423: The _reject_shortest_path() check currently only
inspects MatchClause and OptionalMatchClause, so path_function_binding used in
other clause types (e.g., CREATE, MERGE) is missed; update the logic that
iterates ast.clauses and clause.patterns to not restrict to
MatchClause/OptionalMatchClause — either remove the isinstance guard or extend
it to include CreateClause and MergeClause (or any clause classes that can
contain pattern/path_function_binding) and ensure you still detect dict patterns
with "path_function" and ShortestPathExpression to map to func_name handling;
modify the loop around ast.clauses/for pattern in clause.patterns accordingly in
_reject_shortest_path().
- Around line 3424-3428: The NotImplementedError raised in planner.py (the raise
NotImplementedError(...) that uses func_name and the BFS workaround message)
must include the tracking issue number; update the error text to append or
include "See tracking issue `#468`" (or include "#468") so the message contains
the issue identifier alongside the existing workaround instructions in the
NotImplementedError raised for func_name.
---
Nitpick comments:
In `@tests/integration/test_shortest_path.py`:
- Around line 17-42: The four tests test_shortest_path_raises_not_implemented,
test_shortest_path_message_contains_workaround,
test_all_shortest_paths_raises_not_implemented and
test_all_shortest_paths_message_contains_workaround duplicate the same logic;
refactor them using pytest.mark.parametrize to drive both function-name variants
("shortestPath" and "allShortestPaths") and the two assertion types (match
string vs checking _WORKAROUND_HINT in exception text). Replace the duplicated
GraphForge() setup and gf.execute(...) calls with a single parametrized test
that calls GraphForge().execute with the appropriate Cypher (use the function
name to build the query) and asserts either pytest.raises(..., match=...) for
the function-name message or captures the exception and asserts _WORKAROUND_HINT
in str(exc.value); reference GraphForge.execute and _WORKAROUND_HINT in the new
test.
In `@tests/unit/parser/test_shortest_path_parser.py`:
- Around line 12-32: Combine the two tests test_shortest_path_parses and
test_all_shortest_paths_parses into one parametrized pytest function using
`@pytest.mark.parametrize`; feed it two cases with the Cypher strings "MATCH p =
shortestPath((a)-[*]-(b)) RETURN p" and "MATCH p = allShortestPaths((a)-[*]-(b))
RETURN p" and expected values for expr.all (False, True), then inside the single
test call parse_cypher, retrieve match = ast.clauses[0], pattern =
match.patterns[0], expr = pattern["path_function"], assert isinstance(expr,
ShortestPathExpression) and assert expr.all equals the parameterized expected
value so you remove duplicate logic in test_shortest_path_parses and
test_all_shortest_paths_parses.
🪄 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: 6f25348d-6c8a-4202-b5a2-a9ee53b6e718
📒 Files selected for processing (6)
src/graphforge/ast/expression.pysrc/graphforge/parser/cypher.larksrc/graphforge/parser/parser.pysrc/graphforge/planner/planner.pytests/integration/test_shortest_path.pytests/unit/parser/test_shortest_path_parser.py
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Case-insensitive SHORTEST_PATH_KW / ALL_SHORTEST_PATHS_KW terminals - _reject_shortest_path: use hasattr(clause, 'patterns') instead of MatchClause/OptionalMatchClause guard to catch any clause with patterns - NotImplementedError message now includes tracking issue (#468) - Parametrize parser and integration tests to remove duplication Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This was referenced May 5, 2026
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
1 participant
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
shortestPath(...)andallShortestPaths(...)previously raised an unhelpfulSyntaxError: Unexpected tokenat parse timeShortestPathExpressionAST node; the planner raisesNotImplementedErrorwith a clear BFS workaround messagep = (a)-[:R]->(b)) are unaffectedChanges
SHORTEST_PATH_KW,ALL_SHORTEST_PATHS_KWterminals;path_functionrule;path_function_bindingalternative inpatternShortestPathExpression(all: bool, pattern: Any)shortest_path_func,all_shortest_paths_func,path_function_bindingtransformer methods_reject_shortest_path()— raisesNotImplementedErrorwith BFS workaround hintTest plan
MATCH p = shortestPath((a)-[*]-(b)) RETURN praisesNotImplementedErrorwith workaround messageMATCH p = allShortestPaths((a)-[*]-(b)) RETURN praisesNotImplementedErrorMATCH path = (a)-[*1..N]-(b)BFS workaround hintMATCH p = (a)-[:R]->(b)still works (regular path binding unaffected)[*1..3]) still works (BFS workaround pattern)make pre-pushgreenCloses #468
🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmithwith what you need.Summary by CodeRabbit
New Features
shortestPath()andallShortestPaths()in queries and returns a clear NotImplementedError that includes a suggested BFS (variable-length path) workaround.Tests