Repository navigation
feat: Optimize PG source_kind, relation deletion using indices - BED-8832 - #2942
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:
📝 WalkthroughWalkthrough
ChangesGraph Data Deletion Refactor
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant DeleteCollectedGraphData
participant GraphDeletionCapability
participant StreamingFallback
DeleteCollectedGraphData->>GraphDeletionCapability: delete supported node or relationship sets
DeleteCollectedGraphData->>StreamingFallback: stream IDs when capability is unavailable
StreamingFallback->>StreamingFallback: drain IDs and delete each item
StreamingFallback-->>DeleteCollectedGraphData: return completion or first deletion error
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cmd/api/src/daemons/datapipe/delete.go (1)
206-218: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicated drain-and-delete writer loop.
deleteNodesByOperation's writer (Lines 206-218) anddeleteRelationshipsByOperation's writer (Lines 248-260) are identical except for thebatch.DeleteNode/batch.DeleteRelationshipcall. Could extract a small generic helper to remove the duplication.♻️ Proposed shared helper
func drainAndDelete(ctx context.Context, inC <-chan graph.ID, deleteFn func(graph.ID) error) error { for { if nextID, hasNextID := channels.Receive(ctx, inC); hasNextID { if err := deleteFn(nextID); err != nil { return err } } else { return nil } } }nodeOperation.SubmitWriter(func(ctx context.Context, batch graph.Batch, inC <-chan graph.ID) error { - for { - if nextID, hasNextID := channels.Receive(ctx, inC); hasNextID { - if err := batch.DeleteNode(nextID); err != nil { - return err - } - } else { - break - } - } - - return nil + return drainAndDelete(ctx, inC, batch.DeleteNode) })Also applies to: 248-260
🤖 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 `@cmd/api/src/daemons/datapipe/delete.go` around lines 206 - 218, Extract the duplicated drain-and-delete loop from the writers in deleteNodesByOperation and deleteRelationshipsByOperation into a shared drainAndDelete helper accepting the context, input channel, and deletion callback. Have the helper preserve channels.Receive termination and error propagation, then invoke it with batch.DeleteNode or batch.DeleteRelationship respectively.
🤖 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 `@cmd/api/src/daemons/datapipe/delete.go`:
- Around line 206-218: Extract the duplicated drain-and-delete loop from the
writers in deleteNodesByOperation and deleteRelationshipsByOperation into a
shared drainAndDelete helper accepting the context, input channel, and deletion
callback. Have the helper preserve channels.Receive termination and error
propagation, then invoke it with batch.DeleteNode or batch.DeleteRelationship
respectively.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: af323718-d2b6-42a1-9cd8-fe1af8ba5ddb
📒 Files selected for processing (1)
cmd/api/src/daemons/datapipe/delete.go
zinic
left a comment
zinic
left a comment
There was a problem hiding this comment.
Nothing stood directly out. Type assertions read fine. Operation usage ensures Done is called after launching - may want to investigate a defer if we were being pedantic but I think this is fine as-is.
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Description
Implements the new DeleteNodesByKinds/DeleteRelationshipsByKinds functions supported optionally on DAWGS drivers to execute rapid deletion of objects by kind. Initially, this is supported on PostgreSQL drivers.
Requires SpecterOps/DAWGS#101
Motivation and Context
Resolves BED-8832
Why is this change required? What problem does it solve?
How Has This Been Tested?
Validated locally and confirmed existing tests continue to pass.
Screenshots (optional):
Tested using large OGGen payloads for validation:
Delete by source_kind:
Before:
After:
Delete by relationship:
Before:
After:
Types of changes
Checklist:
Summary by CodeRabbit