-
Notifications
You must be signed in to change notification settings - Fork 0
Re-work segment.Fields/Terms to use iterators #79
base: master
Are you sure you want to change the base?
Conversation
index/segment/fs/segment.go
Outdated
@@ -209,24 +206,29 @@ func (r *fsSegment) Close() error { | |||
return errReaderClosed | |||
} | |||
r.closed = true | |||
// ensure all references that depenend on this segment are released |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: sp of "depend"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for the catch. fixed.
Codecov Report
@@ Coverage Diff @@
## master #79 +/- ##
==========================================
- Coverage 72.54% 72.18% -0.37%
==========================================
Files 54 57 +3
Lines 2284 2398 +114
==========================================
+ Hits 1657 1731 +74
- Misses 411 442 +31
- Partials 216 225 +9
Continue to review full report at Codecov.
|
961d808
to
cc6eb88
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will it be hard to rebase the other FST diff onto this one? Just curious if it would make sense to hold off for that one.
type fstTermsIter struct { | ||
iterOpts newFstTermsIterOpts | ||
|
||
initialized bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure if it's worth it but I think you can resolve the maligned
lint by moving initialized
above done
:
iter *vellum.FSTIterator
err error
initialized bool
done bool
return f.err | ||
} | ||
|
||
func (f *fstTermsIter) Close() error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need to set f.done
to true here in case someone calls close before reaching the end of the iterator?
} | ||
|
||
if err != nil { | ||
f.done = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: maybe only set f.done
to true if the error was a vellum.ErrIteratorDone
in case we ever want to distinguish the two case in the future?
"github.com/couchbase/vellum" | ||
) | ||
|
||
type newFstTermsIterOpts struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: it seems strange to me to call these options since they're actually required parameters and we need to call close on them, maybe it would be worth just passing them directly in the constructor?
var _ sgmt.OrderedBytesSliceIterator = &fstTermsIter{} | ||
|
||
func (f *fstTermsIter) Next() bool { | ||
if f.done || f.err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need to check if the context has been closed here?
@@ -209,24 +206,29 @@ func (r *fsSegment) Close() error { | |||
return errReaderClosed | |||
} | |||
r.closed = true | |||
// ensure all references that depened on this segment are released | |||
r.ctx.BlockingClose() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
side thought: should we integrate this context into the mem segment as well? but if we do, aren't we back to ref-counting?
b.done = true | ||
return false | ||
} | ||
b.current = b.backingSlice[b.currentIdx] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we're reusing the underlying byte slice should we call out in the interface definition that the byte slice returned from Current
is only valid until the subsequent call to Next
?
// Next returns a bool indicating if there are any more elements | ||
Next() bool | ||
|
||
// Current returns the current element. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we call that the element is only valid until the subsequent call to Next
?
Fixes #66