-
-
Notifications
You must be signed in to change notification settings - Fork 3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Change to readonly collection and cleanup #75
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -167,7 +167,7 @@ private static IDictionary<string, object> DeserializeMetadata(MemoryRecordMetad | |||||||||||||||||||||||||||
?? new Dictionary<string, object>(); | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
public Task<List<Vector>> SearchByMetadata(Dictionary<string, object> filters, CancellationToken cancellationToken) | ||||||||||||||||||||||||||||
public Task<IReadOnlyList<Vector>> SearchByMetadata(Dictionary<string, object> filters, CancellationToken cancellationToken) | ||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||
throw new NotSupportedException("Chroma doesn't support collection metadata"); | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
Comment on lines
+170
to
173
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Consider adding documentation and removing async overhead. The signature change to + /// <summary>
+ /// Not supported in ChromaDB as it doesn't support collection metadata.
+ /// </summary>
+ /// <exception cref="NotSupportedException">Always thrown as this operation is not supported.</exception>
- public Task<IReadOnlyList<Vector>> SearchByMetadata(Dictionary<string, object> filters, CancellationToken cancellationToken)
+ public Task<IReadOnlyList<Vector>> SearchByMetadata(Dictionary<string, object> filters, CancellationToken cancellationToken = default)
{
- throw new NotSupportedException("Chroma doesn't support collection metadata");
+ return Task.FromException<IReadOnlyList<Vector>>(
+ new NotSupportedException("Chroma doesn't support collection metadata"));
} This change:
📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -13,31 +13,36 @@ public class MongoVectorCollection( | |||||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||||||
private readonly IMongoCollection<Vector> _mongoCollection = mongoContext.GetCollection<Vector>(name); | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
/// <inheritdoc /> | ||||||||||||||||||||||||||||||||
public async Task<IReadOnlyCollection<string>> AddAsync(IReadOnlyCollection<Vector> items, CancellationToken cancellationToken = default) | ||||||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||||||
await _mongoCollection.InsertManyAsync(items, cancellationToken: cancellationToken).ConfigureAwait(false); | ||||||||||||||||||||||||||||||||
return items.Select(i => i.Id).ToList(); | ||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
Comment on lines
+16
to
22
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Consider returning a read-only collection to enforce immutability Although the method returns Apply this change: return items.Select(i => i.Id).ToList()
+ .AsReadOnly(); 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||
/// <inheritdoc /> | ||||||||||||||||||||||||||||||||
public async Task<bool> DeleteAsync(IEnumerable<string> ids, CancellationToken cancellationToken = default) | ||||||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||||||
var filter = Builders<Vector>.Filter.In(i => i.Id, ids); | ||||||||||||||||||||||||||||||||
var result = await _mongoCollection.DeleteManyAsync(filter, cancellationToken).ConfigureAwait(false); | ||||||||||||||||||||||||||||||||
return result.IsAcknowledged; | ||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||
Comment on lines
+23
to
29
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add a null check for Currently, the method does not check if Apply this change: + if (ids == null) throw new ArgumentNullException(nameof(ids));
var filter = Builders<Vector>.Filter.In(i => i.Id, ids); 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
/// <inheritdoc /> | ||||||||||||||||||||||||||||||||
public async Task<Vector?> GetAsync(string id, CancellationToken cancellationToken = default) | ||||||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||||||
var filter = Builders<Vector>.Filter.Eq(i => i.Id, id); | ||||||||||||||||||||||||||||||||
var result = await _mongoCollection.FindAsync(filter, cancellationToken: cancellationToken).ConfigureAwait(false); | ||||||||||||||||||||||||||||||||
return result.FirstOrDefault(cancellationToken: cancellationToken); | ||||||||||||||||||||||||||||||||
Comment on lines
+31
to
36
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add a null check for The method does not check if Apply this change: + if (id == null) throw new ArgumentNullException(nameof(id));
var filter = Builders<Vector>.Filter.Eq(i => i.Id, id); 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
/// <inheritdoc /> | ||||||||||||||||||||||||||||||||
public async Task<bool> IsEmptyAsync(CancellationToken cancellationToken = default) | ||||||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||||||
return await _mongoCollection.EstimatedDocumentCountAsync(cancellationToken: cancellationToken).ConfigureAwait(false) == 0; | ||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||
Comment on lines
+39
to
43
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use
Apply this change: - return await _mongoCollection.EstimatedDocumentCountAsync(cancellationToken: cancellationToken).ConfigureAwait(false) == 0;
+ return await _mongoCollection.CountDocumentsAsync(FilterDefinition<Vector>.Empty, cancellationToken: cancellationToken).ConfigureAwait(false) == 0; 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
/// <inheritdoc /> | ||||||||||||||||||||||||||||||||
public async Task<VectorSearchResponse> SearchAsync(VectorSearchRequest request, VectorSearchSettings? settings = null, CancellationToken cancellationToken = default) | ||||||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||||||
request = request ?? throw new ArgumentNullException(nameof(request)); | ||||||||||||||||||||||||||||||||
|
@@ -71,7 +76,8 @@ public async Task<VectorSearchResponse> SearchAsync(VectorSearchRequest request, | |||||||||||||||||||||||||||||||
}; | ||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
public async Task<List<Vector>> SearchByMetadata(Dictionary<string, object> filters, CancellationToken cancellationToken = default) | ||||||||||||||||||||||||||||||||
/// <inheritdoc /> | ||||||||||||||||||||||||||||||||
public async Task<IReadOnlyList<Vector>> SearchByMetadata(Dictionary<string, object> filters, CancellationToken cancellationToken = default) | ||||||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||||||
filters = filters ?? throw new ArgumentNullException(nameof(filters)); | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -134,7 +134,7 @@ public Task<bool> IsEmptyAsync(CancellationToken cancellationToken = default) | |
throw new NotImplementedException(); | ||
} | ||
|
||
Task<List<Vector>> IVectorCollection.SearchByMetadata(Dictionary<string, object> filters, CancellationToken cancellationToken) | ||
Task<IReadOnlyList<Vector>> IVectorCollection.SearchByMetadata(Dictionary<string, object> filters, CancellationToken cancellationToken) | ||
{ | ||
throw new NotImplementedException(); | ||
} | ||
Comment on lines
+137
to
140
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Implementation needed for While the method signature has been updated to match the interface changes, throwing Consider implementing the method using OpenSearch's query capabilities. Here's a suggested implementation structure: - Task<IReadOnlyList<Vector>> IVectorCollection.SearchByMetadata(Dictionary<string, object> filters, CancellationToken cancellationToken)
- {
- throw new NotImplementedException();
- }
+ async Task<IReadOnlyList<Vector>> IVectorCollection.SearchByMetadata(Dictionary<string, object> filters, CancellationToken cancellationToken)
+ {
+ var response = await client.SearchAsync<VectorRecord>(s => s
+ .Index(Name)
+ .Query(q => BuildMetadataQuery(q, filters)), cancellationToken)
+ .ConfigureAwait(false);
+
+ if (!response.IsValid)
+ {
+ throw new InvalidOperationException($"Failed to search collection '{Name}' by metadata. DebugInformation: {response.DebugInformation}");
+ }
+
+ return response.Documents
+ .Where(vectorRecord => !string.IsNullOrWhiteSpace(vectorRecord.Text))
+ .Select(vectorRecord => new Vector
+ {
+ Text = vectorRecord.Text ?? string.Empty,
+ Id = vectorRecord.Id,
+ Metadata = vectorRecord.Metadata?.ToDictionary(x => x.Key, x => x.Value),
+ Embedding = vectorRecord.Vector,
+ })
+ .ToList()
+ .AsReadOnly();
+ }
+
+ private static QueryContainer BuildMetadataQuery(QueryContainerDescriptor<VectorRecord> q, Dictionary<string, object> filters)
+ {
+ // Build a bool query combining all metadata filters
+ return q.Bool(b => b.Must(
+ filters.Select(kvp =>
+ q.Term(t => t.Field($"metadata.{kvp.Key}").Value(kvp.Value))
+ )
+ ));
+ }
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Consider enhancing the JSON key validation.
While the implementation is correct, consider these improvements for robustness:
Here's a suggested implementation:
📝 Committable suggestion