Conversation
…loses #367) The chunked one-hop pipeline (`execute_one_hop_chunked`) had no edge-property read or filter stage. `can_use_one_hop_chunked` correctly excluded queries referencing edge variables in RETURN or WHERE, but did NOT exclude queries with inline relationship property filters (e.g. `[r:KNOWS {weight: 0.9}]`). Such queries were routed to the chunked path, which ignored the inline filter entirely — returning rows that should have been excluded and projecting edge prop values as Null. Fix: add a `!pat.rels[0].props.is_empty()` guard to `can_use_one_hop_chunked` so any query with an inline rel prop filter falls back to the row engine (`execute_one_hop`) where the filter is properly evaluated after reading edge_props.bin. Added regression_367.rs covering: float filter + projection, exclusion of non-matching edges, integer filter + exclusion, and post-checkpoint behaviour. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
User does not have a PR Review subscription. Go to Team management and add this email to the PR Review subscription. |
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 18 minutes and 14 seconds. ⌛ 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: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughUpdated Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/sparrowdb-execution/src/engine/pipeline_exec.rs (1)
1099-1116: Consider adding the samerel.propsguard tocan_use_two_hop_chunkedandcan_use_mutual_neighbors_chunkedfor consistency.The two-hop path checks for edge-property references in RETURN/WHERE (lines 1099-1116) and node inline props (line 1125), but doesn't guard against inline relationship property filters (
rel.props). A query like:MATCH (a)-[r1:KNOWS {weight: 0.9}]->(b)-[r2:KNOWS]->(c) RETURN r1.weightwould have the same issue fixed here for one-hop. The same applies to
can_use_mutual_neighbors_chunked(lines 738-755).♻️ Suggested guard for two-hop
// Inline prop filters on node patterns are not evaluated by the chunked // two-hop path — fall back to the row engine. (See `#362`.) if pat.nodes.iter().any(|n| !n.props.is_empty()) { return false; } +// Inline prop filters on relationship patterns are not evaluated by the +// chunked two-hop path — fall back to the row engine. (See `#367`.) +if pat.rels.iter().any(|r| !r.props.is_empty()) { + return false; +} // id(n) and other NodeRef-dependent functions require the row engine (`#372`).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/sparrowdb-execution/src/engine/pipeline_exec.rs` around lines 1099 - 1116, The two-hop and mutual-neighbors chunked checks need the same guard against inline relationship property filters as the one-hop logic: in can_use_two_hop_chunked and can_use_mutual_neighbors_chunked, when iterating pattern relationships (pat.rels) and checking rel.var, also check rel.props (e.g., if !rel.props.is_empty()) and return false if any relationship has inline properties, so queries with relationship property filters (rel.props) are excluded just like node inline props; add this condition alongside the existing rel.var/ref-in-return and expr_references_var checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@crates/sparrowdb-execution/src/engine/pipeline_exec.rs`:
- Around line 1099-1116: The two-hop and mutual-neighbors chunked checks need
the same guard against inline relationship property filters as the one-hop
logic: in can_use_two_hop_chunked and can_use_mutual_neighbors_chunked, when
iterating pattern relationships (pat.rels) and checking rel.var, also check
rel.props (e.g., if !rel.props.is_empty()) and return false if any relationship
has inline properties, so queries with relationship property filters (rel.props)
are excluded just like node inline props; add this condition alongside the
existing rel.var/ref-in-return and expr_references_var checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3e5072ec-18de-4954-9f4e-12ca7a273091
📒 Files selected for processing (2)
crates/sparrowdb-execution/src/engine/pipeline_exec.rscrates/sparrowdb/tests/regression_367.rs
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses issue #367 where inline relationship property filters were not being correctly evaluated in the chunked one-hop path. The fix involves falling back to the row engine when relationship properties are present in the pattern to ensure filters are applied correctly. Additionally, a comprehensive regression test suite has been added to verify the fix across various scenarios, including property projections and post-checkpoint behavior. I have no feedback to provide.
…unked paths CodeRabbit review noted the one-hop fix in #367 left the same gap in can_use_two_hop_chunked and can_use_mutual_neighbors_chunked: inline relationship property filters (e.g. [r:KNOWS {weight: 0.9}]) would still be silently ignored when routed through those chunked paths. - Add `pat.rels.iter().any(|r| !r.props.is_empty())` guard to can_use_mutual_neighbors_chunked (alongside the existing edge-var checks) - Add the same guard to can_use_two_hop_chunked (after the node-props guard) Both functions now consistently fall back to the row engine whenever any inline rel prop filter is present, matching the one-hop behaviour. Co-Authored-By: Claude <noreply@anthropic.com>
Benchmark Results |
1 similar comment
Benchmark Results |
1 participant
Summary
can_use_one_hop_chunkedinpipeline_exec.rsdid not guard against inline relationship property filters ([r:KNOWS {weight: 0.9}]). Queries with such filters were routed to the chunked pipeline, which has no edge-property read or filter stage — so the inline filter was silently ignored, returning rows that should have been excluded.!pat.rels[0].props.is_empty()incan_use_one_hop_chunkedto fall back to the row engine (execute_one_hop) when any inline rel prop filter is present. The row engine correctly readsedge_props.binand applies the filter.crates/sparrowdb/tests/regression_367.rswith 4 end-to-end tests covering float filter + projection, exclusion of non-matching edges, integer filter + exclusion, and post-checkpoint behaviour.Root Cause Detail
The query
MATCH (a)-[r:KNOWS {weight: 0.9}]->(b) RETURN r.weightwent through this path:rel_pat.props = [{key:"weight", value:0.9}]can_use_one_hop_chunked— existing guards excluded RETURN/WHERE edge-var refs, but not inline rel prop filters (which live inpat.rels[0].props, not in the WHERE clause)The fix is minimal and correct: fall back to the proven row engine for any query that needs edge-property filtering. A future enhancement could add an edge-prop filter stage to the chunked pipeline.
Test plan
cargo check -p sparrowdbpassescargo fmt --allno diffscrates/sparrowdb/tests/regression_367.rs— 4 new tests all passcrates/sparrowdb/tests/spa_178_edge_properties.rs— all 11 tests pass (2 were failing before fix)crates/sparrowdb/tests/spa_229_edge_prop_float.rs— all 4 passcrates/sparrowdb/tests/spa_261_edge_props_perf.rs— all 4 passcargo test -p sparrowdb— only pre-existing regression_355 failures (unrelated)🤖 Generated with Claude Code
Summary by CodeRabbit
[r:KNOWS {since:2020}]) that project relationship properties now execute correctly and maintain consistency across database checkpoints.