Skip to content

support deletes for leading segment merges#203

Merged
sam-herman merged 6 commits into
opensearch-project:mainfrom
sam-herman:add-in-place-deletes
Nov 20, 2025
Merged

support deletes for leading segment merges#203
sam-herman merged 6 commits into
opensearch-project:mainfrom
sam-herman:add-in-place-deletes

Conversation

@sam-herman

@sam-herman sam-herman commented Nov 7, 2025

Copy link
Copy Markdown
Collaborator

Description

For leading segment merges #133 we made delete an exception that will force full reconstruction of the graph.
This change removes that exception and allow for deletes to also happen in-place on the leading segment, even if those deletes happen on the leading segment itself.

This allows for the results previously shown for the incremental insertion into existing graph change #167 to also shown during deletes.

Benchmarks

Latest delete benchmark using the delete script shows various deletion vs recall scenarios and confirms that jVector behaves in a FreshDiskANN manner.

image

Related Issues

Resolves #171

Check List

  • New functionality includes testing.
  • New functionality has been documented.
  • API changes companion pull request created.
  • [ x] Commits are signed per the DCO using --signoff.
  • Public documentation issue/PR created.

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.

Signed-off-by: Samuel Herman <sherman8915@gmail.com>
Comment thread gradle.properties Outdated
Comment thread src/main/java/org/opensearch/knn/index/codec/jvector/GraphNodeIdToDocMap.java Outdated
@akash-shankaran

Copy link
Copy Markdown
Collaborator

Is it worth putting this feature behind a feature flag config or experimental tag?
To avoid any regression that may have slipped through.

Signed-off-by: Samuel Herman <sherman8915@gmail.com>
Signed-off-by: Samuel Herman <sherman8915@gmail.com>
@sam-herman

Copy link
Copy Markdown
Collaborator Author

Is it worth putting this feature behind a feature flag config or experimental tag? To avoid any regression that may have slipped through.

Right now this will only kick in if we modify the minimum batch for quantization parameter. If we keep it as is, should be no difference and this functionality of leading segment merges will not be triggered.

@sam-herman

sam-herman commented Nov 20, 2025

Copy link
Copy Markdown
Collaborator Author

I see test failure of timeout, but it doesn't seem to be related to the change as it's not even touching the merge code yet:

InternalKNNEngineTests > testQuantizationWithOverQueryParameter FAILED
    java.net.SocketTimeoutException: 30000 MILLISECONDS
        at org.opensearch.client.RestClient.extractAndWrapCause(RestClient.java:1326)
        at org.opensearch.client.RestClient.performRequest(RestClient.java:371)
        at org.opensearch.client.RestClient.performRequest(RestClient.java:374)
        at org.opensearch.client.RestClient.performRequest(RestClient.java:374)
        at org.opensearch.client.RestClient.performRequest(RestClient.java:374)
        at org.opensearch.client.RestClient.performRequest(RestClient.java:359)
        at org.opensearch.knn.index.engine.CommonTestUtils.bulkAddKnnDocs(CommonTestUtils.java:314)
        at org.opensearch.knn.index.engine.CommonTestUtils.bulkAddKnnDocs(CommonTestUtils.java:280)
        at org.opensearch.knn.index.engine.CommonTestUtils.bulkAddKnnDocs(CommonTestUtils.java:267)
        at org.opensearch.knn.index.engine.InternalKNNEngineTests.testQuantizationWithOverQueryParameter(InternalKNNEngineTests.java:530)

This seems to indicate that the in-memory cluster is super slow and timing out when adding new documents to the index for some reason.
it does pass locally, and seem to pass fine in some environments but sporadically fail on others.

I would recommend that we make changes for this test to share in-memory cluster across tests (switch to @suite mode) and hopefully it will be more stable on GH. Or alternatively see if the next version ugprade fixes it, as it seems to have been introduced after latest upgrade.

what do you think? @akash-shankaran

@akash-shankaran

akash-shankaran commented Nov 20, 2025

Copy link
Copy Markdown
Collaborator

I would recommend that we make changes for this test to share in-memory cluster across tests (switch to @suite mode) and hopefully it will be more stable on GH. Or alternatively see if the next version ugprade fixes it, as it seems to have been introduced after latest upgrade.

what do you think? @akash-shankaran

I agree. This failure is unrelated to your change, lets leave it for now and tackle it after the 3.3 upgrade. This test has also failed (only on GH servers) during the 3.3 upgrade PR. Failure from the upgrade PR (same test) for reference:
https://github.com/opensearch-project/opensearch-jvector/actions/runs/19356571505/job/55378896279?pr=201

I'll create a GH issue to track this, if the next upgrade doesn't fix it. .

Signed-off-by: Samuel Herman <sherman8915@gmail.com>
sam-herman and others added 2 commits November 20, 2025 12:25
Signed-off-by: sam-herman <97131656+sam-herman@users.noreply.github.com>
Signed-off-by: Samuel Herman <sherman8915@gmail.com>
Comment thread src/main/java/org/opensearch/knn/index/codec/jvector/JVectorWriter.java Outdated
@sam-herman
sam-herman merged commit 433ccd3 into opensearch-project:main Nov 20, 2025
10 of 36 checks passed
@sam-herman
sam-herman deleted the add-in-place-deletes branch November 20, 2025 21:13
opensearch-trigger-bot Bot pushed a commit that referenced this pull request Nov 20, 2025
* support deletes for leading segment merges

---------

Signed-off-by: Samuel Herman <sherman8915@gmail.com>
Signed-off-by: sam-herman <97131656+sam-herman@users.noreply.github.com>
(cherry picked from commit 433ccd3)
sam-herman added a commit that referenced this pull request Nov 20, 2025
* support deletes for leading segment merges

---------



(cherry picked from commit 433ccd3)

Signed-off-by: Samuel Herman <sherman8915@gmail.com>
Signed-off-by: sam-herman <97131656+sam-herman@users.noreply.github.com>
Co-authored-by: sam-herman <97131656+sam-herman@users.noreply.github.com>
Comment thread CHANGELOG.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[FEATURE] In-Place Deletes

2 participants