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
.Net: Implementation of store using Azure SQL/SQL Server with vector search. #6142
Conversation
1531f38
to
b69819d
Compare
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.
LGTM, great to see this! See two comments below.
dotnet/src/Connectors/Connectors.Memory.SqlServer/SqlServerClient.cs
Outdated
Show resolved
Hide resolved
dotnet/src/Connectors/Connectors.Memory.SqlServer/SqlServerMemoryBuilderExtensions.cs
Show resolved
Hide resolved
d19420d
to
037af43
Compare
@markwallace-microsoft - please can we have a review from someone on your team. |
037af43
to
4a34783
Compare
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.
Small comments, but in general looks good to me! Thank you!
dotnet/src/Connectors/Connectors.Memory.SqlServer/ISqlServerClient.cs
Outdated
Show resolved
Hide resolved
dotnet/src/Connectors/Connectors.Memory.SqlServer/SqlServerClient.cs
Outdated
Show resolved
Hide resolved
dotnet/src/Connectors/Connectors.Memory.SqlServer/SqlServerClient.cs
Outdated
Show resolved
Hide resolved
dotnet/src/Connectors/Connectors.Memory.SqlServer/SqlServerMemoryEntry.cs
Show resolved
Hide resolved
368b773
to
800596b
Compare
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.
LGTM!
dotnet/src/Connectors/Connectors.Memory.SqlServer/SqlServerClient.cs
Outdated
Show resolved
Hide resolved
dotnet/src/Connectors/Connectors.Memory.SqlServer/SqlServerMemoryStore.cs
Show resolved
Hide resolved
[heart] Pooja Kamath reacted to your message:
________________________________
From: Shay Rojansky ***@***.***>
Sent: Thursday, May 16, 2024 9:49:54 AM
To: microsoft/semantic-kernel ***@***.***>
Cc: Pooja Kamath ***@***.***>; Comment ***@***.***>
Subject: Re: [microsoft/semantic-kernel] .Net: Implementation of store using Azure SQL/SQL Server with vector search. (PR #6142)
@roji approved this pull request.
LGTM!
________________________________
In dotnet/src/Connectors/Connectors.Memory.SqlServer/SqlServerClient.cs<#6142 (comment)>:
+ }
+ await cmd.ExecuteNonQueryAsync(cancellationToken).ConfigureAwait(false);
+ }
+
+ /// <inheritdoc/>
+ public async IAsyncEnumerable<(SqlServerMemoryEntry, double)> GetNearestMatchesAsync(string tableName, ReadOnlyMemory<float> embedding, int limit, double minRelevanceScore = 0, bool withEmbeddings = false, [EnumeratorCancellation] CancellationToken cancellationToken = default)
+ {
+ var queryColumns = withEmbeddings
+ ? "[key], [metadata], [timestamp], 1 - VECTOR_DISTANCE('cosine', [embedding], ***@***.***)) AS [cosine_similarity], VECTOR_TO_JSON_ARRAY([embedding]) AS [embedding]"
+ : "[key], [metadata], [timestamp], 1 - VECTOR_DISTANCE('cosine', [embedding], ***@***.***)) AS [cosine_similarity]";
+ var fullTableName = this.GetFullTableName(tableName);
+ using var connection = new SqlConnection(this._connectionString);
+ await connection.OpenAsync(cancellationToken).ConfigureAwait(false);
+ using var cmd = connection.CreateCommand();
+ cmd.CommandText = $"""
+ WITH data as (
Seems like there's no need for the CTE here, no? Can just append the queryColumns after the TOP, and the fullTableName after FROM?
________________________________
In dotnet/src/Connectors/Connectors.Memory.SqlServer/SqlServerMemoryStore.cs<#6142 (comment)>:
+
+ /// <summary>
+ /// Initializes a new instance of the <see cref="SqlServerMemoryStore"/> class.
+ /// </summary>
+ /// <param name="sqlServerClient">An instance of <see cref="ISqlServerClient"/>.</param>
+ public SqlServerMemoryStore(ISqlServerClient sqlServerClient)
+ {
+ this._sqlServerClient = sqlServerClient;
+ }
+
+ /// <inheritdoc/>
+ public async Task CreateCollectionAsync(string collectionName, CancellationToken cancellationToken = default)
+ {
+ Verify.NotNull(collectionName);
+
+ await this._sqlServerClient.CreateTableAsync(collectionName, cancellationToken).ConfigureAwait(false);
FWIW not sure why we need the separation between SqlServerMemoryStore and SqlServerClient, i.e. why the code can't be directly in here... But I know other connectors are structured this way and it's not important.
—
Reply to this email directly, view it on GitHub<#6142 (review)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AOM3UAWL5CE3J3LNZMWW2D3ZCR6MFAVCNFSM6AAAAABHKPLDL6VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDANJYG42TOMRQGU>.
You are receiving this because you commented.Message ID: ***@***.***>
|
800596b
to
0c01a51
Compare
@dmytrostruk can you re-review the latest changes please (or merge). Thanks. |
4114cd9
to
c323afe
Compare
LGTM, thanks @cincuranet ! |
dotnet/src/Connectors/Connectors.Memory.SqlServer/SqlServerClient.cs
Outdated
Show resolved
Hide resolved
dotnet/src/Connectors/Connectors.Memory.SqlServer/ISqlServerClient.cs
Outdated
Show resolved
Hide resolved
dotnet/src/Connectors/Connectors.Memory.SqlServer/SqlServerMemoryStore.cs
Show resolved
Hide resolved
4bc0e05
to
4f45831
Compare
4f45831
to
683459b
Compare
683459b
to
17b7696
Compare
cc @yorek @SamMonoRT @roji @luisquintanilla