[DRAFT] Add TVP Support for JSON and Vector (float32)#4458
[DRAFT] Add TVP Support for JSON and Vector (float32)#4458apoorvdeshmukh wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This draft PR adds end-to-end Table-Valued Parameter (TVP) support for the new SQL Server json and vector(float32) types by extending the SMI/TDS metadata and value plumbing, and by adding functional/manual validation tests.
Changes:
- Extend TVP TYPE_INFO serialization to support
json(asNVARCHAR(max)+ collation) andvector(token + byte length + element-type indicator). - Update SMI metadata/value utilities to allow JSON and Vector columns in
SqlMetaData/SqlDataRecordand to map accessors/setters appropriately. - Add functional and manual tests validating JSON/Vector TVP round-trips and insert scenarios.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/VectorTest/NativeVectorFloat32Tests.cs | Adds manual TVP round-trip and insert tests for vector(float32). |
| src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/JsonTest/JsonTest.cs | Adds manual TVP round-trip and insert tests for json. |
| src/Microsoft.Data.SqlClient/tests/FunctionalTests/SqlMetaDataTest.cs | Adds in-memory (no-server) functional coverage for JSON/Vector SqlMetaData + SqlDataRecord. |
| src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsValueSetter.cs | Enables writing NULL for non-PLP Vector columns via the varlength null marker. |
| src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParser.cs | Serializes TVP TYPE_INFO for JSON and Vector. |
| src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Server/ValueUtilsSmi.cs | Adds getter/setter plumbing for JSON/Vector values and access map handling. |
| src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Server/SqlMetaData.cs | Adds default metadata and constructor support for JSON/Vector. |
| src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Server/SmiXetterAccessMap.cs | Maps JSON/Vector xetter validity to equivalent existing types (NVarChar/VarBinary). |
| src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Server/SmiMetaData.cs | Adds default SMI metadata and type-name entries for JSON/Vector. |
| src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/Server/MetadataUtilsSmi.cs | Extends type-code inference and schema-row metadata building for JSON/Vector. |
| DBNull.Value, // SqlDbType.DateTime2 | ||
| DBNull.Value, // SqlDbType.DateTimeOffset | ||
| SqlString.Null, // SqlDbTypeExtensions.Json (value 35) | ||
| SqlBinary.Null, // SqlDbTypeExtensions.Vector (value 36) |
| else if (SqlDbTypeExtensions.Vector == dbType) | ||
| { | ||
| // Vector max length is the total byte size (8-byte header + element payload). | ||
| if (maxLength < TdsEnums.VECTOR_HEADER_SIZE) | ||
| { | ||
| throw ADP.Argument(StringsHelper.GetString(Strings.ADP_InvalidDataLength2, maxLength.ToString(CultureInfo.InvariantCulture)), nameof(maxLength)); | ||
| } | ||
| } |
| case SqlDbTypeExtensions.Json: | ||
| { | ||
| Debug.Assert(CanAccessSetterDirectly(metaData[i], ExtendedClrTypeCode.String)); | ||
| // JSON travels as a single UTF-8 PLP value (see TdsValueSetter.SetString). |
| case SqlDbTypeExtensions.Json: | ||
| { | ||
| Debug.Assert(CanAccessSetterDirectly(metaData[i], ExtendedClrTypeCode.String)); | ||
| // JSON travels as a single UTF-8 PLP value (see TdsValueSetter.SetString). |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4458 +/- ##
==========================================
- Coverage 65.83% 65.33% -0.50%
==========================================
Files 287 285 -2
Lines 43763 66986 +23223
==========================================
+ Hits 28812 43767 +14955
- Misses 14951 23219 +8268
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
edwardneal
left a comment
There was a problem hiding this comment.
Coincidentally, I'd been trying to get these types flowing as TVP fields over the last few days! I'd bumped into a problem with undocumented TYPE_INFO for vector and some transport difficulties with json.
Most of these are just questions or ad-hoc housekeeping. The two key points which I'm unsure about are the code paths used for DataTable instances, and whether we should really be transporting JSON data as Unicode-encoded nvarchars.
| WriteUnsignedInt(_defaultCollation._info, stateObj); | ||
| stateObj.WriteByte(_defaultCollation._sortId); |
There was a problem hiding this comment.
Not all collations support every character (é is a decent example here.) In such circumstances, SQL Server will strip away the accent. If the user's database uses one of these collations, will SQL Server convert a JSON string of {"a":"café"} to {"a": "cafe"}?
| // JSON and Vector are outside the contiguous SqlDbType range covered by this map; | ||
| // JSON accepts the same setters as NVarChar (string) and Vector the same as VarBinary (bytes). |
There was a problem hiding this comment.
The array runs from BigInt to DateTimeOffset. Json and Vector directly follow this. I'm not sure we need the special handling here.
There are a handful of other places which this also applies to.
| if (SqlDbTypeExtensions.Json == effectiveType) | ||
| { | ||
| effectiveType = SqlDbType.NVarChar; | ||
| } |
There was a problem hiding this comment.
Treating Json as NVarChar might allow modification of individual characters via SqlDataRecord.SetChars. If that's true, it'd be possible to send invalid JSON strings to SQL Server.
Similar principle might also apply to VarBinary - would it allow arbitrary rewrites of the vector header via SetBytes?
| using (SqlCommand createType = new($"CREATE TYPE {tvpTypeName} AS TABLE (Id int, Data json)", connection)) | ||
| { | ||
| createType.ExecuteNonQuery(); | ||
| } |
There was a problem hiding this comment.
We have an RAII primitive for this.
(here and elsewhere.)
There was a problem hiding this comment.
It's also worth noting that TVPs can be sent using DataTables, and this uses a different pathway to detect the type.
| result = GetSqlValue(getters, ordinal, metaData); | ||
| break; | ||
| case SqlDbTypeExtensions.Json: | ||
| result = new SqlString(GetString_Unchecked(getters, ordinal)); |
There was a problem hiding this comment.
What would be the motivation for exposing this as a SqlString (or, further up, a string) rather than a SqlJson?
| ); | ||
|
|
||
| // Vector values are transferred as their raw TDS payload bytes (8-byte header + elements). | ||
| if (SqlDbTypeExtensions.Vector == metaData.SqlDbType && value is ISqlVector vectorValue) |
There was a problem hiding this comment.
What happens if I pass null / SqlVector<float>.Null to this method? At a glance, I don't think it'll pass value is ISqlVector vectorValue and thus that it'll leave the previous value in place.
| // The SQL Server TVP protocol carries a JSON column as NVARCHAR(max) + collation, | ||
| // with the value sent as UTF-16 (the server converts it to JSON on insert). This | ||
| // matches the Microsoft JDBC driver's TVP/bulk representation of JSON columns. | ||
| stateObj.WriteByte(TdsEnums.SQLNVARCHAR); |
There was a problem hiding this comment.
This is where I hit a wall a couple of days ago.
Encoding this as an SQLNVARCHAR will double the number of bytes needed to transmit ASCII text over the network. If we send SqlJson instances as a scalar parameter we encode the value to UTF8, so I'd tried to send it as a SQLJSON type (which is a PLP type.) This triggered some unusual behaviour within SQL Server though, are you able to replicate this?
|
|
||
| SqlDataRecord row1 = new SqlDataRecord(metadata); | ||
| row1.SetInt32(0, 1); | ||
| row1.SetValue(1, SqlVector<float>.CreateNull(s_vectorDimensions)); // NULL vector |
There was a problem hiding this comment.
What happens if we call row1.SetDBNull?
| }; | ||
| SqlDataRecord row = new SqlDataRecord(metadata); | ||
| row.SetInt32(0, 1); | ||
| row.SetValue(1, new SqlVector<float>(values)); |
There was a problem hiding this comment.
API nitpick: it'd be nice to have SqlDataRecord.SetVector<T> and SqlDataRecord.SetJson...
Description
This PR is a work in progress for adding TVP support for JSON and vector(float32) datatypes.
Issues
#3449
Testing
Added functional and manual tests where applicable.
Guidelines
Please review the contribution guidelines before submitting a pull request: