.Net MEVD: Strongly-typed vector property selector by adamsitnik · Pull Request #10793 · microsoft/semantic-kernel

Adam Sitnik (adamsitnik) · 2025-03-04T20:22:39Z

The property selector can return a property:

var results = await fixture.Collection.VectorizedSearchAsync(
    new ReadOnlyMemory<float>([1, 2, 3]),
    new() { VectorProperty = r => r.SomeVectorProp });

fixes #10420

…o vectorSearchSelector # Conflicts: # dotnet/src/Connectors/Connectors.AzureAISearch.UnitTests/AzureAISearchVectorStoreRecordCollectionTests.cs # dotnet/src/Connectors/Connectors.Weaviate.UnitTests/WeaviateVectorStoreRecordCollectionQueryBuilderTests.cs # dotnet/src/IntegrationTests/Connectors/Memory/AzureAISearch/AzureAISearchVectorStoreRecordCollectionTests.cs # dotnet/src/VectorDataIntegrationTests/VectorDataIntegrationTests/Filter/BasicFilterTests.cs

Adam Sitnik (adamsitnik) · 2025-03-04T21:02:28Z

Shay Rojansky (@roji) westey (@westey-m) Damn, it's late and it seems that one test is failing. I am going to fix it tomorrow and ask you for review again.

Shay Rojansky (roji)

Thanks Adam Sitnik (@adamsitnik), looks great.

Adam Sitnik (adamsitnik) · 2025-03-05T12:02:29Z

    {
        Verify.NotNull(vector);

-        if (this._propertyReader.FirstVectorPropertyName is null)


I've removed this and similar guards from other types as the methods exposed by PropertyReader will simply throw in such cases.

Yeah, I have similar changes in my Hybrid search PR as well, so we'll have to do some merging

Shay Rojansky (roji)

Thanks, LGTM!

westey (westey-m) · 2025-03-05T15:36:08Z

            {
                IncludeVectors = includeVectors,
-                VectorPropertyName = "DescriptionEmbedding",
+                VectorProperty = r => "DescriptionEmbedding",


Suggested change

VectorProperty = r => "DescriptionEmbedding",

VectorProperty = r => r.DescriptionEmbedding,

westey (westey-m) · 2025-03-05T15:36:16Z

            new()
            {
-                VectorPropertyName = "DescriptionEmbedding",
+                VectorProperty = r => "DescriptionEmbedding",


Suggested change

VectorProperty = r => "DescriptionEmbedding",

VectorProperty = r => r.DescriptionEmbedding,

I see a few other tests are also returning a string instead of selecting the prop

westey (westey-m) · 2025-03-05T15:39:46Z

+                    throw new InvalidOperationException($"The property {property.Name} of {typeof(TRecord).FullName} is not a Vector property.");
+                }
+
+                throw new InvalidOperationException("The expression must be a a valid Vector property access.");


Suggested change

throw new InvalidOperationException("The expression must be a a valid Vector property access.");

throw new InvalidOperationException("The expression must be a a valid Vector property accessor.");

westey (westey-m) · 2025-03-05T15:51:04Z

+#pragma warning restore CS0618 // Type or member is obsolete
+            if (!string.IsNullOrEmpty(obsoletePropertyName))
+            {
+                return obsoletePropertyName!;


We are missing some validation here, to ensure that the name is actually referring to a vector property.
In some connectors I see we are subsequently calling GetProperty, but it would only check that the property exists, not that it is a vector. Not checking upfront might lead to poor error messaging later if we just pass the name down to the SDK and let it throw.

In my branch I'm adding a propertyreader method that deals with this checking, so we could hold off until mine is merged and then use that here too.

westey (westey-m) · 2025-03-05T15:52:59Z

+#pragma warning restore CS0618 // Type or member is obsolete
+        };
+
+        Assert.Equal(ExpectedName, sut.GetVectorPropertyName(searchOptions));


This should actually fail in the same way that it would if using the property selection expression.

westey (westey-m) · 2025-03-05T15:54:18Z

+            }
+        }
+
+        return this._vectorProperties.Count > 0


We want to throw here if the user didn't specify a property and more than 1 exists. This is a change from the current behavior, but I'm adding it to my change doc.

westey (westey-m) · 2025-03-05T15:55:26Z

+    }
+
+    [Fact]
+    public void GetVectorPropertyNameReturnsFirstVectorPropertyWhenNoPropertySpecified()


This should actually fail, since first is not necessarily deterministic.

westey (westey-m) · 2025-03-05T15:59:23Z

