.Net: Pack of changes from MEVD API review#11882
.Net: Pack of changes from MEVD API review#11882roji merged 7 commits intomicrosoft:feature-vector-data-preb3from
Conversation
| /// Initializes a new instance of the <see cref="VectorStoreDataProperty"/> class by cloning the given source. | ||
| /// </summary> | ||
| /// <param name="source">The source to clone.</param> | ||
| public VectorStoreDataProperty(VectorStoreDataProperty source) |
There was a problem hiding this comment.
@westey-m please note this separate commit that also makes the record definition property types mutable.
We had this conversation previously in #10879, but since then we introduced the MEVD model, so the record definition is no longer referenced or used in any way after the model building step; that should make it safe to make it fully mutable, as it's only used during the construction of the collection (just like the option bags are only used during GetAsync, SearchAsync, etc).
Hopefully this all makes sense - if not let me know, I'm happy to pull this commit out of the PR and to have a separate discussion on it.
dotnet/src/Connectors/VectorData.Abstractions/RecordDefinition/VectorStoreRecordDefinition.cs
Outdated
Show resolved
Hide resolved
adamsitnik
left a comment
There was a problem hiding this comment.
I've reviewed each commit separately and overall it looks good to me. I've found few nits, feel free to address them before merging if you want to.
I know it's very subjective, but IMO 'VectorSearchOptions' was better name than RecordSearchOptions, but SimilaritySearchOptions would be even better.
Thank you @roji !
dotnet/src/Connectors/Connectors.AzureCosmosDBMongoDB.UnitTests/CosmosMongoCollectionTests.cs
Outdated
Show resolved
Hide resolved
dotnet/src/Connectors/Connectors.Memory.AzureCosmosDBMongoDB/CosmosMongoCollection.cs
Outdated
Show resolved
Hide resolved
dotnet/src/Connectors/VectorData.Abstractions/RecordOptions/FilteredRecordRetrievalOptions.cs
Show resolved
Hide resolved
dotnet/src/Connectors/Connectors.Memory.Redis/RedisHashSetCollectionOptions.cs
Show resolved
Hide resolved
| /// Gets or sets a value indicating whether the key should be auto-generated by the vector store. | ||
| /// </summary> | ||
| public bool AutoGenerate { get; init; } | ||
| public bool AutoGenerate { get; set; } |
There was a problem hiding this comment.
I've just realized that the property was exposed, but we don't ever respect it (yet).
There was a problem hiding this comment.
Yeah, true - good catch... Maybe safer to remove it for now, until we have at least one implementation which actually makes use of it - simply because we're not 100% sure yet what the feature will look like... Made a note, will do that in a separate PR later. Will remove this.
| /// </summary> | ||
| public IReadOnlyList<VectorStoreProperty> Properties { get; init; } = s_emptyFields; | ||
| [AllowNull] | ||
| public IList<VectorStoreProperty> Properties |
There was a problem hiding this comment.
I still believe that we should expose just the list rather than an ability to provide your own collection. Anyway, it could be just a List here as we don't really benefit from exposing an abstraction (in case of IReadOnlyList it could be an array)
There was a problem hiding this comment.
I mainly did IList<T> to align with MEAI - see this comment. I don't really have strong feelings on this - if the user happens to have e.g. an array, IList allows that to be assigned directly (whereas List does not). Any specific thoughts/arguments for exposing List instead of IList?
(will go ahead and merge to move forward, but let's continue the conversation and I can always change it later)
dotnet/src/Connectors/Connectors.Memory.AzureCosmosDBNoSQL/CosmosNoSqlCollection.cs
Outdated
Show resolved
Hide resolved
dotnet/src/Connectors/Connectors.Memory.AzureCosmosDBNoSQL/CosmosNoSqlCollection.cs
Outdated
Show resolved
Hide resolved
dotnet/src/VectorDataIntegrationTests/SqlServerIntegrationTests/SqlServerVectorStoreTests.cs
Show resolved
Hide resolved
IVectorSearch -> IVectorSearchable IKeywordHybridSearch -> IKeywordHybridSearchable
GetRecordOptions -> RecordRetrievalOptions GetFilteredRecordOptions -> FilteredRecordRetrievalOptions VectorSearchOptions -> RecordSearchOptions
Originally these were generic in order to support custom mappers, but these were removed.
This was proposed during API review in order to align the SearchAsync option bag with the GetAsync ones; on the GetAsync side we (now) have RecordRetrievalOptions and FilteredRecordRetrievalOptions, so on the SearchAsync side the proposal was to have RecordSearchOptions (the verb Search in MEVD implicitly means "similarity" or "vector" search). I can see where you're coming from though - VectorSearchOptions is possibly a little bit more clear. I'll go ahead and merge for now but I've taken a note to raise this again to see what people feel - we can always change it back. |
This especially makes sense now that we have a model that gets built based on the record definition (and attributes). Closes microsoft#10879
Note: this PR is based on top of #11879 - ignore first commit.This PR contains a pack of several unrelated changes from API review; each is quite simple on its own, so I submitted them as a single PR. I recommend reviewing this commit-by-commit.