-
Notifications
You must be signed in to change notification settings - Fork 310
Support vector datatype with SqlVector<T> #3443
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
Conversation
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.
Pull Request Overview
This PR replaces the legacy SqlVectorFloat32
type with a new generic SqlVector<T>
implementation, updating all client code, tests, and documentation to use the generic API.
- Introduces
SqlVector<T>
in place ofSqlVectorFloat32
, handling only supported unmanaged types - Removes
SqlVectorFloat32
code, test classes, and related resource entries - Updates SQL client surface (
SqlDataReader
,SqlParameter
,SqlBuffer
,ISqlVector
), tests, and XML docs to reference the generic API
Reviewed Changes
Copilot reviewed 22 out of 23 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlTypes/SqlVector.cs | Adds generic SqlVector<T> implementation |
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlTypes/SqlVectorFloat32.cs | Deleted obsolete specific vector type |
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlDataReader.cs | Replaces GetSqlVectorFloat32 with generic GetSqlVector<T> |
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlParameter.cs | Updates vector payload coercion and return to use generic |
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlBuffer.cs | Replaces SqlVectorFloat32 uses with generic accessors |
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ISqlVector.cs | Updates interface to match generic API |
src/Microsoft.Data.SqlClient/tests/UnitTests/SqlVectorTest.cs | Adds unit tests for generic vector behavior |
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/VectorTest/NativeVectorFloat32Tests.cs | Updates manual tests to use SqlVector<float> |
src/Microsoft.Data.SqlClient/src/Resources/Strings.resx | Removes unused vector resource strings |
doc/snippets/Microsoft.Data.SqlTypes/SqlVector.xml | Adds XML docs for generic SqlVector<T> |
src/Microsoft.Data.SqlClient/src/Microsoft/Data/Common/AdapterUtil.cs | Removes obsolete vector helpers |
Files not reviewed (1)
- src/Microsoft.Data.SqlClient/src/Resources/Strings.Designer.cs: Language not supported
Comments suppressed due to low confidence (2)
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/VectorTest/NativeVectorFloat32Tests.cs:100
- [nitpick] The method name
ValidateSqlVectorFloat32Object
no longer matches the genericSqlVector<T>
type; consider renaming it to something likeValidateSqlVectorObject
for clarity.
private void ValidateSqlVectorFloat32Object(bool isNull, SqlVector<float> sqlVectorFloat32, float[] expectedData, int expectedSize, int expectedLength)
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ISqlVector.cs:28
- [nitpick] Add an XML doc comment for
VectorPayload
to explain that it returns the raw TDS payload of the vector.
byte[] VectorPayload { get; }
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/VectorTest/NativeVectorFloat32Tests.cs
Outdated
Show resolved
Hide resolved
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.
Mostly commentary, and a few minor changes.
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlTypes/SqlVector.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/VectorTest/NativeVectorFloat32Tests.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/VectorTest/NativeVectorFloat32Tests.cs
Show resolved
Hide resolved
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.
Looks good. I like the generic API.
src/Microsoft.Data.SqlClient/src/System/Runtime/CompilerServices/IsExternalInit.netfx.cs
Show resolved
Hide resolved
6095de8
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3443 +/- ##
==========================================
- Coverage 64.55% 58.84% -5.71%
==========================================
Files 281 275 -6
Lines 62519 62220 -299
==========================================
- Hits 40358 36614 -3744
- Misses 22161 25606 +3445
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Description
This PR adds support for vector datatype with generic SqlType class SqlVector
After considering the feedback from discussion #3382, existing APIs are now supported with revised APIs from SqlVector
This helps in reducing the API surface area for unmanaged types.
Issues
#3317
Testing
Unit Tests for SqlVector
BackWardCompatibility Test suite to ensure support for APIs that worked with varchar(max) for vector datatype.
Tests for native vector datatype.