chore+feat: refactor triplestore and cardinality types#36
Conversation
WalkthroughThis PR refactors cardinality interfaces to separate immutable read-only access from mutable operations, optimizes Roaring set operations through length-based branching, and overhauls Triplestore's internal architecture from bitmap-based to edge slice-based storage with index maps for efficient traversal. JSON serialization is added to Edge struct. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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)
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.
Actionable comments posted: 8
🧹 Nitpick comments (2)
cardinality/roaring32.go (1)
70-72:bitmap32.Add()misses the single-value optimisation added tobitmap64.Add()This PR updated
bitmap64.Add()to branch onlen(values)(no-op for 0, directbitmap.Addfor 1,AddManyfor >1), butbitmap32.Add()still unconditionally callsAddManyfor all arities, including the single-element hot path.♻️ Align bitmap32.Add() with bitmap64.Add()
func (s bitmap32) Add(values ...uint32) { - s.bitmap.AddMany(values) + switch len(values) { + case 0: + case 1: + s.bitmap.Add(values[0]) + default: + s.bitmap.AddMany(values) + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cardinality/roaring32.go` around lines 70 - 72, bitmap32.Add currently calls s.bitmap.AddMany for all arities; change it to mirror bitmap64.Add by branching on len(values): return immediately for 0, call s.bitmap.Add(values[0]) for a single value, and call s.bitmap.AddMany(values) only when len(values) > 1 so the single-value hot path uses the direct Add method.container/triplestore.go (1)
204-210:Degreescounts distinct adjacent nodes, not edge count.Since
adjacent()returns deduplicated node IDs (via a bitmap), multi-edges between the same node pair are collapsed. If degree should represent the number of incident edges (standard graph theory definition), this undercounts for multigraphs. If this is intentional, a clarifying comment would help.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@container/triplestore.go` around lines 204 - 210, Degrees currently returns the number of distinct adjacent nodes by calling s.adjacent(node, direction), which de-duplicates multi-edges via a bitmap and thus undercounts in multigraphs; either update Degrees to count incident edges instead (iterate the underlying edge storage or add/consume a method that returns incident edges or edge IDs and return that length) or, if node-degree (unique neighbors) is intentional, add a clarifying comment above triplestore.Degrees stating it returns the number of distinct neighboring nodes (not total incident edges) and document behavior for multiedges; locate the logic in the Degrees function and the s.adjacent(node, direction) call to implement the chosen fix.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@algo/reach.go`:
- Line 281: The current code does a naked assertion of Clone() to
cardinality.Duplex[uint64] (reachBitmap :=
s.ReachOfComponentContainingMember(node,
direction).Clone().(cardinality.Duplex[uint64])) which will panic if Clone()
returns the narrower cardinality.Provider[uint64]; change it to assert to
cardinality.Provider[uint64] first and handle both cases safely: perform a type
switch or ok-check on the Clone() result, use it directly if it's already a
Duplex, otherwise take the Provider and construct or copy into a new Duplex
before using it (so XorReach and other hot-path code always receive a safe,
mutable Duplex without risking a panic).
In `@cardinality/lock.go`:
- Around line 103-108: threadSafeDuplex.Clone() and threadSafeSimplex.Clone()
perform unchecked assertions on s.provider.Clone() which can panic if Clone()
returns a narrower Provider[T]; change both to do a safe type-check: call cloned
:= s.provider.Clone(), attempt a type assertion (or type switch) to
Duplex[T]/Simplex[T] with the comma-ok form, and only wrap with
ThreadSafeDuplex/ThreadSafeSimplex when the assertion succeeds; otherwise return
cloned (the Provider[T]) directly so immutable/narrow implementations don't
cause a panic.
In `@container/adjacencymap.go`:
- Around line 122-125: The code currently does a naked type assertion on
outboundAdjacent.Clone() to cardinality.Duplex[uint64] which can panic if
Clone() returns an immutable Provider; change this to a safe check (e.g. cloned,
ok := outboundAdjacent.Clone().(cardinality.Duplex[uint64])) and if ok use
cloned, otherwise allocate a new mutable Duplex and copy/populate its contents
from the returned Provider before calling Or(inboundAdjacent); ensure you never
mutate the original outboundAdjacent provider and keep the subsequent
Or(inboundAdjacent) on the mutable Duplex (refer to outboundAdjacent.Clone(),
cardinality.Duplex[uint64], combinedAdjacent, and Or).
In `@container/triplestore.go`:
- Around line 56-58: The public method triplestore.Sort() currently panics and
will crash callers via the MutableTriplestore interface; replace the panic with
a safe implementation: either implement the actual sort logic on the
triplestore's internal slice/map of triples (use a stable sort on the slice
field used to store triples inside type triplestore) or, if sorting isn't ready,
change Sort() to be a no-op (remove panic) so it safely returns, or
alternatively change the MutableTriplestore.Sort signature to return an error
and update callers to handle it; locate the method named Sort on the triplestore
type and the MutableTriplestore interface to make the matching change.
- Around line 93-96: AddNode currently unconditionally overwrites
s.startIndex[node] and s.endIndex[node], wiping any indices added earlier (e.g.,
by AddEdge); change AddNode so it only initializes those map entries when the
key is absent instead of always setting them to nil — locate the
triplestore.AddNode method and use a presence check (comma-ok) on s.startIndex
and s.endIndex and only set the entry when it does not already exist.
- Around line 200-202: EachAdjacentNode currently discards the result of
s.adjacent(node, direction) and never calls the delegate; fix by capturing the
returned adjacent nodes (from s.adjacent(node, direction)), iterating over that
slice, invoking delegate(adjacent) for each element and stopping iteration if
delegate returns false so semantics match a typical walker; update the
EachAdjacentNode implementation to iterate the returned list and honor the
delegate's boolean return.
- Around line 174-194: The adjacent() function currently adds both edge.Start
and edge.End in the default (both-directions) branch, which incorrectly includes
the queried node itself; change the logic in adjacent() (and use
adjacentEdgeIndices/edges to find the edge) so that for each edge you only add
the opposite endpoint: if edge.Start == node then nodes.Add(edge.End) else if
edge.End == node then nodes.Add(edge.Start); alternatively skip adding any
endpoint equal to node. This ensures the queried node is not returned in its own
adjacency list.
In `@graph/types.go`:
- Line 185: The unchecked type assertion clone.bitmaps[kind] =
kindBitmap.Clone().(cardinality.Duplex[uint64]) can panic if Clone() returns a
Provider[uint64] that is not a Duplex; replace it with a checked assertion: call
kindBitmap.Clone(), assign to a local (e.g., cloned := kindBitmap.Clone()), then
use a type assertion with the comma-ok form or a type switch to detect whether
cloned implements cardinality.Duplex[uint64]; if it does, store it in
clone.bitmaps[kind], otherwise handle the fallback explicitly (either
create/convert to a Duplex when possible or panic with a clear diagnostic
message referencing kind and the concrete type) so future immutable or
non-Duplex providers do not cause silent runtime crashes.
---
Nitpick comments:
In `@cardinality/roaring32.go`:
- Around line 70-72: bitmap32.Add currently calls s.bitmap.AddMany for all
arities; change it to mirror bitmap64.Add by branching on len(values): return
immediately for 0, call s.bitmap.Add(values[0]) for a single value, and call
s.bitmap.AddMany(values) only when len(values) > 1 so the single-value hot path
uses the direct Add method.
In `@container/triplestore.go`:
- Around line 204-210: Degrees currently returns the number of distinct adjacent
nodes by calling s.adjacent(node, direction), which de-duplicates multi-edges
via a bitmap and thus undercounts in multigraphs; either update Degrees to count
incident edges instead (iterate the underlying edge storage or add/consume a
method that returns incident edges or edge IDs and return that length) or, if
node-degree (unique neighbors) is intentional, add a clarifying comment above
triplestore.Degrees stating it returns the number of distinct neighboring nodes
(not total incident edges) and document behavior for multiedges; locate the
logic in the Degrees function and the s.adjacent(node, direction) call to
implement the chosen fix.
| func (s *ReachabilityCache) XorReach(node uint64, direction graph.Direction, duplex cardinality.Duplex[uint64]) { | ||
| // Reach bitmap will contain the member due to resolution of component reach | ||
| reachBitmap := s.ReachOfComponentContainingMember(node, direction).Clone() | ||
| reachBitmap := s.ReachOfComponentContainingMember(node, direction).Clone().(cardinality.Duplex[uint64]) |
There was a problem hiding this comment.
Same unchecked .(cardinality.Duplex[uint64]) assertion on Clone() result
Same pattern as in graph/types.go:185 — ReachOfComponentContainingMember returns Duplex[uint64], Clone() now deliberately returns the narrower Provider[uint64], and the naked assertion to Duplex[uint64] will panic if an immutable clone implementation is introduced. The impact here is particularly acute because XorReach is called on the hot algorithmic path.
🛡️ Safer assertion form
- reachBitmap := s.ReachOfComponentContainingMember(node, direction).Clone().(cardinality.Duplex[uint64])
+ cloned, ok := s.ReachOfComponentContainingMember(node, direction).Clone().(cardinality.Duplex[uint64])
+ if !ok {
+ panic(fmt.Sprintf("cardinality: Clone() returned non-Duplex type for XorReach"))
+ }
+ reachBitmap := cloned🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@algo/reach.go` at line 281, The current code does a naked assertion of
Clone() to cardinality.Duplex[uint64] (reachBitmap :=
s.ReachOfComponentContainingMember(node,
direction).Clone().(cardinality.Duplex[uint64])) which will panic if Clone()
returns the narrower cardinality.Provider[uint64]; change it to assert to
cardinality.Provider[uint64] first and handle both cases safely: perform a type
switch or ok-check on the Clone() result, use it directly if it's already a
Duplex, otherwise take the Provider and construct or copy into a new Duplex
before using it (so XorReach and other hot-path code always receive a safe,
mutable Duplex without risking a panic).
cardinality/lock.go
Outdated
| func (s threadSafeDuplex[T]) Clone() Provider[T] { | ||
| s.lock.Lock() | ||
| defer s.lock.Unlock() | ||
|
|
||
| return ThreadSafeDuplex(s.provider.Clone()) | ||
| return ThreadSafeDuplex(s.provider.Clone().(Duplex[T])) | ||
| } |
There was a problem hiding this comment.
Clone() in the core thread-safe wrapper uses unchecked type assertions
Both threadSafeDuplex.Clone() (line 107) and threadSafeSimplex.Clone() (line 154) assert the result of s.provider.Clone() to a more specific interface without a safety check. Since Clone() now deliberately returns the narrower Provider[T] to accommodate future immutable implementations, these assertions are the origin of a pattern that propagates across the whole codebase. Any Duplex[T] or Simplex[T] whose Clone() returns an immutable view will panic here and in every wrapper wrapping it.
🛡️ Safer assertion form for both wrappers
func (s threadSafeDuplex[T]) Clone() Provider[T] {
s.lock.Lock()
defer s.lock.Unlock()
- return ThreadSafeDuplex(s.provider.Clone().(Duplex[T]))
+ cloned, ok := s.provider.Clone().(Duplex[T])
+ if !ok {
+ panic(fmt.Sprintf("cardinality: Clone() on Duplex[%T] returned non-Duplex type %T", *new(T), s.provider))
+ }
+ return ThreadSafeDuplex(cloned)
} func (s threadSafeSimplex[T]) Clone() Provider[T] {
s.lock.Lock()
defer s.lock.Unlock()
- return ThreadSafeSimplex(s.provider.Clone().(Simplex[T]))
+ cloned, ok := s.provider.Clone().(Simplex[T])
+ if !ok {
+ panic(fmt.Sprintf("cardinality: Clone() on Simplex[%T] returned non-Simplex type %T", *new(T), s.provider))
+ }
+ return ThreadSafeSimplex(cloned)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cardinality/lock.go` around lines 103 - 108, threadSafeDuplex.Clone() and
threadSafeSimplex.Clone() perform unchecked assertions on s.provider.Clone()
which can panic if Clone() returns a narrower Provider[T]; change both to do a
safe type-check: call cloned := s.provider.Clone(), attempt a type assertion (or
type switch) to Duplex[T]/Simplex[T] with the comma-ok form, and only wrap with
ThreadSafeDuplex/ThreadSafeSimplex when the assertion succeeds; otherwise return
cloned (the Provider[T]) directly so immutable/narrow implementations don't
cause a panic.
There was a problem hiding this comment.
Given the design of the interface I can't really think of anything better? CR, help me out here? Is there a better pattern I'm missing given the Provider interface and the need to have an immutable view of one?
There was a problem hiding this comment.
@zinic, great question! This is a classic tension in Go generics between type flexibility and type safety. Let me walk through several design patterns you could consider:
Option 1: Checked Assertion with Graceful Fallback
Instead of panicking, check the assertion and return the unwrapped Provider[T] if it fails:
func (s threadSafeDuplex[T]) Clone() Provider[T] {
s.lock.Lock()
defer s.lock.Unlock()
cloned := s.provider.Clone()
if duplex, ok := cloned.(Duplex[T]); ok {
return ThreadSafeDuplex(duplex)
}
// Immutable clone - return unwrapped (loses thread-safety)
return cloned
}Trade-off: Clones of thread-safe objects may not be thread-safe, but no panic. This requires documentation that the contract is maintained.
Option 2: Self-Referential Type Parameters (CRTP)
Use a self-referential constraint where the interface is parameterized to require Clone returns the same type:
type Cloneable[T any] interface {
Clone() T
}
type threadSafeDuplex[T uint32 | uint64, D Duplex[T]] struct {
provider D
lock *sync.Mutex
}
func (s threadSafeDuplex[T, D]) Clone() D {
s.lock.Lock()
defer s.lock.Unlock()
return ThreadSafeDuplex(s.provider.Clone().(D))
}Trade-off: More complex signatures, and callers must specify both T and D. Go's type inference is weak for these patterns, making usage awkward.
Option 3: Document the Contract (Current + Comments)
Keep your current design but add strong documentation that Clone() on Duplex[T] must return a type assertable to Duplex[T], even though the interface signature is Provider[T]:
// Clone returns a Provider[T] for flexibility, but implementations
// of Duplex[T] MUST return a value that can be asserted to Duplex[T].
// Violating this contract will cause a panic in thread-safe wrappers.
func (s threadSafeDuplex[T]) Clone() Provider[T] {
s.lock.Lock()
defer s.lock.Unlock()
return ThreadSafeDuplex(s.provider.Clone().(Duplex[T]))
}Trade-off: Simple code, but relies on convention. Any buggy implementation panics.
Option 4: Separate Immutable Views
Instead of Clone() returning different types, have an explicit method for immutability:
// Clone always returns the same mutability level
func (d Duplex[T]) Clone() Duplex[T]
// AsImmutable provides a read-only view
func (d Duplex[T]) AsImmutable() ImmutableDuplex[T]Trade-off: Clearer intent, but changes your API surface.
My Recommendation
For your use case, I'd go with Option 1 (checked assertion with fallback). Here's why:
- It's pragmatic: No panics in production, even if someone writes a non-conforming implementation
- Preserves flexibility: Truly immutable clones can exist
- Minimal code complexity: No extra type parameters or API changes
- Self-documenting: The behavior (loss of thread-safety for immutable clones) is visible in the code
The reality is that if someone implements Clone() on a Duplex[T] to return an ImmutableDuplex[T], losing the thread-safe wrapper is probably correct behavior anyway—immutable objects don't need synchronization.
Would this approach work for your design goals?
container/adjacencymap.go
Outdated
| combinedAdjacent := outboundAdjacent.Clone().(cardinality.Duplex[uint64]) | ||
| combinedAdjacent.Or(inboundAdjacent) | ||
|
|
||
| return combinedAdjacent |
There was a problem hiding this comment.
Unchecked .(cardinality.Duplex[uint64]) assertion on Clone() result
Same pattern as graph/types.go:185 — outboundAdjacent is Duplex[uint64], Clone() returns Provider[uint64], and the naked assertion will panic if any Duplex[uint64] implementation's Clone() returns an immutable provider. Cloning here is essential (to avoid mutating the live adjacency map entry), so a panic would silently corrupt graph traversal.
🛡️ Safer assertion form
- combinedAdjacent := outboundAdjacent.Clone().(cardinality.Duplex[uint64])
+ cloned, ok := outboundAdjacent.Clone().(cardinality.Duplex[uint64])
+ if !ok {
+ panic(fmt.Sprintf("cardinality: Clone() on Duplex[uint64] returned non-Duplex type %T", outboundAdjacent))
+ }
+ combinedAdjacent := cloned📝 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.
| combinedAdjacent := outboundAdjacent.Clone().(cardinality.Duplex[uint64]) | |
| combinedAdjacent.Or(inboundAdjacent) | |
| return combinedAdjacent | |
| cloned, ok := outboundAdjacent.Clone().(cardinality.Duplex[uint64]) | |
| if !ok { | |
| panic(fmt.Sprintf("cardinality: Clone() on Duplex[uint64] returned non-Duplex type %T", outboundAdjacent)) | |
| } | |
| combinedAdjacent := cloned | |
| combinedAdjacent.Or(inboundAdjacent) | |
| return combinedAdjacent |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@container/adjacencymap.go` around lines 122 - 125, The code currently does a
naked type assertion on outboundAdjacent.Clone() to cardinality.Duplex[uint64]
which can panic if Clone() returns an immutable Provider; change this to a safe
check (e.g. cloned, ok := outboundAdjacent.Clone().(cardinality.Duplex[uint64]))
and if ok use cloned, otherwise allocate a new mutable Duplex and copy/populate
its contents from the returned Provider before calling Or(inboundAdjacent);
ensure you never mutate the original outboundAdjacent provider and keep the
subsequent Or(inboundAdjacent) on the mutable Duplex (refer to
outboundAdjacent.Clone(), cardinality.Duplex[uint64], combinedAdjacent, and Or).
| clone := NewThreadSafeKindBitmap() | ||
| for kind, kindBitmap := range s.bitmaps { | ||
| clone.bitmaps[kind] = kindBitmap.Clone() | ||
| clone.bitmaps[kind] = kindBitmap.Clone().(cardinality.Duplex[uint64]) |
There was a problem hiding this comment.
Unchecked type assertion will panic if Clone() returns a non-Duplex provider
kindBitmap.Clone() now returns Provider[uint64] by design, explicitly to leave room for immutable clone implementations. Immediately asserting .(cardinality.Duplex[uint64]) with no fallback will panic the moment any Duplex[uint64] implementation returns a narrower (e.g. immutable) clone. All current concrete types are safe, but this is a latent trap for the future.
Consider a checked assertion with an explicit panic message to aid diagnostics:
🛡️ Safer assertion form
- clone.bitmaps[kind] = kindBitmap.Clone().(cardinality.Duplex[uint64])
+ cloned, ok := kindBitmap.Clone().(cardinality.Duplex[uint64])
+ if !ok {
+ panic(fmt.Sprintf("cardinality: Clone() on Duplex[uint64] returned non-Duplex type %T", kindBitmap))
+ }
+ clone.bitmaps[kind] = cloned📝 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.
| clone.bitmaps[kind] = kindBitmap.Clone().(cardinality.Duplex[uint64]) | |
| cloned, ok := kindBitmap.Clone().(cardinality.Duplex[uint64]) | |
| if !ok { | |
| panic(fmt.Sprintf("cardinality: Clone() on Duplex[uint64] returned non-Duplex type %T", kindBitmap)) | |
| } | |
| clone.bitmaps[kind] = cloned |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@graph/types.go` at line 185, The unchecked type assertion clone.bitmaps[kind]
= kindBitmap.Clone().(cardinality.Duplex[uint64]) can panic if Clone() returns a
Provider[uint64] that is not a Duplex; replace it with a checked assertion: call
kindBitmap.Clone(), assign to a local (e.g., cloned := kindBitmap.Clone()), then
use a type assertion with the comma-ok form or a type switch to detect whether
cloned implements cardinality.Duplex[uint64]; if it does, store it in
clone.bitmaps[kind], otherwise handle the fallback explicitly (either
create/convert to a Duplex when possible or panic with a clear diagnostic
message referencing kind and the concrete type) so future immutable or
non-Duplex providers do not cause silent runtime crashes.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
container/triplestore.go (4)
75-85:cmp.Comparewould simplify theSortFunccomparator.The hand-rolled 3-branch comparator is correct but verbose.
slices.SortFunc's comparator should return a negative number whena < b, a positive number whena > b, and zero when equal. The standard library'scmp.Comparesatisfies exactly this contract for ordered types.♻️ Proposed simplification
+import "cmp" slices.SortFunc(s.edges, func(a, b Edge) int { - if a.ID > b.ID { - return 1 - } - - if a.ID < b.ID { - return -1 - } - - return 0 + return cmp.Compare(a.ID, b.ID) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@container/triplestore.go` around lines 75 - 85, The comparator passed to slices.SortFunc on s.edges uses a verbose three-branch comparison for Edge.ID; replace that logic with the standard library cmp.Compare to simplify and ensure correct sign semantics: in the SortFunc call for s.edges (comparing Edge.ID), call cmp.Compare(a.ID, b.ID) and return its result instead of the manual if/else branches so the comparator is concise and correct.
200-211:triplestore.AdjacentEdges(returns[]uint64IDs) clashes with the package-levelAdjacentEdges(returns[]Edge).Both are named
AdjacentEdgesbut return different types with different semantics. The struct method is not part of any interface, so it has no external users beyond internal calls. Renaming the method (e.g.,AdjacentEdgeIDs) would eliminate the ambiguity without changing the public API.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@container/triplestore.go` around lines 200 - 211, The method triplestore.AdjacentEdges returns []uint64 (edge IDs) and conflicts with the package-level AdjacentEdges that returns []Edge; rename the struct method to AdjacentEdgeIDs to clarify semantics, update its signature and all internal call sites (calls that currently use triplestore.AdjacentEdges) to use AdjacentEdgeIDs, and keep the implementation using adjacentEdgeIndices and s.edges[].ID unchanged so callers continue to receive []uint64 IDs.
109-125:NumNodes()allocates a bitmap on every call — consider a dedicated node set.
nodeBitmap()is called only byNumNodes(). For large triplestores, this allocates and populates a fullBitmap64just to callGetCardinality(). SincestartIndexandendIndexalready track the distinct nodes as map keys, the union cardinality can be computed without any bitmap allocation:♻️ Proposed alternative
func (s *triplestore) NumNodes() uint64 { seen := make(map[uint64]struct{}, len(s.startIndex)) for n := range s.startIndex { seen[n] = struct{}{} } for n := range s.endIndex { seen[n] = struct{}{} } return uint64(len(seen)) }This avoids the bitmap allocation entirely if
nodeBitmaphas no other callers. ThenodeBitmaphelper itself can then be removed ifEachNodeis refactored to use the same pattern.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@container/triplestore.go` around lines 109 - 125, NumNodes currently calls nodeBitmap which allocates and populates a Bitmap64 on every call; replace this with a direct union-cardinality computation over the existing map keys to avoid allocation. Modify NumNodes to iterate startIndex and endIndex, insert keys into a temporary map[uint64]struct{} (or maintain a dedicated node set on triplestore) and return the length as uint64; then remove nodeBitmap if it has no other callers and adjust EachNode (if it iterates nodes) to reuse the same key-union approach or the new dedicated node set to keep behavior consistent. Ensure you reference and update the functions/fields nodeBitmap, NumNodes, EachNode, startIndex, and endIndex accordingly.
137-153:EachNodeallocates aBitmap64for deduplication — same concern asnodeBitmap.Like
NumNodes, this allocates a roaring bitmap purely for set-membership tracking. For large triplestores on hot traversal paths, amap[uint64]struct{}is simpler and has no external dependency. This is a low-priority item paired with theNumNodessuggestion above.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@container/triplestore.go` around lines 137 - 153, EachNode currently creates a cardinality.Bitmap64 named nodes to deduplicate node IDs; replace that with a plain map[uint64]struct{} for membership checks to avoid the external bitmap allocation. In the EachNode method, declare nodes := make(map[uint64]struct{}), use _, ok := nodes[nodeID] to test presence and nodes[nodeID] = struct{}{} to insert when iterating s.startIndex and s.endIndex, preserving the existing short-circuit return when delegate(nodeID) is false; remove the cardinality usage/import once replaced.container/triplestore_test.go (1)
177-201:TestEachAdjacentEdgecouples to insertion order — considerassert.ElementsMatch.
EachAdjacentEdgereturns edges in the order they appear instartIndex[node], which is currently insertion order. The test passes today because that order matches the expected slice, but any change to howAddEdgeoradjacentEdgeIndicesorders indices will silently flip the assertion. Unless the contract guarantees insertion-order traversal, preferassert.ElementsMatchhere (as the analogousTestEachAdjacentNodealready does at Line 145).♻️ Proposed fix
- assert.Equal(t, expected, collected) + assert.ElementsMatch(t, expected, collected)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@container/triplestore_test.go` around lines 177 - 201, The test TestEachAdjacentEdge currently asserts equality which couples the spec to insertion order for edges returned by EachAdjacentEdge (which depends on startIndex/adjacentEdgeIndices and AddEdge); change the final assertion to use assert.ElementsMatch(t, expected, collected) so the test only checks for matching elements regardless of order, or alternatively enforce a deterministic sort before asserting if order is intended.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@container/triplestore.go`:
- Around line 171-198: adjacentEdgeIndices currently returns s.startIndex[node]
or s.endIndex[node] directly for DirectionOutbound/Inbound but returns a
numerically-sorted slice from Bitmap64.Slice() for the default (both) case,
causing inconsistent ordering; update adjacentEdgeIndices to sort the
outbound/inbound slices before returning (e.g., use sort.Slice or sort.Uint64s
on the slice returned from s.startIndex[node] and s.endIndex[node]) so all paths
produce a consistent numeric order, or alternatively add a clear comment
documenting the differing ordering contract if you prefer to keep insertion
order for single-direction calls.
- Around line 10-15: Edge.Kind is declared as the interface graph.Kind which
will marshal but not unmarshal correctly; fix this by making Edge handle JSON
explicitly: implement Edge.MarshalJSON and Edge.UnmarshalJSON (or a wrapper type
like EdgeJSON) to serialize Kind to a stable representation (e.g., its String()
or a type-tagged object) and to reconstruct the concrete graph.Kind
implementation on Unmarshal (using a type switch, registry, or parse helper).
Alternatively, make the Kind field a concrete type instead of graph.Kind if a
single implementation exists; update the Edge struct and any callers accordingly
so JSON round-trips preserve the concrete Kind value.
- Around line 242-248: The loop variable in EachAdjacentNode shadows the
parameter node; change the iteration variable name used in the range over
s.adjacent(node, direction) (e.g., to adj or adjacentID) so it no longer hides
the function parameter, and update uses inside the loop to call delegate with
that new variable name; keep the call to s.adjacent(node, direction) and the
delegate invocation semantics unchanged.
---
Nitpick comments:
In `@container/triplestore_test.go`:
- Around line 177-201: The test TestEachAdjacentEdge currently asserts equality
which couples the spec to insertion order for edges returned by EachAdjacentEdge
(which depends on startIndex/adjacentEdgeIndices and AddEdge); change the final
assertion to use assert.ElementsMatch(t, expected, collected) so the test only
checks for matching elements regardless of order, or alternatively enforce a
deterministic sort before asserting if order is intended.
In `@container/triplestore.go`:
- Around line 75-85: The comparator passed to slices.SortFunc on s.edges uses a
verbose three-branch comparison for Edge.ID; replace that logic with the
standard library cmp.Compare to simplify and ensure correct sign semantics: in
the SortFunc call for s.edges (comparing Edge.ID), call cmp.Compare(a.ID, b.ID)
and return its result instead of the manual if/else branches so the comparator
is concise and correct.
- Around line 200-211: The method triplestore.AdjacentEdges returns []uint64
(edge IDs) and conflicts with the package-level AdjacentEdges that returns
[]Edge; rename the struct method to AdjacentEdgeIDs to clarify semantics, update
its signature and all internal call sites (calls that currently use
triplestore.AdjacentEdges) to use AdjacentEdgeIDs, and keep the implementation
using adjacentEdgeIndices and s.edges[].ID unchanged so callers continue to
receive []uint64 IDs.
- Around line 109-125: NumNodes currently calls nodeBitmap which allocates and
populates a Bitmap64 on every call; replace this with a direct union-cardinality
computation over the existing map keys to avoid allocation. Modify NumNodes to
iterate startIndex and endIndex, insert keys into a temporary
map[uint64]struct{} (or maintain a dedicated node set on triplestore) and return
the length as uint64; then remove nodeBitmap if it has no other callers and
adjust EachNode (if it iterates nodes) to reuse the same key-union approach or
the new dedicated node set to keep behavior consistent. Ensure you reference and
update the functions/fields nodeBitmap, NumNodes, EachNode, startIndex, and
endIndex accordingly.
- Around line 137-153: EachNode currently creates a cardinality.Bitmap64 named
nodes to deduplicate node IDs; replace that with a plain map[uint64]struct{} for
membership checks to avoid the external bitmap allocation. In the EachNode
method, declare nodes := make(map[uint64]struct{}), use _, ok := nodes[nodeID]
to test presence and nodes[nodeID] = struct{}{} to insert when iterating
s.startIndex and s.endIndex, preserving the existing short-circuit return when
delegate(nodeID) is false; remove the cardinality usage/import once replaced.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (8)
algo/reach.gocardinality/cardinality.gocardinality/roaring32.gocardinality/roaring64.gocontainer/triplestore.gocontainer/triplestore_test.gogo.modgraph/types.go
🚧 Files skipped from review as they are similar to previous changes (4)
- graph/types.go
- algo/reach.go
- go.mod
- cardinality/roaring64.go
| type Edge struct { | ||
| ID uint64 | ||
| Start uint64 | ||
| End uint64 | ||
| ID uint64 `json:"id"` | ||
| Kind graph.Kind `json:"kind"` | ||
| Start uint64 `json:"start_id"` | ||
| End uint64 `json:"end_id"` | ||
| } |
There was a problem hiding this comment.
graph.Kind is an interface — JSON unmarshaling of Edge.Kind will fail at runtime.
json.Marshal will encode the concrete value's fields or String() if it implements json.Marshaler, but json.Unmarshal into an interface-typed field produces map[string]interface{}, not the original concrete graph.Kind. If Edge is ever round-tripped through JSON (e.g., stored, sent over the wire, or deserialized in tests), the Kind field will silently unmarshal to the wrong type.
If round-trip fidelity is required, either use a concrete type for Kind, add a custom UnmarshalJSON, or document the limitation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@container/triplestore.go` around lines 10 - 15, Edge.Kind is declared as the
interface graph.Kind which will marshal but not unmarshal correctly; fix this by
making Edge handle JSON explicitly: implement Edge.MarshalJSON and
Edge.UnmarshalJSON (or a wrapper type like EdgeJSON) to serialize Kind to a
stable representation (e.g., its String() or a type-tagged object) and to
reconstruct the concrete graph.Kind implementation on Unmarshal (using a type
switch, registry, or parse helper). Alternatively, make the Kind field a
concrete type instead of graph.Kind if a single implementation exists; update
the Edge struct and any callers accordingly so JSON round-trips preserve the
concrete Kind value.
| func (s *triplestore) adjacentEdgeIndices(node uint64, direction graph.Direction) []uint64 { | ||
| switch direction { | ||
| case graph.DirectionOutbound: | ||
| if outboundEdges, hasOutbound := s.startIndex[node]; hasOutbound { | ||
| edgeIndices.Or(outboundEdges) | ||
| return outboundEdges | ||
| } | ||
|
|
||
| case graph.DirectionInbound: | ||
| if inboundEdges, hasInbound := s.endIndex[node]; hasInbound { | ||
| edgeIndices.Or(inboundEdges) | ||
| return inboundEdges | ||
| } | ||
|
|
||
| default: | ||
| edgeIndices := cardinality.NewBitmap64() | ||
|
|
||
| if outboundEdges, hasOutbound := s.startIndex[node]; hasOutbound { | ||
| edgeIndices.Or(outboundEdges) | ||
| edgeIndices.Add(outboundEdges...) | ||
| } | ||
|
|
||
| if inboundEdges, hasInbound := s.endIndex[node]; hasInbound { | ||
| edgeIndices.Or(inboundEdges) | ||
| edgeIndices.Add(inboundEdges...) | ||
| } | ||
|
|
||
| return edgeIndices.Slice() | ||
| } | ||
|
|
||
| return edgeIndices | ||
| return nil | ||
| } |
There was a problem hiding this comment.
adjacentEdgeIndices returns indices in different orders depending on direction.
For DirectionOutbound/DirectionInbound, indices are returned in insertion order (direct slice from the map). For the default (both-directions) case, indices go through a Bitmap64 whose Slice() returns them in numerically sorted order. Callers using DirectionBoth will observe edge indices — and therefore edges — in a different order than they were inserted, which is inconsistent with the other two directions. If stable ordering matters across all direction values, sort the slice in the outbound/inbound paths as well, or document the ordering contract per direction.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@container/triplestore.go` around lines 171 - 198, adjacentEdgeIndices
currently returns s.startIndex[node] or s.endIndex[node] directly for
DirectionOutbound/Inbound but returns a numerically-sorted slice from
Bitmap64.Slice() for the default (both) case, causing inconsistent ordering;
update adjacentEdgeIndices to sort the outbound/inbound slices before returning
(e.g., use sort.Slice or sort.Uint64s on the slice returned from
s.startIndex[node] and s.endIndex[node]) so all paths produce a consistent
numeric order, or alternatively add a clear comment documenting the differing
ordering contract if you prefer to keep insertion order for single-direction
calls.
| func (s *triplestore) EachAdjacentNode(node uint64, direction graph.Direction, delegate func(adjacent uint64) bool) { | ||
| s.adjacent(node, direction).Each(delegate) | ||
| for _, node := range s.adjacent(node, direction) { | ||
| if !delegate(node) { | ||
| break | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
node loop variable shadows the node parameter in EachAdjacentNode.
func (s *triplestore) EachAdjacentNode(node uint64, direction graph.Direction, delegate func(adjacent uint64) bool) {
for _, node := range s.adjacent(node, direction) {s.adjacent(node, direction) is evaluated correctly before the loop variable is set, so the runtime behaviour is currently fine. However, after the first iteration, the symbol node inside the loop body refers to the iteration value, not the parameter. This is a maintenance hazard — any future code added inside the loop that intends to reference the parameter will silently use the wrong value.
🐛 Proposed fix
func (s *triplestore) EachAdjacentNode(node uint64, direction graph.Direction, delegate func(adjacent uint64) bool) {
- for _, node := range s.adjacent(node, direction) {
- if !delegate(node) {
- break
- }
- }
+ for _, adjacentNode := range s.adjacent(node, direction) {
+ if !delegate(adjacentNode) {
+ break
+ }
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@container/triplestore.go` around lines 242 - 248, The loop variable in
EachAdjacentNode shadows the parameter node; change the iteration variable name
used in the range over s.adjacent(node, direction) (e.g., to adj or adjacentID)
so it no longer hides the function parameter, and update uses inside the loop to
call delegate with that new variable name; keep the call to s.adjacent(node,
direction) and the delegate invocation semantics unchanged.
1 participant
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Tests
Chores