+        if (!this._storagePropertyNames.TryGetValue(vectorDataModelPropertyName, out var vectorPropertyName))
        {
-            throw new InvalidOperationException("The collection does not have any vector properties, so vector search is not possible.");
+            throw new InvalidOperationException($"The collection does not have a vector field named '{vectorDataModelPropertyName}'.");


It would be nice to just verify this in GetVectorPropertyName.

westey (westey-m) · 2025-03-05T16:02:14Z

+        var vectorProperty = this._propertyReader.GetProperty<VectorStoreRecordVectorProperty>(this._propertyReader.GetVectorPropertyName(internalOptions));
+        var vectorPropertyName = vectorProperty.DataModelPropertyName;


this._propertyReader.GetVectorPropertyName(internalOptions) already returns DataModelPropertyName. However in GetVectorPropertyName we are typically looking through the list if Properties to get the right vector property.
Consider just returning the vector property from GetVectorPropertyName and then the caller can read its DataModelPropertyName or whatever other property is required.

…o vectorSearchSelector # Conflicts: # dotnet/src/Connectors/VectorData.Abstractions/VectorSearch/VectorSearchOptions.cs

Shay Rojansky (roji) · 2025-03-06T10:43:45Z

    /// </value>
    public string? VectorPropertyName { get; init; }

+    /// <summary>


Adam Sitnik (@adamsitnik) did I see correctly that you removed the [Obsolete] from VectorPropertyName just above? Don't we want to obsolete it?

Shay Rojansky (@roji) great catch! It must have been removed during the merge (xml docs were updated), I am going to add it back of course.

Adam Sitnik (adamsitnik) · 2025-03-07T14:15:27Z

Closing, I've sent a new PR for that that handles HybridSerarch as well: #10850

strongly-typed vector property selector

61bf088

Adam Sitnik (adamsitnik) requested review from Shay Rojansky (roji) and westey (westey-m) March 4, 2025 20:22

Adam Sitnik (adamsitnik) requested a review from a team as a code owner March 4, 2025 20:22

Mark Wallace (markwallace-microsoft) added .NET Issue or Pull requests regarding .NET code kernel Issues or pull requests impacting the core kernel memory labels Mar 4, 2025

Adam Sitnik (adamsitnik) had a problem deploying to integration March 4, 2025 20:27 — with GitHub Actions Failure

Adam Sitnik (adamsitnik) marked this pull request as draft March 4, 2025 21:02

Shay Rojansky (roji) requested changes Mar 5, 2025

View reviewed changes

Adam Sitnik (adamsitnik) had a problem deploying to integration March 5, 2025 11:59 — with GitHub Actions Error

Adam Sitnik (adamsitnik) had a problem deploying to integration March 5, 2025 12:01 — with GitHub Actions Failure

Adam Sitnik (adamsitnik) commented Mar 5, 2025

View reviewed changes

address the code review feedback and fix the build

bb801b5

Adam Sitnik (adamsitnik) force-pushed the vectorSearchSelector branch from 5de40f6 to bb801b5 Compare March 5, 2025 13:20

Adam Sitnik (adamsitnik) temporarily deployed to integration March 5, 2025 13:21 — with GitHub Actions Inactive

Adam Sitnik (adamsitnik) marked this pull request as ready for review March 5, 2025 13:40

Adam Sitnik (adamsitnik) requested a review from Shay Rojansky (roji) March 5, 2025 13:40

Shay Rojansky (roji) approved these changes Mar 5, 2025

View reviewed changes

westey (westey-m) reviewed Mar 5, 2025

View reviewed changes

Merge remote-tracking branch 'upstream/feature-vector-data-preb1' int…

3a236dd

…o vectorSearchSelector # Conflicts: # dotnet/src/Connectors/VectorData.Abstractions/VectorSearch/VectorSearchOptions.cs

Adam Sitnik (adamsitnik) had a problem deploying to integration March 6, 2025 10:13 — with GitHub Actions Failure

Shay Rojansky (roji) reviewed Mar 6, 2025

View reviewed changes

Adam Sitnik (adamsitnik) mentioned this pull request Mar 7, 2025

.Net MEVD: Strongly-typed property selectors #10850

Merged

Adam Sitnik (adamsitnik) closed this Mar 7, 2025

	VectorProperty = r => "DescriptionEmbedding",
	VectorProperty = r => r.DescriptionEmbedding,

	throw new InvalidOperationException("The expression must be a a valid Vector property access.");
	throw new InvalidOperationException("The expression must be a a valid Vector property accessor.");

		var vectorProperty = this._propertyReader.GetProperty<VectorStoreRecordVectorProperty>(this._propertyReader.GetVectorPropertyName(internalOptions));
		var vectorPropertyName = vectorProperty.DataModelPropertyName;

Conversation

Adam Sitnik (adamsitnik) commented Mar 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Adam Sitnik (adamsitnik) commented Mar 4, 2025

Uh oh!

Shay Rojansky (roji) left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Shay Rojansky (roji) left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

westey (westey-m) Mar 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

westey (westey-m) Mar 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Adam Sitnik (adamsitnik) commented Mar 7, 2025

Uh oh!

This branch had an error being deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Adam Sitnik (adamsitnik) commented Mar 4, 2025 •

edited

Loading

westey (westey-m) Mar 5, 2025 •

edited

Loading

westey (westey-m) Mar 5, 2025 •

edited

Loading