.Net: IVectorStore implementation for Azure SQL #10623

adamsitnik · 2025-02-20T15:54:57Z

The PR is ready for review.

fixes #10416

This reverts commit 186fda2.

…o sqlServerTests # Conflicts: # dotnet/SK-dotnet.sln

…t throws NotImplementedException

…ngs that need to be addressed

…o sqlServerNew

The ToListAsync change is just a wokaround dotnet/runtime#79782 (comment)

dotnet/src/Connectors/Connectors.Memory.Common/SqlFilterTranslator.cs

dotnet/src/InternalUtilities/src/Data/VectorStoreRecordPropertyReader.cs

dotnet/src/Connectors/Connectors.Memory.SqlServer/SqlServerCommandBuilder.cs

westey-m · 2025-02-27T09:13:56Z

...egrationTests/SqlServerIntegrationTests/VectorSearch/SqlServerBasicVectorSearchTests_Hnsw.cs

+
+namespace SqlServerIntegrationTests.VectorSearch;
+
+public class SqlServerBasicVectorSearchTests_Hnsw(SqlServerFixture fixture)


Do we still need this now with Hsnw being unsupported after all?

I just wanted to make sure it's actually covered with tests (so we keep throwing for non-supported filter names)

Wouldn't they be covered with the other SqlServerBasicVectorSearchTests implementation anyway, i.e. the one using FLAT. Although I assume both are using FLAT since HNSW isn't supported.

The SqlServerVectorSearchDistanceFunctionComplianceTests file does not set the index name, so it's using the default.

The _Hnsw does provide the index name and the tests ensure that we throw NotSupportedException on purpose.

dotnet/src/Connectors/VectorData.Abstractions/RecordDefinition/VectorStoreRecordKeyProperty.cs

westey-m · 2025-02-27T09:36:55Z

dotnet/src/VectorDataIntegrationTests/VectorDataIntegrationTests/CRUD/BasicConformanceTests.cs

