fix: operator precedence, outline substitution, xfail precision#359
Conversation
- Add ParenthesizedExpr AST node to preserve explicit parentheses through all four layers (parser → planner → executor → evaluator), fixing column name fidelity for queries like `(12 / 4 * (3 - 2 * 4))` - Fix pytest-bdd Scenario Outline substitution: replace broad `<(.+?)>` regex (which consumed across `<>` not-equal operators) with a word-char- only alternative, unblocking all operator-precedence TCK scenarios - Refactor `_XFAIL_REASONS_AND_PATTERNS` from 8 broad patterns to 7 precise ones, eliminating all 339 false XPASS results while retaining 20 genuine platform-limitation xfails (CPython nanosecond truncation, clock-twice non-determinism, extreme-year overflow) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
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 (5)
WalkthroughThis PR introduces a Changes
Sequence DiagramsequenceDiagram
participant Parser as Parser (Cypher.lark)
participant Transformer as ASTTransformer
participant AST as ParenthesizedExpr Node
participant Evaluator as Evaluator
participant Executor as Executor
participant Planner as Planner
Parser->>Parser: Parse `(a + b)` as parenthesized_expr
Parser->>Transformer: Invoke parenthesized_expr transformer
Transformer->>AST: Create ParenthesizedExpr(expr=inner_expr)
Evaluator->>AST: Evaluate ParenthesizedExpr
AST->>Evaluator: Return evaluation of inner expr
Executor->>AST: Generate column name
AST->>Executor: Return "(a + b)" preserving parentheses
Planner->>AST: Check for aggregates
AST->>Planner: Unwrap and delegate to inner expr
Planner->>Planner: Detect aggregates transparently
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 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}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/graphforge/executor/evaluator.py (1)
130-149:⚠️ Potential issue | 🔴 CriticalForward
executorand recurseParenthesizedExprin literal containers.At Line 192,
evaluate_expressionis called withoutexecutor, which breaks parenthesized expressions that depend on executor context (e.g., subqueries/custom functions).
Also,ParenthesizedExpris missing from the literal list/map recursive evaluation checks, so parenthesized elements can be treated as raw Python objects instead of AST expressions.🐛 Proposed fix
@@ - if isinstance(expr, ParenthesizedExpr): - return evaluate_expression(expr.expr, ctx) + if isinstance(expr, ParenthesizedExpr): + return evaluate_expression(expr.expr, ctx, executor) @@ if isinstance( item, ( Literal, Variable, PropertyAccess, Subscript, BinaryOp, UnaryOp, FunctionCall, CaseExpression, ListComprehension, PatternComprehension, QuantifierExpression, SubqueryExpression, + ParenthesizedExpr, ), ) @@ if isinstance( val, ( Literal, Variable, PropertyAccess, Subscript, BinaryOp, UnaryOp, FunctionCall, CaseExpression, ListComprehension, PatternComprehension, QuantifierExpression, SubqueryExpression, + ParenthesizedExpr, ), ):Also applies to: 159-174, 190-193
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/graphforge/executor/evaluator.py` around lines 130 - 149, The recursive evaluation of literal containers misses ParenthesizedExpr and fails to forward the executor to evaluate_expression, so update the type-check lists used when iterating list/map items to include ParenthesizedExpr (alongside Literal, Variable, PropertyAccess, Subscript, BinaryOp, UnaryOp, FunctionCall, CaseExpression, ListComprehension, PatternComprehension, QuantifierExpression, SubqueryExpression) and ensure every call to evaluate_expression inside those container-handling blocks passes the executor argument (e.g., evaluate_expression(item, ctx, executor)); apply the same changes to the other similar blocks handling maps/sets where evaluate_expression is invoked.
🧹 Nitpick comments (2)
tests/tck/conftest.py (2)
333-380: Step signature/refactor LGTM — minor DRY suggestion.All four step functions now do the same
_fix_outline_placeholders → _substitute_parameters → execute → capture-errorpipeline. The duplication is fine for now, but if you add more execution variants in the future this is a natural place to factor out a small private helper:♻️ Optional consolidation
def _run_query(tck_context, docstring, request, *, use_params: bool): params = tck_context.get("parameters", {}) query = _fix_outline_placeholders(docstring, request) query = _substitute_parameters(query, params) try: kwargs = {"parameters": params} if use_params and params else {} tck_context["result"] = tck_context["graph"].execute(query, **kwargs) except Exception as e: tck_context["result"] = {"error": str(e)}Each step function then becomes a 1-liner.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/tck/conftest.py` around lines 333 - 380, The four step handlers (execute_query_colon, execute_query, execute_control_query_colon, execute_control_query) duplicate the same pipeline; refactor by adding a private helper (e.g. _run_query) that accepts (tck_context, docstring, request, use_params: bool) to perform _fix_outline_placeholders → _substitute_parameters → execute and error capture, then replace each handler with a one-line call to _run_query passing use_params=True for the parameterized query variants and False for control-query variants so behavior is preserved.
305-330: Consider adding a unit test for_fix_outline_placeholders.Per the repo's testing guidelines, new code under
tests/**should still be exercised._fix_outline_placeholdershas several non-trivial branches (no<short-circuit, missingcallspec, empty example dict, mixed<>+<placeholder>input) that aren't covered by indirect TCK runs.A small
tests/unit/test_outline_placeholder_fix.pyparametrized over inputs like:
"WHERE a <> 1 AND b = <name>"withexample={"name": "'x'"}→"WHERE a <> 1 AND b = 'x'""<a> <> <b>"withexample={"a": "1", "b": "2"}→"1 <> 2"- query without
<→ unchanged- request without
callspec→ unchangedwould lock in the regex behavior and protect against future pytest-bdd internal-API changes.
As per coding guidelines: "Write comprehensive unit tests for each layer (parser, planner, executor) with 100% coverage on new code (90% minimum)" and "Use
@pytest.mark.parametrizefor testing the same logic with different inputs to avoid code duplication".Want me to draft the parametrized unit test file?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/tck/conftest.py` around lines 305 - 330, Add a new parametrized unit test module (e.g., tests/unit/test_outline_placeholder_fix.py) that directly calls _fix_outline_placeholders and covers its branches: no-"<" short-circuit, request without callspec (simulate a fake request object lacking node.callspec), empty example dict, and mixed "<>"/"<placeholder>" inputs; use inputs like "WHERE a <> 1 AND b = <name>" with example={"name":"'x'"} expecting "WHERE a <> 1 AND b = 'x'", "<a> <> <b>" with example={"a":"1","b":"2"} expecting "1 <> 2", a query without "<" expecting unchanged output, and a request missing callspec expecting unchanged output; construct the request fixture by creating a simple object with node.callspec.params or omitting node/callspec to trigger the AttributeError path, and assert outputs against expected strings to lock behavior of _OUTLINE_PLACEHOLDER_RE and _fix_outline_placeholders.
🤖 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 322-324: The matching/substitution logic doesn't handle
ParenthesizedExpr even though _expression_to_string preserves parens; update
both _expressions_match and _substitute_groupby_aliases to recognize
ParenthesizedExpr and recurse into its .expr: in _expressions_match, when either
side is a ParenthesizedExpr, unwrap and call _expressions_match on the inner
expr (so parenthesized forms compare equal to their inner expression for
matching/sort-path purposes); in _substitute_groupby_aliases, when encountering
a ParenthesizedExpr, call _substitute_groupby_aliases on .expr and wrap the
result back into a ParenthesizedExpr (preserving explicit parens while enabling
alias substitution). Ensure the logic references the ParenthesizedExpr type and
uses the existing helper functions for recursion.
In `@src/graphforge/planner/planner.py`:
- Around line 1374-1375: The three expression-walking methods
QueryPlanner._collect_expr_variables, QueryPlanner._validate_non_agg_context,
and QueryPlanner._validate_agg_arg_scope currently skip ParenthesizedExpr nodes;
update each to handle ParenthesizedExpr the same way as the aggregate walker
(i.e., when encountering a ParenthesizedExpr delegate to the inner expr:
return/continue with self._collect_expr_variables(expr.expr) or
self._validate_non_agg_context(expr.expr) /
self._validate_agg_arg_scope(expr.expr) as appropriate) so parenthesized
expressions are traversed for ORDER BY/scope validation and aggregate checking
consistently.
---
Outside diff comments:
In `@src/graphforge/executor/evaluator.py`:
- Around line 130-149: The recursive evaluation of literal containers misses
ParenthesizedExpr and fails to forward the executor to evaluate_expression, so
update the type-check lists used when iterating list/map items to include
ParenthesizedExpr (alongside Literal, Variable, PropertyAccess, Subscript,
BinaryOp, UnaryOp, FunctionCall, CaseExpression, ListComprehension,
PatternComprehension, QuantifierExpression, SubqueryExpression) and ensure every
call to evaluate_expression inside those container-handling blocks passes the
executor argument (e.g., evaluate_expression(item, ctx, executor)); apply the
same changes to the other similar blocks handling maps/sets where
evaluate_expression is invoked.
---
Nitpick comments:
In `@tests/tck/conftest.py`:
- Around line 333-380: The four step handlers (execute_query_colon,
execute_query, execute_control_query_colon, execute_control_query) duplicate the
same pipeline; refactor by adding a private helper (e.g. _run_query) that
accepts (tck_context, docstring, request, use_params: bool) to perform
_fix_outline_placeholders → _substitute_parameters → execute and error capture,
then replace each handler with a one-line call to _run_query passing
use_params=True for the parameterized query variants and False for control-query
variants so behavior is preserved.
- Around line 305-330: Add a new parametrized unit test module (e.g.,
tests/unit/test_outline_placeholder_fix.py) that directly calls
_fix_outline_placeholders and covers its branches: no-"<" short-circuit, request
without callspec (simulate a fake request object lacking node.callspec), empty
example dict, and mixed "<>"/"<placeholder>" inputs; use inputs like "WHERE a <>
1 AND b = <name>" with example={"name":"'x'"} expecting "WHERE a <> 1 AND b =
'x'", "<a> <> <b>" with example={"a":"1","b":"2"} expecting "1 <> 2", a query
without "<" expecting unchanged output, and a request missing callspec expecting
unchanged output; construct the request fixture by creating a simple object with
node.callspec.params or omitting node/callspec to trigger the AttributeError
path, and assert outputs against expected strings to lock behavior of
_OUTLINE_PLACEHOLDER_RE and _fix_outline_placeholders.
🪄 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: 1d8cd029-527b-4722-9cee-c94c39130460
📒 Files selected for processing (8)
src/graphforge/ast/__init__.pysrc/graphforge/ast/expression.pysrc/graphforge/executor/evaluator.pysrc/graphforge/executor/executor.pysrc/graphforge/parser/cypher.larksrc/graphforge/parser/parser.pysrc/graphforge/planner/planner.pytests/tck/conftest.py
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #359 +/- ##
==========================================
+ Coverage 89.11% 89.29% +0.18%
==========================================
Files 39 39
Lines 11683 11714 +31
Branches 2531 2540 +9
==========================================
+ Hits 10411 10460 +49
+ Misses 891 855 -36
- Partials 381 399 +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.
|
Address CodeRabbit review findings: - evaluator.py: forward executor arg in ParenthesizedExpr pass-through; add ParenthesizedExpr to literal list/map item isinstance guards - executor.py: handle ParenthesizedExpr in _substitute_groupby_aliases and unwrap in _expressions_match (both sides) for ORDER BY alias matching - planner.py: add ParenthesizedExpr cases to _collect_expr_variables, _validate_non_agg_context, and _validate_agg_arg_scope Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Cover the code paths added in the previous two commits: - evaluator: ParenthesizedExpr pass-through (with executor), and ParenthesizedExpr inside list/map literals - executor: _expressions_match unwrapping on both left and right sides - planner: _collect_expr_variables, _validate_non_agg_context, and _validate_agg_arg_scope traversal through ParenthesizedExpr Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
1 participant
Closes #358
Summary
ParenthesizedExprAST node: New node to preserve explicit parentheses through all four layers, fixing column name fidelity for expressions like(12 / 4 * (3 - 2 * 4)). Added to grammar (named rule), transformer, evaluator (pass-through), executor (_expression_to_string), and planner (aggregate detection/extraction).render_stringused<(.+?)>which consumed across<>(not-equal operator), causingKeyErrorin all operator-precedence Scenario Outlines. Fixed with a word-char-only_fix_outline_placeholdershelper called from all query-executing step functions._XFAIL_REASONS_AND_PATTERNSwith 7 precise ones. Eliminates all 339 false XPASS results (stale patterns matching tests our impl passes fine) while retaining 20 genuine platform-limitation xfails.Test plan
uv run pytest tests/tck/test_features.py --tb=no -q→ 3419 passed, 429 failed, 20 xfailed, 0 xpasseduv run ruff check src/ tests/— no issuesuv run ruff format --check src/ tests/— no issuesuv run mypy src/graphforge --strict-optional --show-error-codes— no issues🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes