Skip to content

Commit

Permalink
GH-2165: Simplify VectorStore delete method to return void
Browse files Browse the repository at this point in the history
- Change VectorStore.delete() and related implementations to return void instead of Optional<Boolean>
- Remove unnecessary boolean return values and success status checks across all vector store implementations
- Clean up tests by removing redundant assertions
- Implementations continue using runtime exceptions for error signaling

Signed-off-by: Soby Chacko <[email protected]>
  • Loading branch information
sobychacko authored and ilayaperumalg committed Feb 6, 2025
1 parent b466159 commit 6035516
Show file tree
Hide file tree
Showing 26 changed files with 43 additions and 127 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -106,11 +106,10 @@ public void doAdd(List<Document> documents) {
}

@Override
public Optional<Boolean> doDelete(List<String> idList) {
public void doDelete(List<String> idList) {
for (String id : idList) {
this.store.remove(id);
}
return Optional.of(true);
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,10 +59,8 @@ default void accept(List<Document> documents) {
/**
* Deletes documents from the vector store.
* @param idList list of document ids for which documents will be removed.
* @return Returns true if the documents were successfully deleted.
*/
@Nullable
Optional<Boolean> delete(List<String> idList);
void delete(List<String> idList);

/**
* Deletes documents from the vector store based on filter criteria.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -87,14 +87,13 @@ public void add(List<Document> documents) {
}

@Override
@Nullable
public Optional<Boolean> delete(List<String> deleteDocIds) {
public void delete(List<String> deleteDocIds) {

VectorStoreObservationContext observationContext = this
.createObservationContextBuilder(VectorStoreObservationContext.Operation.DELETE.value())
.build();

return VectorStoreObservationDocumentation.AI_VECTOR_STORE
VectorStoreObservationDocumentation.AI_VECTOR_STORE
.observation(this.customObservationConvention, DEFAULT_OBSERVATION_CONVENTION, () -> observationContext,
this.observationRegistry)
.observe(() -> this.doDelete(deleteDocIds));
Expand Down Expand Up @@ -140,9 +139,8 @@ public List<Document> similaritySearch(SearchRequest request) {
/**
* Perform the actual delete operation.
* @param idList the list of document IDs to delete
* @return true if the documents were successfully deleted
*/
public abstract Optional<Boolean> doDelete(List<String> idList);
public abstract void doDelete(List<String> idList);

/**
* Template method for concrete implementations to provide filter-based deletion
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@

import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatThrownBy;
import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;
Expand Down Expand Up @@ -112,8 +113,8 @@ void shouldDeleteDocuments() {
@Test
void shouldHandleDeleteOfNonexistentDocument() {
this.vectorStore.delete(List.of("nonexistent-id"));
// Should not throw exception and return true
assertThat(this.vectorStore.delete(List.of("nonexistent-id")).get()).isTrue();
// Should not throw exception
assertDoesNotThrow(() -> this.vectorStore.delete(List.of("nonexistent-id")));
}

@Test
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -271,7 +271,7 @@ public void doAdd(List<Document> documents) {
}

@Override
public Optional<Boolean> doDelete(List<String> idList) {
public void doDelete(List<String> idList) {
try {
// Convert the list of IDs into bulk delete operations
List<CosmosItemOperation> itemOperations = idList.stream()
Expand All @@ -285,12 +285,9 @@ public Optional<Boolean> doDelete(List<String> idList) {
response.getResponse().getStatusCode()))
.doOnError(error -> logger.error("Error deleting document: {}", error.getMessage()))
.blockLast(); // This will block until all operations have finished

return Optional.of(true);
}
catch (Exception e) {
logger.error("Exception while deleting documents: {}", e.getMessage());
return Optional.of(false);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -188,31 +188,17 @@ public void doAdd(List<Document> documents) {
}

@Override
public Optional<Boolean> doDelete(List<String> documentIds) {
public void doDelete(List<String> documentIds) {

Assert.notNull(documentIds, "The document ID list should not be null.");
if (CollectionUtils.isEmpty(documentIds)) {
return Optional.of(true); // nothing to do;
}

final var searchDocumentIds = documentIds.stream().map(documentId -> {
SearchDocument searchDocument = new SearchDocument();
searchDocument.put(ID_FIELD_NAME, documentId);
return searchDocument;
}).toList();

var results = this.searchClient.deleteDocuments(searchDocumentIds);

boolean resSuccess = true;

for (IndexingResult result : results.getResults()) {
if (!result.isSucceeded()) {
resSuccess = false;
break;
}
}

return Optional.of(resSuccess);
this.searchClient.deleteDocuments(searchDocumentIds);
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -305,7 +305,7 @@ public void doAdd(List<Document> documents) {
}

@Override
public Optional<Boolean> doDelete(List<String> idList) {
public void doDelete(List<String> idList) {
CompletableFuture[] futures = new CompletableFuture[idList.size()];
int i = 0;
for (String id : idList) {
Expand All @@ -314,7 +314,6 @@ public Optional<Boolean> doDelete(List<String> idList) {
futures[i++] = this.session.executeAsync(s).toCompletableFuture();
}
CompletableFuture.allOf(futures).join();
return Optional.of(Boolean.TRUE);
}

@Override
Expand All @@ -339,13 +338,7 @@ protected void doDelete(Filter.Expression filterExpression) {
if (!matchingDocs.isEmpty()) {
// Then delete those documents by ID
List<String> idsToDelete = matchingDocs.stream().map(Document::getId).collect(Collectors.toList());

Optional<Boolean> result = delete(idsToDelete);

if (result.isPresent() && !result.get()) {
throw new IllegalStateException("Failed to delete some documents");
}

delete(idsToDelete);
logger.debug("Deleted {} documents matching filter expression", idsToDelete.size());
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -158,10 +158,9 @@ public void doAdd(@NonNull List<Document> documents) {
}

@Override
public Optional<Boolean> doDelete(@NonNull List<String> idList) {
public void doDelete(List<String> idList) {
Assert.notNull(idList, "Document id list must not be null");
int status = this.chromaApi.deleteEmbeddings(this.collectionId, new DeleteEmbeddingsRequest(idList));
return Optional.of(status == 200);
this.chromaApi.deleteEmbeddings(this.collectionId, new DeleteEmbeddingsRequest(idList));
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -88,8 +88,7 @@ public void addAndSearch() {
assertThat(resultDoc.getMetadata()).containsKeys("meta2", DocumentMetadata.DISTANCE.value());

// Remove all documents from the store
assertThat(vectorStore.delete(this.documents.stream().map(doc -> doc.getId()).toList()))
.isEqualTo(Optional.of(Boolean.TRUE));
vectorStore.delete(this.documents.stream().map(doc -> doc.getId()).toList());

List<Document> results2 = vectorStore
.similaritySearch(SearchRequest.builder().query("Great").topK(1).build());
Expand Down Expand Up @@ -118,7 +117,7 @@ public void simpleSearch() {
assertThat(resultDoc.getText()).isEqualTo("The sky is blue because of Rayleigh scattering.");

// Remove all documents from the store
assertThat(vectorStore.delete(List.of(document.getId()))).isEqualTo(Optional.of(Boolean.TRUE));
vectorStore.delete(List.of(document.getId()));

results = vectorStore.similaritySearch(SearchRequest.builder().query("Why is the sky blue?").build());
assertThat(results).hasSize(0);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -178,21 +178,15 @@ public void doAdd(final List<Document> documents) {
}

@Override
public Optional<Boolean> doDelete(final List<String> idList) {
public void doDelete(final List<String> idList) {
var chunkIds = idList.stream().map(this::toChunkId).toList();
Map<DocumentChunk.Id, Boolean> results = this.documentChunks.invokeAll(chunkIds, entry -> {
this.documentChunks.invokeAll(chunkIds, entry -> {
if (entry.isPresent()) {
entry.remove(false);
return true;
}
return false;
});
for (boolean r : results.values()) {
if (!r) {
return Optional.of(false);
}
}
return Optional.of(true);
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -209,7 +209,7 @@ public void doAdd(List<Document> documents) {
}

@Override
public Optional<Boolean> doDelete(List<String> idList) {
public void doDelete(List<String> idList) {
BulkRequest.Builder bulkRequestBuilder = new BulkRequest.Builder();
// For the index to be present, either it must be pre-created or set the
// initializeSchema to true.
Expand All @@ -219,7 +219,6 @@ public Optional<Boolean> doDelete(List<String> idList) {
for (String id : idList) {
bulkRequestBuilder.operations(op -> op.delete(idx -> idx.index(this.options.getIndexName()).id(id)));
}
return Optional.of(bulkRequest(bulkRequestBuilder.build()).errors());
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -231,7 +231,7 @@ public void doAdd(List<Document> documents) {
}

@Override
public Optional<Boolean> doDelete(List<String> idList) {
public void doDelete(List<String> idList) {
try {
this.client.method(HttpMethod.DELETE)
.uri("/" + this.indexName + EMBEDDINGS)
Expand All @@ -242,9 +242,7 @@ public Optional<Boolean> doDelete(List<String> idList) {
}
catch (Exception e) {
logger.warn("Error removing embedding: {}", e.getMessage(), e);
return Optional.of(false);
}
return Optional.of(true);
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -126,10 +126,9 @@ public void doAdd(List<Document> documents) {
}

@Override
public Optional<Boolean> doDelete(List<String> idList) {
public void doDelete(List<String> idList) {
int deleteCount = this.repository.deleteEmbeddingsById(this.tableName, idList);
logger.info("{} embeddings deleted", deleteCount);
return Optional.of(deleteCount == idList.size());
}

public int purgeEmbeddings() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -318,15 +318,13 @@ private String toJson(Map<String, Object> map) {
}

@Override
public Optional<Boolean> doDelete(List<String> idList) {
public void doDelete(List<String> idList) {
int updateCount = 0;
for (String id : idList) {
int count = this.jdbcTemplate.update(
String.format("DELETE FROM %s WHERE %s = ?", getFullyQualifiedTableName(), this.idFieldName), id);
updateCount = updateCount + count;
}

return Optional.of(updateCount == idList.size());
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -274,7 +274,7 @@ public void doAdd(List<Document> documents) {
}

@Override
public Optional<Boolean> doDelete(List<String> idList) {
public void doDelete(List<String> idList) {
Assert.notNull(idList, "Document id list must not be null");

String deleteExpression = String.format("%s in [%s]", this.idFieldName,
Expand All @@ -290,8 +290,6 @@ public Optional<Boolean> doDelete(List<String> idList) {
if (deleteCount != idList.size()) {
logger.warn(String.format("Deleted only %s entries from requested %s ", deleteCount, idList.size()));
}

return Optional.of(status.getStatus() == Status.Success.getCode());
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -268,13 +268,9 @@ public void doAdd(List<Document> documents) {
}

@Override
public Optional<Boolean> doDelete(List<String> idList) {
public void doDelete(List<String> idList) {
Query query = new Query(org.springframework.data.mongodb.core.query.Criteria.where(ID_FIELD_NAME).in(idList));

var deleteRes = this.mongoTemplate.remove(query, this.collectionName);
long deleteCount = deleteRes.getDeletedCount();

return Optional.of(deleteCount == idList.size());
this.mongoTemplate.remove(query, this.collectionName);
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -221,20 +221,19 @@ public void doAdd(List<Document> documents) {
}

@Override
public Optional<Boolean> doDelete(List<String> idList) {
public void doDelete(List<String> idList) {

try (var session = this.driver.session(this.sessionConfig)) {

// Those queries with internal, cypher based transaction management cannot be
// run with executeWrite
var summary = session
session
.run("""
MATCH (n:%s) WHERE n.%s IN $ids
CALL { WITH n DETACH DELETE n } IN TRANSACTIONS OF $transactionSize ROWS
""".formatted(this.label, this.idProperty),
Map.of("ids", idList, "transactionSize", DEFAULT_TRANSACTION_SIZE))
.consume();
return Optional.of(idList.size() == summary.counters().nodesDeleted());
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -217,12 +217,11 @@ public void doAdd(List<Document> documents) {
}

@Override
public Optional<Boolean> doDelete(List<String> idList) {
public void doDelete(List<String> idList) {
BulkRequest.Builder bulkRequestBuilder = new BulkRequest.Builder();
for (String id : idList) {
bulkRequestBuilder.operations(op -> op.delete(idx -> idx.index(this.index).id(id)));
}
return Optional.of(bulkRequest(bulkRequestBuilder.build()).errors());
}

private BulkResponse bulkRequest(BulkRequest bulkRequest) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -286,7 +286,7 @@ private double[] normalize(final double[] v) {
}

@Override
public Optional<Boolean> doDelete(final List<String> idList) {
public void doDelete(final List<String> idList) {
final String sql = String.format("delete from %s where id=?", this.tableName);
final int[] argTypes = { Types.VARCHAR };

Expand All @@ -297,19 +297,15 @@ public Optional<Boolean> doDelete(final List<String> idList) {

final int[] deleteCounts = this.jdbcTemplate.batchUpdate(sql, batchArgs, argTypes);

int deleteCount = 0;
for (int detailedResult : deleteCounts) {
switch (detailedResult) {
case Statement.EXECUTE_FAILED:
break;
case 1:
case Statement.SUCCESS_NO_INFO:
deleteCount++;
break;
}
}

return Optional.of(deleteCount == idList.size());
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -318,15 +318,13 @@ private Object convertIdToPgType(String id) {
}

@Override
public Optional<Boolean> doDelete(List<String> idList) {
public void doDelete(List<String> idList) {
int updateCount = 0;
for (String id : idList) {
int count = this.jdbcTemplate.update("DELETE FROM " + getFullyQualifiedTableName() + " WHERE id = ?",
UUID.fromString(id));
updateCount = updateCount + count;
}

return Optional.of(updateCount == idList.size());
}

@Override
Expand Down
Loading

0 comments on commit 6035516

Please sign in to comment.