+    {
+        await this.ExecuteAsync(async collection =>
+        {
+            Assert.Empty(await collection.UpsertBatchAsync([]).ToArrayAsync());


Apologies, during our discussion yesterday I misunderstood when you asked whether we should throw if we are upsetting with an empty collection. I thought you were referring to the storage collection, not the collection being passed into the method. Hence my confusion as to why this would be a problem 😄

That said, I don't have a strong opinion about whether to throw in this case. I tend to lean towards throwing, since it could help the user find bugs, but it's not a strong opinion.

@roji we have agreed offline to keep it as is (don't throw) and wait until you are back since you most likely have designed a similar API in the past and have opinion about it

dotnet/src/VectorDataIntegrationTests/VectorDataIntegrationTests/CRUD/BasicConformanceTests.cs

dotnet/src/Connectors/Connectors.Memory.SqlServer/SqlServerVectorStoreRecordCollection.cs

- add support for VectorStoreGenericDataModel - respect IncludeVectors in Get* methods - rename BasicVectorSearchTests to VectorSearchDistanceFunctionComplianceTests

…/VectorStoreRecordKeyProperty.cs Co-authored-by: westey <164392973+westey-m@users.noreply.github.com>

roji

@adamsitnik great work, it's really nice to see this. Take a look at all the comments.

There's a lot that I think we could improve, but we also shouldn't get blocked too much on this. I do have a bit of a problem with introducing AutoGenerated as a public API into the abstraction (or at least want us to discuss this first). A lot of the rest is easy/simple, and I'm OK with deferring most of it to a future PR so we can release soon.

dotnet/src/Connectors/Connectors.Memory.Postgres/PostgresFilterTranslator.cs

dotnet/src/Connectors/Connectors.Memory.Common/SqlFilterTranslator.cs

dotnet/src/VectorDataIntegrationTests/SqlServerIntegrationTests/SqlServerCommandBuilderTests.cs

...src/VectorDataIntegrationTests/SqlServerIntegrationTests/Filter/SqlServerBasicFilterTests.cs

roji · 2025-03-04T23:12:19Z

dotnet/src/VectorDataIntegrationTests/VectorDataIntegrationTests/CRUD/ConformanceTestsBase.cs

+{
+    protected VectorStoreFixture Fixture { get; } = fixture;
+
+    protected virtual string GetUniqueCollectionName() => Guid.NewGuid().ToString();


In some databases, creating collections is a heavy operation, so we should generally try to avoid creating a collection for each test unless it's necessary. This is why the filter tests have a single collection, and all tests reuse it. I think that should be possible for the new tests being introduced here.

roji · 2025-03-04T23:13:40Z

dotnet/src/VectorDataIntegrationTests/VectorDataIntegrationTests/CRUD/ConformanceTestsBase.cs

+        var collection = this.Fixture.TestStore.DefaultVectorStore.GetCollection<TKey, TRecord>(collectionName,
+            this.GetRecordDefinition());
+
+        await collection.CreateCollectionAsync();


Unfortunately, creating a collection is a bit more complicated - in some databases this happens asynchronously, so CreateCollectionAsync returns before the collection is actually ready, and the test will fail.

See how this is done in BasicfilterTests - I've tried to design this in a way which is reusable across test suites (e.g. WaitForDataAsync).

Thanks, I was not aware of that. Is it OK to fix it later, when we actually add some tests for such DBs?

Of course. I really hope we'll be able to invest a bit in the integration test suite/infrastructure before the release to ensure we cover behavior properly across connectors.

Co-authored-by: Shay Rojansky <roji@roji.org>

…o sqlServerNew # Conflicts: # dotnet/SK-dotnet.sln # dotnet/src/Connectors/Connectors.Memory.Postgres/PostgresVectorStoreCollectionSqlBuilder.cs # dotnet/src/Connectors/Connectors.Memory.Sqlite/SqliteVectorStoreRecordCollection.cs

- remove NONCLUSTERED used PRIMARY KEY - remove COLLATE Latin1_General_100_BIN2 for VARCHAR columns - remove TimeSpan => TIME mapping - add support for TimeOnly (TIME) - make Schema optional - handle Where(x => x.Bool) - use square brackets for escaping column names in WHERE clause

…o sqlServerNew # Conflicts: # dotnet/src/Connectors/Connectors.Memory.Postgres/PostgresVectorStoreRecordCollection.cs

roji

LGTM once the comments below have been addressed, thanks @adamsitnik!

roji · 2025-03-07T14:47:56Z

dotnet/src/Connectors/Connectors.Memory.Common/SqlFilterTranslator.cs

+                this._sql.Append('\'').Append(s.Replace("'", "''")).Append('\'');
+                return;
+            case bool b:
+                this._sql.Append(b ? "TRUE" : "FALSE");


Since the representation of bool varies considerably across databases, this probably shouldn't be here (only in the overrides of this method in connector subtypes).

roji · 2025-03-07T16:12:27Z

dotnet/src/Connectors/Connectors.Memory.SqlServer/SqlServerCommandBuilder.cs

+            Type t when t == typeof(bool) => ("BIT", null),
+            Type t when t == typeof(DateTime) => ("DATETIME2", null),
+#if NET
+            Type t when t == typeof(TimeOnly) => ("TIME", null),


We also need DateOnly here, mapped to DATE.

I took a look at implementing DateOnly and I don't see a way to easily do that as of now. The reason is that the SqlDataReader maps such an output as DateTime and reader.GetProviderSpecificFieldType(this._sqlDataReader.GetOrdinal(storageName)) also returns DateTime and we also support DATETIME2 which does the same.

For TIME it was similar, as it also returns TimeSpan rather than TimeOnly. But we don't support TimeSpan, so we can just map all TimeSpan instances into TimeOnly.

I know I could give the reader the property infos or the definitions to deal with that, I just don't believe it's worth it as of now, especially if we want to get rid of this layer (the need to represent SqlDataReader as IDictionary<string, object?>).

dotnet/src/Connectors/Connectors.Memory.SqlServer/SqlServerConstants.cs

roji · 2025-03-07T16:13:19Z

dotnet/src/Connectors/Connectors.Memory.SqlServer/SqlServerConstants.cs

+#if NET
+        // We don't support mapping TimeSpan to TIME on purpose
+        // See https://github.com/microsoft/semantic-kernel/pull/10623#discussion_r1980350721
+        typeof(TimeOnly), // TIME


Also DateOnly

dotnet/src/Connectors/Connectors.Memory.SqlServer/SqlServerFilterTranslator.cs

roji · 2025-03-07T16:21:17Z

dotnet/src/Connectors/Connectors.Memory.SqlServer/SqlServerVectorStoreOptions.cs

+    /// <summary>
+    /// Gets or sets the database schema.
+    /// </summary>
+    public string? Schema { get; init; } = null;


Am noting that in PG and Sqlite this isn't nullable, but rather initialized to the default; as discussed before, I think this means that all SQLs always contain the schema (e.g. public for PG), whereas we should just omit it like we do here.

@adamsitnik do you mind applying this change (turn Schema to nullable) for the other relation providers as well? Definitely doesn't have to be in this PR.

Sure. I've logged an issue to make sure I don't forget to do that: #10853

dotnet/src/InternalUtilities/src/Data/VectorStoreRecordPropertyReader.cs

Co-authored-by: Shay Rojansky <roji@roji.org>

adamsitnik added 22 commits February 11, 2025 16:49

add new test project, move the existing tests to it

160e08e

port existing tests to Testcontainers.MsSql and re-enable them

186fda2

Revert "port existing tests to Testcontainers.MsSql and re-enable them"

22495b9

This reverts commit 186fda2.

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

3c3fd4a

…o sqlServerTests # Conflicts: # dotnet/SK-dotnet.sln

implement the tests using the new pattern, provide implementation tha…

179f56e

…t throws NotImplementedException

implement collection removal, existence check and creation

486d028

implement record insert and update (upsert)

a41cac4

implement delete operations

6dfb04a

GetAsync and GetBatchAsync

e905409

refactor

7c212da

implement UpsertBatchAsync

8d71b11

implement SelectTableNames, read the code again and add TODOs for thi…

7f18352

…ngs that need to be addressed

ensure that parameter names are always valid

32605da

add some comments

b4a73ee

support storing more types, support auto-generated keys

e8584be

simplify: don't use a dedicated query for inserting a single record

f397f3f

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

ffc4b14

…o sqlServerNew

vector search

7c8d2dc

implement filtering by reusing a lot of code implemented by @roji

9e5ef1c

reduce code duplication

080811f

skip some tests, some polishing

c17021e

The ToListAsync change is just a wokaround dotnet/runtime#79782 (comment)

remove a comment added by Copilot

4669e91

adamsitnik requested review from roji and westey-m February 20, 2025 15:54

adamsitnik had a problem deploying to integration February 20, 2025 15:55 — with GitHub Actions Failure