Return correct type when invoking GetFieldType and GetProviderSpecificFieldType for vector float32 column#4105
Return correct type when invoking GetFieldType and GetProviderSpecificFieldType for vector float32 column#4105apoorvdeshmukh wants to merge 5 commits intomainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes SqlDataReader.GetFieldType() and GetProviderSpecificFieldType() to report the correct CLR type for vector(float32) columns (matching how GetValue() already materializes the value as SqlVector<float>), addressing issue #4104.
Changes:
- Update
SqlDataReaderto returntypeof(SqlVector<float>)forSqlDbTypeExtensions.Vector(float32) inGetFieldTypeInternal. - Apply the same correction to
GetProviderSpecificFieldTypeInternal. - Add a manual test covering both APIs for a
vector(float32)column.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlDataReader.cs |
Special-cases Vector metadata so GetFieldType/GetProviderSpecificFieldType report SqlVector<float> instead of the metatype’s default byte[]. |
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/VectorTest/NativeVectorFloat32Tests.cs |
Adds test coverage to validate the updated type reporting and alignment with GetValue(). |
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlDataReader.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlDataReader.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlDataReader.cs
Outdated
Show resolved
Hide resolved
|
Shouldn't this be fixed on the SqlBuffer layer. It already identifies SqlVector and does the deserialization. Something like diff --git a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlBuffer.cs b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlBuffer.cs
index dc1abf1c..04683c3c 100644
--- a/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlBuffer.cs
+++ b/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlBuffer.cs
@@ -1136,8 +1136,16 @@ namespace Microsoft.Data.SqlClient
case StorageType.String:
return String;
case StorageType.SqlBinary:
- case StorageType.Vector:
return ByteArray;
+ case StorageType.Vector:
+ var elementType = (MetaType.SqlVectorElementType)_value._vectorInfo._elementType;
+ switch (elementType)
+ {
+ case MetaType.SqlVectorElementType.Float32:
+ return GetSqlVector<float>();
+ default:
+ throw SQL.VectorTypeNotSupported(elementType.ToString());
+ }
case StorageType.SqlCachedBuffer:
{
// If we have a CachedBuffer, it's because it's an XMLTYPE column
@@ -1213,6 +1221,15 @@ namespace Microsoft.Data.SqlClient
case StorageType.Json:
return typeof(SqlJson);
// Time Date DateTime2 and DateTimeOffset have no direct Sql type to contain them
+ case StorageType.Vector:
+ var elementType = (MetaType.SqlVectorElementType)_value._vectorInfo._elementType;
+ switch (elementType)
+ {
+ case MetaType.SqlVectorElementType.Float32:
+ return typeof(SqlVector<float>);
+ default:
+ throw SQL.VectorTypeNotSupported(elementType.ToString());
+ }
}
}
else
@@ -1262,7 +1279,14 @@ namespace Microsoft.Data.SqlClient
case StorageType.Json:
return typeof(string);
case StorageType.Vector:
- return typeof(byte[]);
+ var elementType = (MetaType.SqlVectorElementType)_value._vectorInfo._elementType;
+ switch (elementType)
+ {
+ case MetaType.SqlVectorElementType.Float32:
+ return typeof(SqlVector<float>);
+ default:
+ throw SQL.VectorTypeNotSupported(elementType.ToString());
+ }
#if NET
case StorageType.Time:
return typeof(TimeOnly);I think this last hunk is the current issue. The type returned should be retrievable. Not sure if some of this should be extracted to a helper function or something though. And more than just Float32 could be supported these days, like Half Precision Float Vectors |
Thanks for the detailed analysis @rhuijben! You're right that And yes, support for Half precision vectors is coming soon in SqlClient. :) |
src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlDataReader.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/VectorTest/NativeVectorFloat32Tests.cs
Show resolved
Hide resolved
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4105 +/- ##
==========================================
- Coverage 73.22% 66.49% -6.73%
==========================================
Files 280 274 -6
Lines 43000 65794 +22794
==========================================
+ Hits 31486 43752 +12266
- Misses 11514 22042 +10528
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
Fixes #4104.
GetFieldTypereturned type fromMetaType.ClassTypeThe fix follows similar pattern as
GetValueto determine correct type forVectormetatype.The fix has been extended to
GetProviderSpecificFieldTypewhich had similar issue.Issues
#4104
Testing
Extended NativeVectorFloat32Tests with test coverage for above APIs.
Guidelines
Please review the contribution guidelines before submitting a pull request: