fix(cypher): frontend should always emit exclusive kind matchers BED-7495#37
Open
fix(cypher): frontend should always emit exclusive kind matchers BED-7495#37
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughAdds an IsExclusive boolean to KindMatcher, sets it for frontend node-label parsing, propagates it through model copy/constructors, and uses it in PostgreSQL kind-ID translation to pick array-contains vs. overlap operators. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Frontend as Frontend Parser
participant Model as KindMatcher Model
participant Translator as PG Translator
participant Postgres as PostgreSQL
Client->>Frontend: submit query with node labels
Frontend->>Model: NewKindMatcher(..., isExclusive=true)
Model->>Translator: translateKindMatcher(kindMatcher{IsExclusive})
Translator->>Translator: choose operator (array-contains vs overlap) based on IsExclusive
Translator->>Postgres: emit SQL using chosen operator
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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.
🧹 Nitpick comments (2)
query/model.go (1)
132-137:Kind()relies on zero-value forIsExclusivewhile sibling functions are explicit.
DeleteKind,DeleteKinds, andKindInall explicitly setIsExclusive: false, butKind()creates aKindMatchervia struct literal without it. The zero-value behavior is correct, but adding the explicit field (or using the constructor) would be consistent with the rest of this PR.♻️ Consistency suggestion
func Kind(reference graph.Criteria, kinds ...graph.Kind) *cypherModel.KindMatcher { - return &cypherModel.KindMatcher{ - Reference: reference, - Kinds: kinds, - } + return cypherModel.NewKindMatcher(reference, kinds, false) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@query/model.go` around lines 132 - 137, Kind() currently relies on the zero-value for KindMatcher.IsExclusive while sibling functions (DeleteKind, DeleteKinds, KindIn) set IsExclusive: false explicitly; update Kind() to set IsExclusive: false in the returned cypherModel.KindMatcher struct literal (or call the same constructor used by the siblings) so it matches the explicit behavior of DeleteKind/DeleteKinds/KindIn and keeps consistency across the API.cypher/models/pgsql/translate/kind.go (1)
12-34:isExclusiveis silently ignored for edge/composite bindings.For edges (line 26–30), the function always emits
= ANY(...)regardless ofisExclusive. This is likely correct since edges carry a singlekind_id, but it's worth a brief comment at the edge branch to make this explicit — prevents future maintainers from wondering if it's an oversight.📝 Suggested comment
case pgsql.EdgeComposite, pgsql.ExpansionEdge: + // Edges have a single kind_id, so the exclusive/overlap distinction does not apply. treeTranslator.PushOperand(pgsql.CompoundIdentifier{binding.Identifier, pgsql.ColumnKindID})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cypher/models/pgsql/translate/kind.go` around lines 12 - 34, The branch in newPGKindIDMatcher that handles binding.DataType == pgsql.EdgeComposite or pgsql.ExpansionEdge ignores the isExclusive flag (it always emits an = ANY(...) expression); add a concise comment above the edge branch explaining that edges store a single kind_id so exclusivity is not applicable and isExclusive is intentionally ignored for EdgeComposite/ExpansionEdge, referencing newPGKindIDMatcher, binding.DataType, and the edge branch to make intent explicit for future maintainers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@cypher/models/pgsql/translate/kind.go`:
- Around line 12-34: The branch in newPGKindIDMatcher that handles
binding.DataType == pgsql.EdgeComposite or pgsql.ExpansionEdge ignores the
isExclusive flag (it always emits an = ANY(...) expression); add a concise
comment above the edge branch explaining that edges store a single kind_id so
exclusivity is not applicable and isExclusive is intentionally ignored for
EdgeComposite/ExpansionEdge, referencing newPGKindIDMatcher, binding.DataType,
and the edge branch to make intent explicit for future maintainers.
In `@query/model.go`:
- Around line 132-137: Kind() currently relies on the zero-value for
KindMatcher.IsExclusive while sibling functions (DeleteKind, DeleteKinds,
KindIn) set IsExclusive: false explicitly; update Kind() to set IsExclusive:
false in the returned cypherModel.KindMatcher struct literal (or call the same
constructor used by the siblings) so it matches the explicit behavior of
DeleteKind/DeleteKinds/KindIn and keeps consistency across the API.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cypher/frontend/expression.gocypher/models/cypher/model.gocypher/models/pgsql/translate/kind.goquery/model.go
seanjSO
force-pushed
the
seanj/BED-7495
branch
2 times, most recently
from
ad5ef1e to
7a63ae4
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cypher/models/pgsql/translate/kind.go`:
- Around line 12-24: Add a guard in newPGKindIDMatcher to handle empty kindIDs
when isExclusive is true: if isExclusive && len(kindIDs) == 0, push a boolean
FALSE literal (e.g. pgsql.NewLiteral(false, pgsql.Boolean)) into treeTranslator
via treeTranslator.PushOperand and return as a completed expression so the
matcher yields no rows; otherwise keep the existing logic that uses
pgsql.OperatorPGArrayLHSContainsRHS and pgsql.OperatorPGArrayOverlap. This
ensures the @> path (OperatorPGArrayLHSContainsRHS) does not vacuously match all
rows when kindIDs is empty.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cypher/frontend/expression.gocypher/models/cypher/model.gocypher/models/pgsql/translate/kind.goquery/model.go
🚧 Files skipped from review as they are similar to previous changes (2)
- cypher/models/cypher/model.go
- cypher/frontend/expression.go
seanjSO
force-pushed
the
seanj/BED-7495
branch
from
7a63ae4 to
7dadc28
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Refactor