fix(pg): Inconsistent conditional pattern matching BED-6695#73
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR refactors pattern predicate translation for Cypher queries by introducing outer-correlated traversal root builders. The changes branch join/WHERE logic based on which nodes are pre-bound in the outer query, then update SQL test expectations and add integration fixtures to validate pattern-existence semantics for negated and conditional patterns. ChangesPattern Predicate Outer-Correlated Translation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
seanjSO
changed the title
seanjSO
force-pushed
the
seanj/BED-6695
branch
2 times, most recently
from
4894815 to
24385cb
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
integration/testdata/cases/pattern_predicate_direction_inline.json (1)
1-131: ⚡ Quick winConsider extracting common fixture data to reduce duplication.
All six test cases define nearly identical inline fixtures (nodes u1, u2, g1, g2, g3, g4 and their edges). While this makes each test self-contained, it creates a maintenance burden—any fixture adjustment requires updating six locations.
If these fixtures need to remain inline, consider at least documenting why the duplication is intentional (e.g., for regression test isolation).
🤖 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 `@integration/testdata/cases/pattern_predicate_direction_inline.json` around lines 1 - 131, The fixtures for the six cases (e.g., cases named "regression: incoming negated pattern with contains predicate (left-directed form)", "right-directed equivalent", etc.) duplicate the same nodes and edges; extract that shared nodes/edges JSON into a single reusable fixture object (or helper function) and reference it from each case to avoid repetition, or if inline fixtures must remain, add a short explanatory comment in each case noting the intentional duplication for regression/isolation; update the cases that reference the repeated nodes (u1,u2,g1,g2,g3,g4) and edges (EdgeKind1/EdgeKind2 entries) to use the shared fixture identifier or include the comment.cypher/models/pgsql/test/pattern_predicate_shape_test.go (2)
38-40: 💤 Low valueAdd a comment explaining why this pattern is forbidden.
The forbidden fragment
"from s0 join edge e0 on (s0.n0).id = e0.end_id"represents the buggy SQL structure that this fix addresses, but the reason isn't documented. A brief comment would help future maintainers understand the regression being prevented.📝 Suggested comment
forbiddenFragments := []string{ + // This join pattern caused incorrect correlation in negated pattern predicates (BED-6695). + // The fix restructures the CTE to avoid binding the outer reference too early. "from s0 join edge e0 on (s0.n0).id = e0.end_id", }🤖 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 `@cypher/models/pgsql/test/pattern_predicate_shape_test.go` around lines 38 - 40, Add a short explanatory comment above the forbiddenFragments entry for the string "from s0 join edge e0 on (s0.n0).id = e0.end_id" stating that this SQL pattern is forbidden because it uses a nested node accessor `(s0.n0).id` in the JOIN ON clause (producing incorrect/buggy SQL generation or runtime errors for nullable/composite node columns) and that the test prevents regressions related to that specific buggy join shape; update the comment to reference the exact fragment and the regression it guards against so future maintainers understand why it must remain forbidden.
24-30: 💤 Low valueConsider deriving kind IDs dynamically from test constants.
The hard-coded array values
[1]and[3]in the SQL assertions must matchNodeKind1andEdgeKind1as mapped bynewKindMapper(). If the kind mapper changes, these assertions could silently pass with incorrect SQL.♻️ Alternative: compute expected values from the mapper
You could call
newKindMapper()and extract the mapped IDs to build the expected fragments dynamically. However, since this is a focused regression test for a specific bug fix and the mapper is stable test infrastructure, the current approach may be acceptable.🤖 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 `@cypher/models/pgsql/test/pattern_predicate_shape_test.go` around lines 24 - 30, The test currently hardcodes kind ID arrays in requiredFragments which can drift from the mapping; obtain the mapper via newKindMapper(), use it to look up the mapped IDs for NodeKind1 and EdgeKind1, and construct the expected SQL fragments using those computed IDs instead of literal "[1]" and "[3]"; update the requiredFragments construction (the variable named requiredFragments in the test) to interpolate the derived values so assertions remain correct if newKindMapper() changes.
🤖 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.
Nitpick comments:
In `@cypher/models/pgsql/test/pattern_predicate_shape_test.go`:
- Around line 38-40: Add a short explanatory comment above the
forbiddenFragments entry for the string "from s0 join edge e0 on (s0.n0).id =
e0.end_id" stating that this SQL pattern is forbidden because it uses a nested
node accessor `(s0.n0).id` in the JOIN ON clause (producing incorrect/buggy SQL
generation or runtime errors for nullable/composite node columns) and that the
test prevents regressions related to that specific buggy join shape; update the
comment to reference the exact fragment and the regression it guards against so
future maintainers understand why it must remain forbidden.
- Around line 24-30: The test currently hardcodes kind ID arrays in
requiredFragments which can drift from the mapping; obtain the mapper via
newKindMapper(), use it to look up the mapped IDs for NodeKind1 and EdgeKind1,
and construct the expected SQL fragments using those computed IDs instead of
literal "[1]" and "[3]"; update the requiredFragments construction (the variable
named requiredFragments in the test) to interpolate the derived values so
assertions remain correct if newKindMapper() changes.
In `@integration/testdata/cases/pattern_predicate_direction_inline.json`:
- Around line 1-131: The fixtures for the six cases (e.g., cases named
"regression: incoming negated pattern with contains predicate (left-directed
form)", "right-directed equivalent", etc.) duplicate the same nodes and edges;
extract that shared nodes/edges JSON into a single reusable fixture object (or
helper function) and reference it from each case to avoid repetition, or if
inline fixtures must remain, add a short explanatory comment in each case noting
the intentional duplication for regression/isolation; update the cases that
reference the repeated nodes (u1,u2,g1,g2,g3,g4) and edges (EdgeKind1/EdgeKind2
entries) to use the shared fixture identifier or include the comment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 50427ad2-cb7e-47a1-8ffb-cd7db5659309
📒 Files selected for processing (9)
cypher/models/pgsql/test/pattern_predicate_shape_test.gocypher/models/pgsql/test/translation_cases/nodes.sqlcypher/models/pgsql/test/translation_cases/stepwise_traversal.sqlcypher/models/pgsql/translate/predicate.gocypher/models/pgsql/translate/traversal.gointegration/testdata/bed6695.jsonintegration/testdata/cases/pattern_predicate_direction_inline.jsonintegration/testdata/cases/pattern_predicates.jsonintegration/testdata/empty.json
|
Change appears to cover the current issue identified in the ticket. I noticed however that this missed the undirected traversal path. Checking with local tests and tooling, the defect does still exist in undirected path predicate translation: I don't have a problem with deferring the fix above for undirected path predicates to a different ticket but I felt it worth bringing up. Everything else in this changeset passes muster. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cypher/models/pgsql/translate/predicate.go (1)
154-179:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep processing later pattern predicates after an optimized hit.
Line 178 exits
buildPatternPredicatesfrom inside the loop. If the sameWHEREhas another deferred pattern predicate after this one, it never gets resolved. This shouldcontinue, not return.Suggested fix
- return nil + continue🤖 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 `@cypher/models/pgsql/translate/predicate.go` around lines 154 - 179, In buildPatternPredicates, when an optimized relationship-exists predicate is applied for a single-step traversal (the block that calls buildOptimizedRelationshipExistPredicate and sets predicateFuture.SyntaxNode), do not return nil immediately; replace the early return with a continue so the loop keeps processing subsequent pattern predicates in the same WHERE; ensure the same traversalStep, traversalStepIdentifiers and predicateFuture handling remains unchanged and only the control flow is modified.
🤖 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 `@cypher/models/pgsql/translate/predicate.go`:
- Around line 75-90: The current fallback branch still references
traversalStep.LeftNode.Identifier even when both LeftNodeBound and
RightNodeBound are false, producing invalid SQL for a fully-unbound pattern; add
an explicit branch that detects (!traversalStep.LeftNodeBound &&
!traversalStep.RightNodeBound) and for that case set whereClause to a simple
edge-existence predicate (e.g., an IS NOT NULL or existence check on
traversalStep.Edge.Identifier/its primary id) or omit the predicate entirely
instead of comparing to traversalStep.LeftNode.Identifier.id; update the logic
around whereClause construction (the block using pgsql.NewBinaryExpression with
ColumnStartID/ColumnEndID and ColumnID) to only run when at least one side is
bound.
---
Outside diff comments:
In `@cypher/models/pgsql/translate/predicate.go`:
- Around line 154-179: In buildPatternPredicates, when an optimized
relationship-exists predicate is applied for a single-step traversal (the block
that calls buildOptimizedRelationshipExistPredicate and sets
predicateFuture.SyntaxNode), do not return nil immediately; replace the early
return with a continue so the loop keeps processing subsequent pattern
predicates in the same WHERE; ensure the same traversalStep,
traversalStepIdentifiers and predicateFuture handling remains unchanged and only
the control flow is modified.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a57d3c2d-4793-474f-8322-c533de8e9c46
📒 Files selected for processing (3)
cypher/models/pgsql/test/translation_cases/pattern_binding.sqlcypher/models/pgsql/translate/predicate.gocypher/models/pgsql/translate/traversal.go
seanjSO
force-pushed
the
seanj/BED-6695
branch
2 times, most recently
from
5fa1740 to
a5c8d42
Compare
2 participants
Description
Resolves: BED-6695
Type of Change
Testing
go test -tags manual_integration ./integration/...)Screenshots (if appropriate):
Driver Impact
drivers/pg)drivers/neo4j)Checklist
go.mod/go.sumare up to date if dependencies changedSummary by CodeRabbit
Bug Fixes
Tests