fix(pg): Inconsistent conditional pattern matching BED-6695#73
Conversation
WalkthroughThis PR adds an outer-correlated traversal root path and supporting helpers, updates predicate dispatch to choose the new root when endpoints are pre-bound, regenerates SQL translation tests to adjust JOIN vs WHERE scoping for existence/negation patterns, and adds integration fixtures for BED-6695 pattern-predicate cases. ChangesPattern Predicate Outer-Correlated Translation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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
| } else { | ||
| // Left-side node is bound OR neither is bound | ||
| whereClause = pgsql.NewBinaryExpression( | ||
| pgsql.NewBinaryExpression( | ||
| pgsql.CompoundIdentifier{traversalStep.Edge.Identifier, pgsql.ColumnStartID}, | ||
| pgsql.OperatorEquals, | ||
| pgsql.CompoundIdentifier{traversalStep.LeftNode.Identifier, pgsql.ColumnID}, | ||
| ), | ||
| pgsql.OperatorOr, | ||
| pgsql.NewBinaryExpression( | ||
| pgsql.CompoundIdentifier{traversalStep.Edge.Identifier, pgsql.ColumnEndID}, | ||
| pgsql.OperatorEquals, | ||
| pgsql.CompoundIdentifier{traversalStep.LeftNode.Identifier, pgsql.ColumnID}, | ||
| ), | ||
| ) | ||
| } |
There was a problem hiding this comment.
Handle the fully-unbound optimization separately.
This fallback also covers !LeftNodeBound && !RightNodeBound, but it still compares against traversalStep.LeftNode.Identifier.id. That identifier is never introduced in the subquery, so a legal predicate like WHERE ()--() generates invalid SQL instead of a plain edge-existence check.
Suggested fix
- } else {
- // Left-side node is bound OR neither is bound
+ } else if traversalStep.LeftNodeBound {
+ // Left-side node is bound
whereClause = pgsql.NewBinaryExpression(
pgsql.NewBinaryExpression(
pgsql.CompoundIdentifier{traversalStep.Edge.Identifier, pgsql.ColumnStartID},
pgsql.OperatorEquals,
pgsql.CompoundIdentifier{traversalStep.LeftNode.Identifier, pgsql.ColumnID},
@@
),
)
+ } else {
+ // Neither side is bound; any edge satisfies the predicate.
+ whereClause = pgsql.NewLiteral(true, pgsql.Boolean)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } else { | |
| // Left-side node is bound OR neither is bound | |
| whereClause = pgsql.NewBinaryExpression( | |
| pgsql.NewBinaryExpression( | |
| pgsql.CompoundIdentifier{traversalStep.Edge.Identifier, pgsql.ColumnStartID}, | |
| pgsql.OperatorEquals, | |
| pgsql.CompoundIdentifier{traversalStep.LeftNode.Identifier, pgsql.ColumnID}, | |
| ), | |
| pgsql.OperatorOr, | |
| pgsql.NewBinaryExpression( | |
| pgsql.CompoundIdentifier{traversalStep.Edge.Identifier, pgsql.ColumnEndID}, | |
| pgsql.OperatorEquals, | |
| pgsql.CompoundIdentifier{traversalStep.LeftNode.Identifier, pgsql.ColumnID}, | |
| ), | |
| ) | |
| } | |
| } else if traversalStep.LeftNodeBound { | |
| // Left-side node is bound | |
| whereClause = pgsql.NewBinaryExpression( | |
| pgsql.NewBinaryExpression( | |
| pgsql.CompoundIdentifier{traversalStep.Edge.Identifier, pgsql.ColumnStartID}, | |
| pgsql.OperatorEquals, | |
| pgsql.CompoundIdentifier{traversalStep.LeftNode.Identifier, pgsql.ColumnID}, | |
| ), | |
| pgsql.OperatorOr, | |
| pgsql.NewBinaryExpression( | |
| pgsql.CompoundIdentifier{traversalStep.Edge.Identifier, pgsql.ColumnEndID}, | |
| pgsql.OperatorEquals, | |
| pgsql.CompoundIdentifier{traversalStep.LeftNode.Identifier, pgsql.ColumnID}, | |
| ), | |
| ) | |
| } else { | |
| // Neither side is bound; any edge satisfies the predicate. | |
| whereClause = pgsql.NewLiteral(true, pgsql.Boolean) | |
| } |
🤖 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 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.
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