feat: implement redundant traversal elimination in query optimizer#184
Conversation
) Add optimization pass to detect and eliminate duplicate pattern scan operators (ScanNodes, ExpandEdges, ExpandVariableLength), improving query performance by avoiding redundant graph traversals. Implementation: - Add _redundant_traversal_elimination_pass() method to QueryOptimizer - Add _compute_operator_signature() to compute unique signatures for operators - Add _predicate_signature() helper for predicate comparison - Integrate pass into optimize() pipeline (runs after predicate reordering) - Add enable_redundant_elimination parameter (default: True) Key features: - Detects operators with identical signatures (variable, labels, types, predicates) - Removes duplicates while preserving first occurrence - Respects pipeline boundaries (WITH, UNION, Subquery) - Correctly handles predicates and None values in signature matching Testing: - Add 20 unit tests covering basic elimination, edge cases, boundaries, complex patterns - Add 11 integration tests for end-to-end validation - Achieve 100% coverage on new optimizer code - All 3,031 tests passing (91.08% total coverage) Expected performance improvement: 5-20% on queries with duplicate patterns. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
WalkthroughAdds a redundant traversal elimination pass to the query optimizer that detects and removes duplicate pattern scans (ScanNodes, ExpandEdges, ExpandVariableLength) using operator signatures while respecting pipeline boundaries and side‑effect/NULL semantics. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 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
🤖 Fix all issues with AI agents
In `@src/graphforge/optimizer/optimizer.py`:
- Around line 366-398: The operator signature builder omits the path_var field,
causing distinct operators that bind different path variables (e.g.,
ScanNodes.path_var, ExpandEdges.path_var, ExpandVariableLength.path_var) to
collide; update the returns for the ScanNodes, ExpandEdges, and
ExpandVariableLength branches to include op.path_var (normalized to None when
absent) as an additional element in each signature tuple (keep existing
predicate_repr and other elements intact, and continue using tuple(...) for
edge/label collections and self._predicate_signature for predicates).
In `@tests/integration/test_optimizer_redundant_elimination.py`:
- Around line 192-215: The test currently only runs a single variable-length
match, so it doesn't exercise redundant variable-length path elimination; update
test_variable_length_path_redundancy to include a duplicate variable-length
pattern (for example duplicate the pattern in the MATCH clause like MATCH
(a:Person {name: 'Alice'})-[*1..2]->(b), (a)-[*1..2]->(b) or add an identical
MATCH clause) so the optimizer has redundant patterns to eliminate while keeping
the same assertions on results (names == {"Bob","Charlie"}). Ensure the change
is made in the test function test_variable_length_path_redundancy and that the
query string variable is the one modified.
🧹 Nitpick comments (5)
tests/unit/optimizer/test_redundant_elimination.py (2)
112-265: Consider using@pytest.mark.parametrizefor non-duplicate test cases.The six tests in
TestRedundantEliminationNonDuplicatesall follow the same pattern: build two operators with one differing attribute, optimize, assert both remain. These are strong candidates for parametrization.Similarly, the three boundary tests (With, Union, Subquery) at lines 271–323 share the same structure.
Example parametrized boundary tests
+ `@pytest.mark.parametrize`( + "boundary_op", + [ + With(items=[ReturnItem(expression=Variable(name="a"), alias="a")]), + Union(branches=[[], []], all=False), + Subquery( + operators=[ScanNodes(variable="n", labels=[["Person"]])], + expression_type="EXISTS", + ), + ], + ids=["with", "union", "subquery"], + ) + def test_dont_eliminate_across_boundary(self, boundary_op): + """Duplicates separated by a pipeline boundary are not eliminated.""" + ops = [ + ScanNodes(variable="a", labels=[["Person"]]), + boundary_op, + ScanNodes(variable="a", labels=[["Person"]]), + ] + + optimizer = QueryOptimizer() + result = optimizer.optimize(ops) + + assert len(result) == 3 + assert isinstance(result[0], ScanNodes) + assert isinstance(result[1], type(boundary_op)) + assert isinstance(result[2], ScanNodes)As per coding guidelines,
tests/**/*.py: "Use@pytest.mark.parametrizefor testing the same logic with different inputs".
367-390: This test conflates filter pushdown with redundant elimination.
optimizer.optimize(ops)runs all three passes. Here, filter pushdown absorbs theFilterinto the firstScanNodes(changing its signature), so the duplicate is actually not eliminated — bothScanNodessurvive because their signatures now differ. The comment on line 388 acknowledges this ambiguity, and the assertion at line 386 (len(result) == 3) happens to pass under both interpretations, making the test brittle.For a true unit test of redundant elimination in isolation, consider instantiating the optimizer with
enable_filter_pushdown=Falseor calling_redundant_traversal_elimination_passdirectly. As per coding guidelines,tests/unit/**/*.py: "Unit tests must test one component in isolation and complete in less than 1ms".src/graphforge/optimizer/optimizer.py (3)
318-319:seen_signaturesstores index values that are never read.The dict maps
signature → index in result, but only membership (signature in seen_signatures) is ever checked. Asetwould be simpler and communicates intent better. If the index is reserved for a planned downstream-reference-update feature, a brief comment would help.Simplification
- seen_signatures: dict[tuple, int] = {} # signature -> index in result + seen_signatures: set[tuple] = set() ... - seen_signatures[signature] = len(result) + seen_signatures.add(signature)
403-413:repr()-based predicate signatures are functional but worth documenting the assumption.
repr()on Pydantic v2 models is deterministic for identical field values, so this works today. However, it's sensitive to changes in the AST model definitions (e.g., adding fields, changing repr). A brief inline comment noting the Pydantic deterministic-repr dependency would help future maintainers.
256-269: IncludeExpandVariableLengthin predicate reordering.
ExpandVariableLengthnow carries apredicatefield but the_predicate_reorder_passmethod only handlesFilter,ScanNodes, andExpandEdges(line 260). Predicates inExpandVariableLengthoperators won't be reordered for selectivity optimization.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #184 +/- ##
==========================================
+ Coverage 87.37% 87.68% +0.30%
==========================================
Files 34 34
Lines 6022 6056 +34
Branches 1610 1618 +8
==========================================
+ Hits 5262 5310 +48
+ Misses 430 416 -14
Partials 330 330
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Sentry.
|
…tes (#167) Fixed a bug where operators with different path_var values were incorrectly considered duplicates, causing them to be eliminated when they should not be. Changes: - Add path_var field to ScanNodes signature tuple - Add path_var field to ExpandEdges signature tuple - Add path_var field to ExpandVariableLength signature tuple - Update test_variable_length_path_redundancy to use explicit edge variable (fixes test to actually exercise redundancy elimination) - Add unit tests for path_var handling: - test_dont_eliminate_different_path_vars: Operators with different path_var should not be eliminated - test_eliminate_with_same_path_var: Operators with same path_var should be eliminated This ensures operators are only considered duplicates when ALL fields match, including the path variable binding. All tests passing (3,055 tests, 100% coverage on optimizer code). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (7)
src/graphforge/optimizer/optimizer.py (3)
319-319:seen_signaturesvalue is never read — simplify to aset.The
intvalue (index inresult) is stored but never consumed. A plainset[tuple]communicates intent more clearly and avoids the unused mapping.♻️ Proposed fix
- seen_signatures: dict[tuple, int] = {} # signature -> index in result + seen_signatures: set[tuple] = set() ... - if signature in seen_signatures: + if signature in seen_signatures: # Duplicate found - skip it (first occurrence handles the work) continue - seen_signatures[signature] = len(result) + seen_signatures.add(signature) result.append(op)
347-365: Docstring omitspath_varfrom the signature description.The bullet list at lines 353–358 enumerates what the signature includes but doesn't mention
path_var, even though the implementation correctly includes it for all three operator types.📝 Proposed fix
Signature includes: - Operator type (class name) - Variable bindings - Labels/types - Direction + - Path variable (path_var) - Predicate (if present)
405-415:repr()-based predicate signatures are correct for frozen Pydantic models but fragile if AST types change.Since all AST models use
frozen = True,repr()is deterministic and this works. Just be aware that if any predicate AST node ever includes non-deterministic fields (e.g., insertion-ordered dicts that vary) or changes its__repr__, signatures could silently break. A structural recursive signature would be more robust long-term.tests/unit/optimizer/test_redundant_elimination.py (4)
18-34: Tests calloptimize()(all passes) instead of isolating the elimination pass.Per the unit-test guideline ("test one component in isolation"), these tests exercise filter-pushdown and predicate-reordering as well, which can mask or amplify elimination behaviour (as the comment on line 431 acknowledges). Consider either disabling the other passes or calling the pass method directly:
optimizer = QueryOptimizer( enable_filter_pushdown=False, enable_predicate_reorder=False, ) result = optimizer.optimize(ops)This keeps the tests focused on the elimination pass alone. As per coding guidelines,
tests/unit/**/*.py: "Unit tests must test one component in isolation and complete in less than 1ms."
112-280: Parametrize the non-duplicate tests — they share the same structure.The tests in
TestRedundantEliminationNonDuplicateseach create two operators that differ in one attribute, optimize, and assert both remain. This is a textbook@pytest.mark.parametrizecase. As per coding guidelines,tests/**/*.py: "Use@pytest.mark.parametrizefor testing the same logic with different inputs."♻️ Sketch (not exhaustive)
`@pytest.mark.parametrize`("ops,expected_count", [ # different variables ([ScanNodes(variable="a", labels=[["Person"]]), ScanNodes(variable="b", labels=[["Person"]])], 2), # different labels ([ScanNodes(variable="a", labels=[["Person"]]), ScanNodes(variable="a", labels=[["Company"]])], 2), # different path_var ([ScanNodes(variable="a", labels=[["Person"]], path_var="p1"), ScanNodes(variable="a", labels=[["Person"]], path_var="p2")], 2), # ... other cases ], ids=["diff_vars", "diff_labels", "diff_path_var"]) def test_non_duplicate_not_eliminated(self, ops, expected_count): optimizer = QueryOptimizer(enable_filter_pushdown=False, enable_predicate_reorder=False) result = optimizer.optimize(ops) assert len(result) == expected_count
410-433: Assertions are too loose — test acknowledges interference from other passes.The comment on line 431 notes that filter pushdown may alter the result, making the assertions vague (
len == 3,any(...Project...)). If you disable the other passes (per the earlier suggestion), you can assert the exact expected operator sequence, making this test deterministic and meaningful.
331-346:Union(branches=[[], []])uses empty branch lists.This works because the validator only checks
min_length=2on the outer list, but semantically these are empty pipelines. Consider using minimal realistic branches for clarity, though this is a minor point.
1 participant
Summary
Implements redundant traversal elimination optimization pass to detect and eliminate duplicate pattern scan operators, improving query performance by avoiding redundant graph traversals.
Closes #167
Changes
Core Implementation:
_redundant_traversal_elimination_pass()method toQueryOptimizer_compute_operator_signature()to compute unique operator signatures_predicate_signature()helper for predicate comparisonoptimize()pipeline (runs after predicate reordering)enable_redundant_eliminationparameter (default: True)Operators Supported:
ScanNodes- Node pattern scansExpandEdges- Relationship traversalsExpandVariableLength- Variable-length path expansionKey Features:
WITH,UNION,Subquery)Nonevalues in signature matchingenable_redundant_elimination=FalseTesting
Unit Tests (20 tests):
Integration Tests (11 tests):
Coverage:
Performance Impact
Expected improvement: 5-20% on queries with duplicate patterns
Example:
Before: 2 node scans
After: 1 node scan (duplicate eliminated)
Test Plan
make pre-push(all checks passed)🤖 Generated with Claude Code
Summary by CodeRabbit