Fix mergedbytevectorvalues lastord#15553
Conversation
|
Tagging few folks. @mikemccand, @kaivalnp and @vigyasharma to see if they can review the PR. |
kaivalnp
left a comment
kaivalnp
left a comment
There was a problem hiding this comment.
The fix LGTM (makes logic same as MergedFloat32VectorValues), one small comment about testing though
| * <p>The bug: MergedByteVectorValues.nextDoc() does not increment lastOrd, so when you skip N | ||
| * vectors and then try to load vectorValue(N), it fails because lastOrd is still -1. | ||
| */ | ||
| public void testSkipThenLoadByteVectorDuringMerge() throws IOException { |
There was a problem hiding this comment.
The setup of this test seems complicated to me -- considering the gist is:
// Run the test
ByteVectorValues values =
KnnVectorsWriter.MergedVectorValues.mergeByteVectorValues(
fieldInfo, mergeState);
// Skip doc 0 and load doc 1
values.iterator().nextDoc(); // doc 0
values.iterator().nextDoc(); // doc 1
// Read vector for doc 1
assertArrayEquals(expected, values.vectorValue(1)); // throws IllegalArgumentException on mainIt's probably because ByteVectorValuesSub and MergedByteVectorValues are private, so we have to go through public APIs to instantiate them + execute the test?
Perhaps we can make them package-private for testing? It would simplify the test to something like:
/**
* Test that skipping vectors in MergedByteVectorValues via nextDoc() and then loading a
* subsequent vector via vectorValue() works correctly.
*/
public void testSkipsInMergedByteVectorValues() throws IOException {
// Data
List<byte[]> vectors = List.of(new byte[] {0}, new byte[] {1});
// Setup
KnnVectorsWriter.ByteVectorValuesSub sub =
new KnnVectorsWriter.ByteVectorValuesSub(x -> x, ByteVectorValues.fromBytes(vectors, 1));
MergeState state =
new MergeState(
null, null, null, null, null, null, null, null, null, null, null, null, null, null,
null, false);
// Run the test
ByteVectorValues values =
new KnnVectorsWriter.MergedVectorValues.MergedByteVectorValues(List.of(sub), state);
// Skip doc 0 and load doc 1
values.iterator().nextDoc(); // doc 0
values.iterator().nextDoc(); // doc 1
// Read vector for doc 1
assertArrayEquals(vectors.get(1), values.vectorValue(1));
}Although I'll wait for someone else to weigh in on this..
|
This PR has not had activity in the past 2 weeks, labeling it as stale. If the PR is waiting for review, notify the dev@lucene.apache.org list. Thank you for your contribution! |
finnroblin
force-pushed
the
fix-mergedbytevectorvalues-lastord
branch
from
cabb630 to
f62ff8a
Compare
kaivalnp
left a comment
kaivalnp
left a comment
There was a problem hiding this comment.
LGTM, thanks!
This bug only affects users doing something specific during merging that skips a vector? But it's nice to fix nevertheless..
Can you also add a CHANGES.txt entry?
| } | ||
|
|
||
| private static class ByteVectorValuesSub extends DocIDMerger.Sub { | ||
| static class ByteVectorValuesSub extends DocIDMerger.Sub { |
There was a problem hiding this comment.
Can we add some Javadocs with the @lucene.internal tag to avoid users from depending on this class, now that its more accessible?
| ByteVectorValuesSub current; | ||
|
|
||
| private MergedByteVectorValues(List<ByteVectorValuesSub> subs, MergeState mergeState) | ||
| MergedByteVectorValues(List<ByteVectorValuesSub> subs, MergeState mergeState) |
There was a problem hiding this comment.
Can you add a comment specifying that this is package-private for testing?
| * Test that skipping vectors in MergedByteVectorValues via nextDoc() and then loading a | ||
| * subsequent vector via vectorValue() works correctly. | ||
| */ | ||
| public void testSkipsInMergedByteVectorValues() throws IOException { |
There was a problem hiding this comment.
Should we include an equivalent test for the float vector values? (making the class more generic like TestMergedVectorValues)
github-actions
bot
added
module:core/store
module:analysis
module:core/index
module:demo
module:core/search
module:highlighter
module:benchmark
module:suggest
module:spatial
module:luke
module:facet
module:grouping
module:core/FSTs
module:queryparser
module:join
module:test-framework
module:expressions
module:sandbox
module:spatial3d
labels
Signed-off-by: Finn Roblin <finnrobl@amazon.com>
|
Thanks @kaivalnp ! I've added
Yes, the bugfix will allow OpenSearch k-NN to partition different parts of the vectorvalues during merging so we can upload the vectors in parallel for a GPU acceleration use case (more detail here). I suppose it will also affect any user who uses |
kaivalnp
left a comment
kaivalnp
left a comment
There was a problem hiding this comment.
LGTM!
Thanks @finnroblin, the build was failing so I pushed a small commit to tidy, hope you don't mind :)
Also tagging @benwtrent who's cutting the 10.4 branch soon (tomorrow!) -- this bug fix seems small + good to include? (please feel free to merge if you agree)
|
@benwtrent sorry I'm a bit unfamiliar with what can and cannot be merged in the freeze -- do you mean we can put this bug-fix in 10.4? (i.e. first commit to Or does it have to go into 10.5 now? |
|
Freeze means only justified bug fixes Since we are blocked, I think this one can go in, but please backport to 10.4 branch as soon as possible. I hope to start back up the releases process soon |
Move internal ordinal tracking in `MergedByteVectorValues` from `vectorValue` -> `nextDoc` to allow loading only a subset of vectors during iteration.
Move internal ordinal tracking in `MergedByteVectorValues` from `vectorValue` -> `nextDoc` to allow loading only a subset of vectors during iteration.
|
Thanks @benwtrent -- I merged, backported to Edit: please let me know if the PR was unnecessary because we are blocked, and if I should push it instead. |
|
Thanks @kaivalnp @benwtrent @navneet1v ! |
Move internal ordinal tracking in `MergedByteVectorValues` from `vectorValue` -> `nextDoc` to allow loading only a subset of vectors during iteration.
4 participants
Description
Fixes MergedByteVectorValues behavior as described in #14992. Currently in
MergedByteVectorValues::nextDoc()thelastOrdvalue is not incremented when the iterator is advanced. IfnextDocis called several times and a vector is loaded then the bad state oflastOrdcauses an exception. One case where this occurs is In OpenSearch k-NN where we split a list of vectors into multiple parts to upload the partitions in parallel. (Please see opensearch-project/k-NN#2803 for more details about the use case this bugfix solves).This PR includes a unit test that fails without the bugfix.
Thanks @0ctopus13prime for the original RFC and bugfix!
Failed test output, pre-bug fix: (use the first commit in this PR to replicate. The second commit contains bugfix.)