[Enhancement] Optimize Merge On KNN Field when soft_delete mode - #18050
[Enhancement] Optimize Merge On KNN Field when soft_delete mode#18050luyuncheng wants to merge 1 commit into
Conversation
Signed-off-by: luyuncheng <luyuncheng@bytedance.com>
|
@jmazanec15 @navneet1v i create a pr at OpenSearch repo for k-NN enhancement, because plugin can not handle merge controls, also would you please take a look about this enhancement? |
|
❌ Gradle check result for d9b585c: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
bugmakerrrrrr
left a comment
bugmakerrrrrr
left a comment
There was a problem hiding this comment.
@luyuncheng Good idea! I left some comments, also I was wondering if this would affect recovery if I enable derived source.
| @Override | ||
| public BinaryDocValues getBinary(FieldInfo fieldInfo) throws IOException { | ||
| BinaryDocValues binaryDocValues = docValuesProducer.getBinary(fieldInfo); | ||
| if (binaryDocValues != null && fieldInfo.attributes().containsKey(KNN_FIELD)) { |
There was a problem hiding this comment.
I don't know if we should expose something of the plugin within the core. Maybe we can provide an extension point in the engine plugin.
There was a problem hiding this comment.
@luyuncheng I think we should just make this generic. Why we want to make this specific to k-NN. Docs containing BDV should just be removed if they are deleted. What is the point of having the deleted docs?
There was a problem hiding this comment.
I think we should just make this generic. Why we want to make this specific to k-NN. Docs containing BDV should just be removed if they are deleted. What is the point of having the deleted docs?
Good Idea. i think all BinaryDocValues should skipped on merge with soft_deletes.
| } | ||
|
|
||
| @Override | ||
| public KnnVectorsReader getVectorReader() { |
There was a problem hiding this comment.
why don't we pruning the vector field?
There was a problem hiding this comment.
i need to override the KnnVectorsReader with different type like 'FloatVectorValues' and 'ByteVectorValues' and handle the iterator with different type and merge logic. so i think it would be better to create new PR and handle KnnVectorsReader carefully
Sure I will take a look. |
|
@luyuncheng can you share some details on the soft_delete mode, since this is something I have not done any deep-dive on. Getting that understanding will help review the PR in better fashion. |
@navneet1v softdelete would record all changes for the docs, and write it into docValues. it changes to defaults mode at OpenSearch #1933 and at #2077. |
|
@luyuncheng will take a look - sorry I missed this one |
|
Overall, this PR seems great to me! Thanks @luyuncheng! This looks like it would provide a clear benefit on merge. @luyuncheng Do you know where in OpenSearch code base soft deletes are accessed to provide document history/delete recovery? One potential implication of making this change would be that features like derived source would not work to retrieve these vector values if they are not indexed into DocValues/VectorValues. |
|
This PR is stalled because it has been open for 30 days with no activity. |
|
This PR is stalled because it has been open for 30 days with no activity. |
4 participants
Description
[Describe what this change achieves]
when #1933 and #2077, OpenSearch always use soft deletes. but it would bring some performance drops.
OpenSearch K-NN would use
KNN80BinaryDocValueswith native engine andKNNVectorValueswith lucene engine.I found some scenarios as followings:
in some update scenarios, like index 10000 docs, and update/delete 60% docs, in one segment.
and then happens merge tasks. nativeEngine would merge all docs including deleted docs because of soft_delete mode.
easy tests like
OpenSearch/server/src/test/java/org/opensearch/index/engine/PrunePostingsAndKnnMergePolicyTests.java
Lines 253 to 258 in d9b585c
but we do not want to do heavy construct HNSW graph which is a CPU consuming tasks. and merge task take a very long time to index useless docs.
so i think we can skip this soft_delete docs at merge and it can optimize merge and index time obviously 30% at out benchmarks
Related Issues
Resolves #[Issue number to be closed when this PR is merged]
Check List
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.