fix(collections): add IterateRaw() implementation for Multi index #25123
Conversation
📝 WalkthroughWalkthroughA new Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant MultiIndex
participant RefKeys
Client->>MultiIndex: IterateRaw(ctx, start, end, order)
MultiIndex->>RefKeys: IterateRaw(ctx, start, end, order)
RefKeys-->>MultiIndex: Iterator
MultiIndex-->>Client: Iterator
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes detected. Note 🔌 MCP (Model Context Protocol) integration is now available in Early Access!Pro users can now connect to remote MCP servers under the Integrations page to get reviews and chat conversations that understand additional development context. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
collections/indexes/multi.go (1)
122-129: Add a doc comment for IterateRawBriefly document delegation, caller Close() responsibility, and that semantics mirror KeySet.IterateRaw.
+// IterateRaw exposes raw iteration over the underlying index key space. +// It delegates to the embedded KeySet. The returned iterator must be closed +// by the caller. Range and ordering semantics follow KeySet.IterateRaw. func (m *Multi[ReferenceKey, PrimaryKey, Value]) IterateRaw( ctx context.Context, start, end []byte, order collections.Order, ) ( iter collections.Iterator[collections.Pair[ReferenceKey, PrimaryKey], collections.NoValue], err error, ) { return m.refKeys.IterateRaw(ctx, start, end, order) }collections/indexes/multi_test.go (3)
170-174: Use ErrNotFound for new inserts in lazyOldValueReturning a non-error from lazyOldValue implies an existing value and triggers unreference(). For fresh inserts, return ErrNotFound to avoid relying on Remove semantics.
- err := mi.Reference(ctx, ref, c, func() (company, error) { return c, nil }) + err := mi.Reference(ctx, ref, c, func() (company, error) { return company{}, collections.ErrNotFound })
176-190: Strengthen assertions to validate ordering and contentThese loops only assert non-empty keys and counts. Consider asserting the actual sequence to verify ascending vs. descending order and key correctness, catching ordering regressions.
I can propose concrete assertions comparing the collected slices from both iterations to ensure they are exact reversals and match expected key tuples.
Also applies to: 191-205
206-219: Add a raw-range test for IterateRawThis block uses MatchExact (typed range) rather than exercising IterateRaw’s start/end range. Add a test deriving start/end for a single reference key (e.g., "milan") via the key codec so IterateRaw is validated for bounded scans too.
If you confirm the preferred helper(s) for deriving raw range bounds from the pair key codec, I can provide a drop-in test snippet.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Knowledge Base: Disabled due to Reviews > Disable Knowledge Base setting
📒 Files selected for processing (2)
collections/indexes/multi.go(1 hunks)collections/indexes/multi_test.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Summary
🔇 Additional comments (1)
collections/indexes/multi.go (1)
122-129: LGTM: IterateRaw passthrough is correctDirectly delegating to refKeys.IterateRaw is the right behavior here and brings Multi in line with the Collection interface for raw iteration.
| iter, err := mi.MatchExact(ctx, uint64(1)) | ||
| require.NoError(t, err) | ||
| pks, err := iter.PrimaryKeys() | ||
| require.NoError(t, err) | ||
| expectedPks := []collections.Pair[uint64, uint64]{ | ||
| collections.Join(uint64(1), uint64(1)), | ||
| collections.Join(uint64(2), uint64(1)), | ||
| } | ||
| require.Equal(t, expectedPks, pks) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure iterator from MatchExact is closed
The iterator created at Line 131 isn’t closed. Add a defer right after the error check.
iter, err := mi.MatchExact(ctx, uint64(1))
require.NoError(t, err)
+defer iter.Close()
pks, err := iter.PrimaryKeys()📝 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.
| iter, err := mi.MatchExact(ctx, uint64(1)) | |
| require.NoError(t, err) | |
| pks, err := iter.PrimaryKeys() | |
| require.NoError(t, err) | |
| expectedPks := []collections.Pair[uint64, uint64]{ | |
| collections.Join(uint64(1), uint64(1)), | |
| collections.Join(uint64(2), uint64(1)), | |
| } | |
| require.Equal(t, expectedPks, pks) | |
| iter, err := mi.MatchExact(ctx, uint64(1)) | |
| require.NoError(t, err) | |
| defer iter.Close() | |
| pks, err := iter.PrimaryKeys() | |
| require.NoError(t, err) | |
| expectedPks := []collections.Pair[uint64, uint64]{ | |
| collections.Join(uint64(1), uint64(1)), | |
| collections.Join(uint64(2), uint64(1)), | |
| } | |
| require.Equal(t, expectedPks, pks) |
🤖 Prompt for AI Agents
In collections/indexes/multi_test.go around lines 131 to 139, the iterator
returned by mi.MatchExact is not closed, which can lead to resource leaks. After
checking that err is nil, add a defer statement to close the iterator to ensure
it is properly released after use.
|
|
||
| rawIter, err := mi.IterateRaw(ctx, nil, nil, collections.OrderAscending) | ||
| require.NoError(t, err) | ||
| defer iter.Close() |
There was a problem hiding this comment.
Bug: Closing the wrong iterator (resource leak)
Line 143 defers iter.Close(), but the iterator created at Line 141 is rawIter. rawIter is never closed.
- defer iter.Close()
+ defer rawIter.Close()📝 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.
| defer iter.Close() | |
| defer rawIter.Close() |
🤖 Prompt for AI Agents
In collections/indexes/multi_test.go at line 143, the deferred call incorrectly
closes iter instead of rawIter, causing a resource leak. Replace defer
iter.Close() with defer rawIter.Close() to ensure the correct iterator is closed
and resources are properly released.
|
@aljo242 any chance to review this anytime soon? |
Description
Implements
IterateRaw()missing for Multi indexCloses: #25122
Summary by CodeRabbit
New Features
Tests