Remove writing redundant binary doc value copy of jVector based vectors. - #715
Conversation
| // JVectorReader#getFloatVectorValues. Binary doc values copy is never read. | ||
| // Skipping it removes one raw fp32 copy per document giving indexing/merge time improvements. Do this | ||
| // for new indices after v3.9.0.0. | ||
| if (this.hasDocValues |
There was a problem hiding this comment.
@akash-shankaran I am a bit confused by this change:
- user has a choice to skip over doc values by setting
doc_valuestofalsein the mapping (any field, including vectors) - also, introducing this new setting (if
doc_valueswill not work for some reasons) would conflict with Added capability to retrieve float data type vectors using doc_values k-NN#3321
Signed-off-by: Akash Shankaran <akash.shankaran1@gmail.com>
| if (knnEngine != KNNEngine.JVECTOR) { | ||
| return true; | ||
| } | ||
| return indexCreatedVersion.before(Version.V_3_9_0); |
There was a problem hiding this comment.
hi @reta - based on our offline discussion, I think this is the cleanest way to do this code. If I have to update the config value directly ( for e.g. hasDocValues), that would require to move such logic up into the builder, which doesn't seem like the right place for it.
By the time such values are read in the mapper, they have already been set in the builder.
There was a problem hiding this comment.
@akash-shankaran I believe this is not the right way to deal with doc values: the field mapper is the right way to have a single knob where doc_values property has to be set, the KNNVectorFieldMapper should have something along these lines:
* Behavior matrix:
* - Index < 3.9.0: Uses original default value
* - Index >= 3.9.0, docValues not configured: Sets to false
* - Any version, docValues explicitly configured: Respects configured value
*/
if (indexCreatedVersion.before(Version.V_3_9_0)) {
hasDocValues = Parameter.docValuesParam(m -> toType(m).hasDocValues, true);
} else {
hasDocValues = Parameter.docValuesParam(m -> toType(m).hasDocValues, false);
}
The intent here is two fold:
- single source of truth:
doc_valuesin the mapping - single correct way to turn them on or off at any time:
doc_valuesin the mapping - safe default change: 3.9.0 -
false. anything below staystrue
2 participants
Description
Presently jvector writes 2 copies of vector files:
The .dvd copy is never read by jVector, as all its readers rely on being served from the inline vector graph.
This leads to higher storage and added overhead on index build, and merge times.
Given modifying doc values of existing indices could lead to issues, this change is safe for newer indices created in v3.9.0.0 onwards.
Advantages:
Experimental Data:
Measured on sift-100k (128d) PQ, 1 shard force-merged to 1 segment:
index size goes from 125.18 MB to 76.50 MB (-38.9%), with
data-jvectorfile being byte-identical and no impact on recall@k and recall@1. The removed ~50MB bytes is one fp32 copy (100000 x 128 x 4).Related Issues
Resolves #716
Check List
--signoff.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.