Skip afterUid after decoder seek #3149
Conversation
There was a problem hiding this comment.
Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion (waiting on @srfrog)
posting/list.go, line 106 at r2 (raw file):
// The decoder Seek includes afterUid in uids, we must skip it to iterate correctly. if it.Valid() && it.uids[0] == afterUid {
Can you add tests for some edge cases. Like if you put after to math.Maxuint64, etc. what happens?
There was a problem hiding this comment.
Reviewable status: 0 of 3 files reviewed, 2 unresolved discussions (waiting on @srfrog)
posting/list.go, line 290 at r2 (raw file):
} return l.iterate(txn.StartTs, 0, func(obj *pb.Posting) error {
Also, the right place to do this would be in iterate function. Because we might have the UID come in from a mutation as well. In which case, it would still get output.
Also, can you add which PR caused the breakage in the first place? So, the previous behavior (before v1.0.10) can be understood -- because it used to work before apparently.
The whence value indicates how to use the uid offset with Seek. Whence can be SeekStart (0) or SeekCurrent (1). SeekStart will return the list including the offset. SeekCurrent returns values after the offset.
|
I've tested this PR and is working to #3109 |
srfrog
added
the
kind/maintenance
There was a problem hiding this comment.
Reviewed 2 of 3 files at r1, 5 of 6 files at r3.
Reviewable status: 7 of 8 files reviewed, 3 unresolved discussions (waiting on @srfrog)
codec/codec_test.go, line 93 at r3 (raw file):
{in: 10000, out: 10000, whence: SeekStart}, {in: 9999, out: 10000, whence: SeekCurrent}, }
Is there a reason there are no tests for SeekEnd?
There was a problem hiding this comment.
Reviewable status: 7 of 8 files reviewed, 3 unresolved discussions (waiting on @codexnull and @srfrog)
codec/codec_test.go, line 93 at r3 (raw file):
Previously, codexnull (Javier Alvarado) wrote…
Is there a reason there are no tests for SeekEnd?
I haven't implemented it yet. First I wanted to know if this API change is acceptable.
There was a problem hiding this comment.
Reviewed 2 of 3 files at r1, 6 of 6 files at r3.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @codexnull and @srfrog)
algo/uidlist.go, line 47 at r3 (raw file):
} dec := codec.Decoder{Pack: pack} dec.Seek(afterUID, codec.SeekStart)
Is this the right behavior? Do we have tests which can check for this edge case?
codec/codec.go, line 128 at r3 (raw file):
return (whence == SeekStart && pack.Blocks[i].Base >= uid) || (whence == SeekCurrent && pack.Blocks[i].Base > uid) })
The SeekStart or SeekCurrent can be done after sort.Search. And sort.Search can use >=. That'd be cheaper computationally speaking (which makes a difference here).
codec/codec.go, line 148 at r3 (raw file):
uidx := sort.Search(len(d.uids), func(i int) bool { return (whence == SeekStart && d.uids[i] >= uid) || (whence == SeekCurrent && d.uids[i] > uid)
Same here.
There was a problem hiding this comment.
Reviewable status: 6 of 8 files reviewed, 6 unresolved discussions (waiting on @codexnull and @manishrjain)
algo/uidlist.go, line 47 at r3 (raw file):
Previously, manishrjain (Manish R Jain) wrote…
Is this the right behavior? Do we have tests which can check for this edge case?
Done.
codec/codec.go, line 128 at r3 (raw file):
Previously, manishrjain (Manish R Jain) wrote…
The SeekStart or SeekCurrent can be done after sort.Search. And sort.Search can use
>=. That'd be cheaper computationally speaking (which makes a difference here).
Done.
codec/codec.go, line 148 at r3 (raw file):
Previously, manishrjain (Manish R Jain) wrote…
Same here.
Done.
posting/list.go, line 106 at r2 (raw file):
Previously, manishrjain (Manish R Jain) wrote…
Can you add tests for some edge cases. Like if you put after to math.Maxuint64, etc. what happens?
Done.
posting/list.go, line 290 at r2 (raw file):
Previously, manishrjain (Manish R Jain) wrote…
Also, the right place to do this would be in iterate function. Because we might have the UID come in from a mutation as well. In which case, it would still get output.
Also, can you add which PR caused the breakage in the first place? So, the previous behavior (before v1.0.10) can be understood -- because it used to work before apparently.
The offending PR was #2719
Basically this has been broken since then.
There was a problem hiding this comment.
Reviewed 2 of 2 files at r4.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @manishrjain and @srfrog)
codec/codec.go, line 32 at r4 (raw file):
SeekStart = 0 // seek relative to the origin of the list (inclusive) SeekCurrent = 1 // seek relative to the current offset SeekEnd = 2 // seek relative to the end
If SeekEnd is not implemented can you remove it so that someone doesn't try to use it? Looks like right now because of the switch statements it will result in SeekStart bahavior.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @manishrjain and @srfrog)
codec/codec.go, line 134 at r4 (raw file):
default: // SeekStart f = func(i int) bool { return pack.Blocks[i].Base >= uid } }
Since when is a plain int, it could be any number. I think it would be better to check explicitly for only the allowed values and flag anything else as a logic error in the code. It seems wrong that a whence value of -1 starts seeking at the start of the list.
There was a problem hiding this comment.
Once you have approval from @codexnull , you can merge.
Reviewed 2 of 2 files at r4.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @srfrog)
SeekEnd is future functionality that we don't need right now.
There was a problem hiding this comment.
Reviewable status: 6 of 8 files reviewed, 2 unresolved discussions (waiting on @codexnull and @manishrjain)
codec/codec.go, line 32 at r4 (raw file):
Previously, codexnull (Javier Alvarado) wrote…
If SeekEnd is not implemented can you remove it so that someone doesn't try to use it? Looks like right now because of the switch statements it will result in SeekStart bahavior.
Done.
codec/codec.go, line 134 at r4 (raw file):
Previously, codexnull (Javier Alvarado) wrote…
Since
whenis a plain int, it could be any number. I think it would be better to check explicitly for only the allowed values and flag anything else as a logic error in the code. It seems wrong that a whence value of -1 starts seeking at the start of the list.
Done.
There was a problem hiding this comment.
Reviewed 2 of 2 files at r5.
Reviewable status:complete! all files reviewed, all discussions resolved
* codec/codec.go: add whence to Seek The whence value indicates how to use the uid offset with Seek. Whence can be SeekStart (0) or SeekCurrent (1). SeekStart will return the list including the offset. SeekCurrent returns values after the offset.
When using pagination, both at root and children, the pagination results include the uid specified in
afterclause. This change fixes the posting list iterator by skipping theafterUidfrom the unpacked uid list, to yield the correct result.Closes #3109
This change is