You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Bind CLR UDT and binary sql_variant parameters - #655
Adds two extended SQL Server parameter targets: CLR user-defined types (SQL_SS_UDT) and SQL_C_BINARY → sql_variant.
The problem
Neither target could be bound at all.
1. A UDT parameter had no way in.SQL_SS_UDT had no row in the conversion matrix, so SQLBindParameter rejected it outright:
// Before: HYC00 at bind time, no matter what you did next.SQLBindParameter(hstmt, 1, SQL_PARAM_INPUT,
SQL_C_BINARY, SQL_SS_UDT, 0, 0,
payload, payload_len, &ind);
Even past the matrix there was nowhere to put the type name. A UDT's TDS parameter header carries a mandatory three-part name (CRPCPolicy::WriteUDTHeader, tds/tdsrpc.cpp:1099 — three B_VARCHARs, then the PLP body length), and DescRecord had no field for it, so the SQL_CA_SS_UDT_* descriptor fields were unwritable.
2. SQL_C_BINARY could not reach sql_variant.SQL_C_CHAR and SQL_C_WCHAR already did; binary was missing, so a varbinary value could not be sent inside a variant.
What this PR does
UDT payload passes through untouched. A UDT's bytes are already its IBinarySerialize form, so there is nothing to convert — only to name. SQL_C_BINARY is the accepted C type. fValidConversion also admits SQL_C_CHAR and SQL_C_WCHAR, but msodbcsql does not pass those through: rgbTRANSTYPE* gives SQL_UDT_MAPPED a SQL_C_BINARY transfer type (sqlcmisc.cpp:67), so a character source misses ConvertLongData's pass-through guard (sqlccnvt.cpp:874-877) and reaches the hex decode at sqlccnvt.cpp:1017 — "CHAR/WCHAR ->binary (2 chars are converted to only one single binary byte)". Implementing that decode is tracked separately; only_a_binary_buffer_reaches_udt_today and a_character_buffer_does_not_reach_a_udt pin the current row.
The identity comes from one of two places, the same two msodbcsql uses:
// (a) The application names the type on the IPD.SQLGetStmtAttr(hstmt, SQL_ATTR_IMP_PARAM_DESC, &ipd, 0, NULL);
SQLSetDescField(ipd, 1, SQL_CA_SS_UDT_TYPE_NAME, L"hierarchyid", SQL_NTS);
// (b) Or the server names it: describing before binding fills the IPD from// sp_describe_undeclared_parameters' suggested_user_type_* columns.SQLDescribeParam(hstmt, 1, &type, &size, &digits, &nullable);
SQLBindParameter(hstmt, 1, SQL_PARAM_INPUT, SQL_C_BINARY, SQL_SS_UDT, 0, 0, payload, len, &ind);
// no SQLSetDescField needed
(b) is refine_ipd mirroring msodbcsql's AutoFillIPD (sqlcdesc.cpp:9358), which GetIPDRec runs for fSQLDESCRIBEPARAM without requiring SQL_ATTR_ENABLE_AUTO_IPD (sqlcdesc.cpp:7877). An identity the application set explicitly always wins, and an already-bound record is left alone — the analogue of AutoFillIPD's "no parameters bound yet" condition.
With neither source, execute fails with HY000 "At least 3-parts name of a UDT type should be present", the text msodbcsql's IDS_S1_000_95 carries (sqlccmd.cpp:9977). AUdtParameterWithoutATypeNameFails asserts the SQLSTATE on both legs and msodbcsql answers HY000 too; the message text is not compared, since the IDS_S1_000_95 literal is not in this checkout.
A UDT is declared by its own type name.sp_executesql cannot resolve the TDS type name udt:
Column, parameter, or variable #1: Cannot find data type udt. (native=2715)
So get_sql_name_impl now emits the quoted one-, two-, or three-part name ([db].[schema].[type]), mirroring msodbcsql's three branches at sqlccmd.cpp:7485, the same special-casing TVPs already had.
sql_variant picks its inner type from the C type, not the value — SQL_C_BINARY declares varbinary, matching msodbcsql's CTypeToSqlType (sqlcprot.h).
Notable design point
The UDT identity lives on a new per-ordinal ParamSnapshot, not on BoundParam:
BoundParam::for_row runs once per parameter per row. An owned name inside it would cost BoundParam its Copy and re-allocate the same three strings for every row of a parameter array. msodbcsql splits it the same way: PARAMETER_INFO (per ordinal, on the IPD) holds a DWORD offset into a per-descriptor name pool, while BIND_INFO (APD side) carries the row dimension and no names at all.
Parity
Every functional aspect is pinned by e2e tests that run against both drivers. sp_describe_undeclared_parameters column indices, the WriteUDTHeader block layout, the missing-name SQLSTATE, and the declaration format are each cited to msodbcsql source in the code.
One measured divergence, asserted per-leg rather than skipped so the reference stays measured: an oversized binary sql_variant payload is refused client-side here with 22001, where msodbcsql sends it and surfaces the server's 42000. This comes from variant_column_size, which predates this PR and already governed character variants, so it is not a new decision — but binary payloads make it reachable for a second family of C types, so it is now recorded as parity deviation 20. Only the binary leg is measured, and the entry says so. Refusing locally is the same call .NET SqlClient makes, at the same threshold: SqlParameter.GetActualSize() throws ParameterInvalidVariant when isSqlVariant && coercedSize > TdsEnums.TYPE_SIZE_LIMIT, with TYPE_SIZE_LIMIT = 8000 and the comment "don't even send big values over to the variant".
SQL_CA_SS_UDT_ASSEMBLY_TYPE_NAME is accepted and echoed back but never sent: the parameter header has no field for it, and msodbcsql stores it without writing it either.
Reaching this from mssql-python
mssql-python drives the UDT path through setinputsizes:
cursor.setinputsizes([(SQL_SS_UDT, 8000, 0)])
cursor.execute("INSERT INTO t (h) VALUES (?)", serialized_hierarchyid)
microsoft/mssql-python#818 (AB#48445) supplies the identity: PreResolveUdtTypes calls SQLFreeStmt(SQL_RESET_PARAMS) and then SQLDescribeParam on each SQL_SS_UDT marker before binding, so no Python API change is needed. Both halves of that sequence depend on this PR — the describe is what fills the IPD identity, and the reset is what keeps refine_ipd from skipping a record left over from an earlier bind. UdtParamLiveTest.ResetThenDescribeRecoversTheUdtNameOnStatementReuse and ASecondResetDescribeCycleKeepsTheUdtNameFromTheCache pin that sequence on both legs.
The C type is not the application's to choose there: setinputsizes derives it from the SQL type alone (_get_c_type_for_sql_type), and _SQL_TO_C_TYPE maps SQL_SS_UDT to SQL_C_BINARY. The binary-only row above therefore costs mssql-python nothing. Temp tables and table variables may not support metadata discovery, which is a server-side sp_describe_undeclared_parameters limit rather than a driver one.
Known gaps (tracked, not in scope)
A UDT supplied through data-at-execution is buffered rather than streamed — correct on the wire, but it holds the whole value in memory. AB#48349.
A scalar C type against a UDT is refused with HYC00; msodbcsql answers 07006. The conversion matrix is still an implementation-progress list rather than a legality table. AB#48249.
cargo bclippy passes — run via scripts/bclippy.ps1, which lints the workspace andmssql-py-core; a bare cargo clippy --workspace misses the latter, which is how the SqlType::Udt match break reached CI
cargo btest passes — mssqlodbc 1830/1830 and mssql-tds 2242/2249 pass locally; the 7 failures are certificate_validator/win_tls tests that need the gitignored *.pem fixtures from generate_certs.sh, unrelated to this change
New/changed functionality has tests
Public API changes are documented
Validation
e2e run against live SQL Server, both drivers, 2026-09-25:
Leg
Tests
Passed
Failed
mssql-odbc
115
114 (1 pre-existing skip)
0
msodbcsql 18
115
106 (9 pre-existing skips)
0
All 18 UDT and 8 sql_variant e2e tests pass on both legs (115 tests in param_conversions_test, 0 failures on each).
Running these found a real bug that compilation and unit tests both missed: the udt declaration above broke every prepared UDT statement. AUdtParameterReachesAStoredProcedure was added from msodbcsql's TCLargeUDT.cpp variation_11 and is the one path that would have kept working despite it, since a procedure call never consults @params.
Unit: 1830 mssqlodbc, plus byte-level mssql-tds tests pinning the WriteUDTHeader block layout, UTF-16 length-unit counting, and the declaration format.
Caveat: the e2e run used Windows auth against a local SQL Server on master; SQL_DRIVER_VER was not recorded.
The reason will be displayed to describe this comment to others. Learn more.
Copilot review overview
🟡 Changes recommended
UDT-name changes can reuse stale prepared declarations, auto-filled identities can survive re-prepare incorrectly, and the fuzz target has an outdated call signature.
Get a fresh assessment by requesting another Copilot review.
UDT data-at-execution now intentionally buffers the entire payload, but this user-visible memory trade-off is documented only in source/PR text. The ODBC engineering guidance requires architecture-level buffering trade-offs to be recorded in mssql-odbc/README.md; add a brief UDT DAE note there with the AB#48349 follow-up.
Preserve UDT name discovery when assembly type is supplied
mssql-odbc/src/api/set_desc_field.rs:721
Writing only SQL_CA_SS_UDT_ASSEMBLY_TYPE_NAME marks the entire identity as application-supplied even though assembly is never sent. If the application then calls SQLDescribeParam, refine_ipd sees udt_names = Some with udt_names_auto_filled = false, skips the server's catalog/schema/type, and execute fails with a missing type name. Keep assembly provenance separate (or otherwise allow describe to fill the three wire-relevant fields) so an echoed-only field cannot disable server name discovery.
Serialize variant MaxLength from declared column size
mssql-odbc/src/conversion/param_convert.rs:385
variant_column_size is lost before the value reaches the wire. SqlType::Variant preserves this VarBinary length in TdsTypeContext::max_size, but TdsValueSerializer::write_type_info_bytes writes v.len() for ColumnValues::Bytes. Thus a 4-byte value bound with ColumnSize = 16 advertises MaxLength = 4, so the new “honours an explicit ColumnSize” test does not test its stated contract. Serialize the binary variant property from the declared context and assert SQL_VARIANT_PROPERTY(?, 'MaxLength') returns 16.
Avoid cloning ParamSnapshot for every array-row parameter
mssql-odbc/src/api/execute.rs:1032
This clones the whole ParamSnapshot for every parameter in every array row. For a UDT, that reallocates all three name strings on the exact per-row path ParamSnapshot was introduced to avoid. Copy only the BoundParam here and leave the per-ordinal UDT identity borrowed in stmt_state.
Assert HY000 diagnostic mapping in regression test
This regression only checks SQL_ERROR, so it does not pin the newly promised HY000 mapping; changing MissingUdtTypeName::diag() to another SQLSTATE would leave both the unit and e2e tests green. Assert the diagnostic on the exported path as the test comment and PR description claim.
Use declared ColumnSize for sql_variant varbinary metadata
mssql-odbc/src/conversion/param_convert.rs:385
ColumnSize is lost when this value reaches the wire. This creates VarBinary(..., variant_column_size(...)), but TdsValueSerializer::write_type_info_bytes writes the sql_variant BigVarBinary max-length property from v.len() rather than the preserved ctx.max_size (mssql-tds/src/datatypes/tds_value_serializer.rs:2174-2178). A 4-byte value bound with ColumnSize = 16 therefore arrives as varbinary(4), not varbinary(16); the new e2e test only round-trips the bytes and cannot detect this. Use the declared context size for the variant property and assert SQL_VARIANT_PROPERTY(?, 'MaxLength') = 16.
Add byte-level coverage for SQL UDT PLP serialization
This points to byte-level coverage that does not exist: datatypes::sql_udt::tests calls only write_udt_type_name, so it pins the three name fields but never serializes SqlType::Udt or checks the PLP length/chunk/terminator. Either add a full serialization test or narrow this comment so it does not claim the PLP body is byte-pinned.
The reason will be displayed to describe this comment to others. Learn more.
Automated review — generated by GitHub Copilot on behalf of David Engel (@David-Engel). This is not an approval and does not satisfy the human review requirement. Findings may be incomplete or wrong; push back on anything that looks off.
Summary
Adds SQL_SS_UDT parameter binding (conversion-matrix row, IPD SQL_CA_SS_UDT_* identity with explicit-vs-auto-filled provenance, UdtTypeName + write_udt_type_name on the TDS side, and the quoted one/two/three-part sp_executesql declaration) plus SQL_C_BINARY → sql_variant. The layering is right, the identity is correctly kept per-ordinal on ParamSnapshot rather than per-row on BoundParam, SqlType::Udt is properly rejected inside sql_variant and as a TVP column, and the suggested_user_type_* column ordinals (8/9/10 zero-based) match sp_describe_undeclared_parameters. Prior review rounds are thorough and I did not re-file anything already raised.
One new defect, found by probing the interaction between the two provenance mechanisms this PR introduces: clear_auto_filled_udt_names and refine_ipd disagree about the record SQL_CA_SS_UDT_ASSEMBLY_TYPE_NAME leaves behind. Details inline on mssql-odbc/src/handles/desc.rs.
Verification
Read the full diff against merge base 50a315de (28 files, +2505/-189) plus surrounding unchanged code: to_column_value_and_context / write_type_info / validate_variant_inner in sqltypes.rs, the PLP branches in tds_value_serializer.rs, and every production caller of bound_param_to_rpc (all funnel through build_named_params_for_row, so SQLExecDirect and the DAE replay both carry the identity).
Wrote a throwaway #[cfg(test)] probe in describe_param.rs reproducing describe → set assembly name → clear_auto_filled_udt_names → describe, ran it with cargo nextest run -p mssqlodbc --lib, and confirmed the record ends as UdtNames { catalog: "", schema: "", type_name: "", assembly_type_name: "MyAsm" } with udt_names_auto_filled=false. Probe reverted; the worktree is unmodified.
Did not re-run the e2e suite (needs a live SQL Server and msodbcsql) or the cross-repo mssql-python suite.
Findings
Blocking
mssql-odbc/src/handles/desc.rs — clearing an auto-filled UDT identity permanently blocks refine_ipd from re-supplying it once the application has written the assembly name. Reproduced; see the inline comment for the trace and a one-line fix.
Suggestion
None beyond what earlier rounds already raised.
Nit
None.
CI
mssql-rs Pull request validation (Build mssql-python mssql-python suite on mssql-odbc driver (cross-repo)) is failing on this head (build 178264, 4m07s); the rest of the matrix is still running. Worth confirming whether that is this change or a pre-existing cross-repo break before the PR goes up for human review — the description's validation table covers the local e2e legs but not this job.
Thanks — the blocking finding was real. Reproduced it, fixed it in 40cf3b36, and replied in detail on the inline thread.
On the cross-repo job. I dug into this and I do not think my fix explains it, so flagging rather than quietly assuming it is handled.
The correlation is suggestive: mssql-python suite on mssql-odbc driver passed on b4fb69e1 and failed on ad2a5a95, and ad2a5a95 is exactly the commit that introduced the assembly-name branch carrying the defect. It also passes on #650 and #653 on current main, so it is not a general cross-repo break.
But the mechanism does not fit. Reaching the stranded-record state requires the application to write SQL_CA_SS_UDT_ASSEMBLY_TYPE_NAME, and mssql-python has no path that does — per microsoft/mssql-python#818 its SQLSetDescField use is SQL_C_NUMERIC-only. With an empty assembly name, clear_auto_filled_udt_names behaves identically before and after ad2a5a95 (record dropped, flag cleared), so that commit is behaviour-neutral for this consumer on that path.
The likelier candidate in the same commit is the per-row allocation work in exec_common.rs / execute.rs — bound_params moved from a cloned Vec<Option<ParamSnapshot>> to a borrowed slice, and the per-row path from cloning the snapshot to copying only snapshot.param. That touches every parameterised execute, not just UDT ones, which matches a whole-suite failure far better than a UDT-identity edge case does. I read it closely and it looks correct, and mssqlodbc is 1823/1823 locally, but local unit tests would not catch a behavioural regression that only shows up through the Python binding.
I cannot read the ADO logs from here, so I could not confirm the actual failure. The job is re-running against 40cf3b36 now; if it fails again that rules out my fix and points at the allocation change, and I will bisect exec_common.rs / execute.rs from there. Agreed it should be resolved before human review — the description's validation table covers the local e2e legs only.
The ceiling added in ae5e308 was a new client-side rejection with no test
driving it through SQLBindParameter, and every UDT case in the e2e suite binds
ColumnSize 0, so neither leg exercised the boundary. That matters because
mssql-python's documented setinputsizes([(SQL_SS_UDT, 8000, 0)]) sits exactly
on the inclusive edge.
Adds a both-leg e2e asserting HY104 at 8001 and a successful bind at 8000 and
at 0, so the parity claim is measured rather than source-read, plus a
bind-level unit test covering the same three points through SQLBindParameter
rather than through the predicate alone.
Both use literals instead of SQL_PREC_UDT. Written first in terms of the
constant, the mutation that motivated the test - shrinking SQL_PREC_UDT by one
- left both tests green, because the assertions moved with the constant they
were meant to pin. With literals the same mutation fails both.
Also names the zero after the public contract: it is SQL_SS_LENGTH_UNLIMITED
(msodbcsql.h:564), the only way to bind a UDT larger than the ceiling, not
merely a permissive zero by analogy with varchar(max).
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
8d9b12d relabelled `TvpTableData::validate` as dead code and rewrote two
comments around it to say a UDT column in a TVP still writes malformed bytes.
That is wrong. `serialize_table` calls `data.validate()?` in its `Some(data)`
arm (sqltypes.rs:1245), before any column metadata or row bytes are written,
so the guard does prevent the malformed output it describes.
Restore the original meaning and record the boundary that is actually true:
the check runs after the TVP type byte and three-part name, so a rejection
keeps the metadata and rows off the wire but leaves that prefix in the writer -
already sent if it overflowed a packet. That is the partial-request shape
`validate_parameters` exists to avoid for scalar parameters, and it is worth
stating rather than implying a full preflight.
Also corrects the new integration test's own doc, which said the JS and Python
bindings reach this path. They do not yet: `mssql-py-core` refuses a UDT
parameter (types.rs:441) and `mssql-js` has no UDT path. The justification that
survives is narrower and checked - `SqlType`, `UdtTypeName` and `RpcParameter`
are public mssql-tds surface, and the preflight's rejection arms are
unreachable from the ODBC suite because SQLBindParameter refuses an empty or
overlong type name before mssql-tds is called.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The reason will be displayed to describe this comment to others. Learn more.
Summary
Reviewed the UDT and binary sql_variant binding paths against msodbcsql and the TDS UDT framing rules. I found one new bounded-UDT parity issue; existing and resolved discussions were not repeated.
The binary-to-UDT arm forwarded the whole buffer regardless of the declaration,
so a bounded binding such as ColumnSize 3 accepted and sent a four-byte
payload. msodbcsql raises 22001 when cbData > min(cbColDef, SQL_PREC_UDT)
(sqlcfunc.cpp:2681-2696), and skips the check entirely when ColumnSize is
SQL_SS_LENGTH_UNLIMITED - which is what fIsVarMax means for a UDT
(sqlcfunc.cpp:2577-2584).
Reject rather than trim, which is the part worth reading the neighbouring arm
for: SQL_BINARY/VARBINARY/LONGVARBINARY call CheckTrailingZeros and trim a
zero-only overflow (sqlcfunc.cpp:2606-2616), the behaviour trim_zero_overflow
mirrors here. The UDT arm has no such call, so reusing that helper would have
accepted a zero-padded payload the reference refuses. Both halves are asserted.
Unit coverage over the declaration edge, one byte past it, the zero-padded
overflow and the unlimited case; mutation-verified by deleting the guard. Plus
a both-leg e2e asserting 22001 for a payload past a bounded ColumnSize and a
successful round trip for the same payload at SQL_SS_LENGTH_UNLIMITED, so the
reference side stays measured rather than source-read.
This is the neighbour of the ceiling added in f507f27: that bounded the
declaration, this bounds the payload against it.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The reason will be displayed to describe this comment to others. Learn more.
Copilot review overview
🔵 Needs a closer look
Malformed UDT describe metadata is currently accepted and deferred as a misleading execute-time error.
Review effort: Balanced Findings: None
Previously missed (1)
In code that hasn't changed since last review
Reject malformed suggested user type metadata during parameter parsing
mssql-odbc/src/api/describe_param.rs:936
Malformed describe metadata is silently treated as an absent name here. Unlike read_i32/read_optional_u8, this helper maps a missing or non-string suggested_user_type_* value to None; for a UDT type-name column that lets SQLDescribeParam succeed and defers the failure to execute as a misleading missing-name error. Return Result<Option<String>, String>, accept only String/Null, and propagate type mismatches through parse_parameter_row so the existing metadata-error path drains and reports the bad response.
The reason will be displayed to describe this comment to others. Learn more.
Automated review (unattended run). This review was posted without human confirmation. It is a comment only, not an approval or change request.
Summary
Reviewed the net merge-base diff at fa598754e8c5bbb7b74ccef90beceb18db129729, including the current bounded-UDT fix, data-at-execution reconstruction, parameter arrays, descriptor metadata, RPC framing/preflight, and existing discussion. I found one genuinely new medium-severity issue in the bounded UDT data-at-execution path.
The reason will be displayed to describe this comment to others. Learn more.
Supplemental unattended review. The required e2e result completed after my first review was submitted and exposed one additional new finding. This is a comment only, not an approval or change request.
The ColumnSize bound added in fa59875 caught a data-at-execution UDT only at
close. `dae_length_limit` returned None for SQL_C_BINARY -> SQL_SS_UDT because
`same_unit` recognised SqlFamily::Binary alone, so nothing bounded the value
per chunk: a bounded UDT could accumulate arbitrarily more than its
at-most-8000-byte declaration across SQLPutData calls, and the 22001 arrived on
SQLParamData rather than on the call that overflowed. The rest of the DAE path
deliberately reports on the overflowing call, mirroring msodbcsql's SQLPutData
arms, so this was inconsistent with its own neighbours as well as the
reference.
Treat a UDT as byte-measurable: a UDT payload is bytes passed through
untouched, which is the same correspondence SqlFamily::Binary has. The bound
follows the materialized arm - min(ColumnSize, SQL_PREC_UDT), unbounded at
SQL_SS_LENGTH_UNLIMITED.
Overflow policy is now explicit rather than implied by pad_unit. Every
character and binary declaration keeps trimming an all-padding overflow; a UDT
does not, because the reference's SQL_UDT_MAPPED arm has no CheckTrailingZeros
call (sqlcfunc.cpp:2681-2696 against the varbinary arm at :2606-2616). Reusing
the binary policy would have accepted a zero-padded payload msodbcsql refuses.
Regression covers the accumulated-total bound, the first overflowing chunk, the
zero-padded overflow and the unlimited case. Mutation-verified twice: dropping
the Udt arm from same_unit, and letting a UDT trim padding, each fail it.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
AUdtPayloadPastABoundedColumnSizeIsRefused parsed a single-level hierarchyid,
which serializes to exactly one byte, so its guard assertion failed and took
the Windows and Linux ARM legs red before either half of the test ran.
The guard was right and the payload was wrong. With a one-byte payload
`payload.size() - 1` is zero, and zero is SQL_SS_LENGTH_UNLIMITED - so even
without the assertion the "bounded" case would have bound an unlimited
parameter and proved nothing. Every other hierarchyid in this suite is
single-level, so there was no precedent to borrow the size from.
Use a ten-level path, keep the assertion as the guard the reviewer asked for,
and give it a message so a future mismatch reports what is wrong rather than
`1 vs 1`.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The reason will be displayed to describe this comment to others. Learn more.
Copilot review overview
🔵 Needs a closer look
Prepared declarations can bypass the new UDT preflight and send invalid metadata before failing.
Review effort: Balanced Findings: None
Previously missed (2)
In code that hasn't changed since last review
Preflight UDT declarations before building or sending RPC
mssql-tds/src/message/rpc.rs:171
validate_parameters only sees the parameters attached to this RPC. execute_sp_prepare first converts the caller's UDT declarations into the @params string, then creates an RPC containing only @handle, @params, and @stmt; an empty or overlong UdtTypeName is therefore never checked here and the malformed declaration is sent (potentially after an Always Encrypted describe round trip). Add a declaration preflight over the original named_params before building or sending @params, plus a capturing-transport regression showing an invalid UDT declaration sends no bytes.
Update PR documentation for Arc ownership design
mssql-odbc/src/params/bound_param.rs:83
The PR description's “Notable design point” still shows Option<Box<UdtNames>>, but this field now uses shared Arc ownership (with copy-on-write updates elsewhere). Update the snippet and ownership explanation so the PR documents the design that is actually being reviewed.
The reason will be displayed to describe this comment to others. Learn more.
Copilot review overview
🔵 Needs a closer look
Large UDT serialization clones the complete buffered payload, potentially doubling peak memory usage.
Review effort: Balanced Findings: None
Previously missed (2)
In code that hasn't changed since last review
Correct inaccurate ParamSnapshot clone comment
mssql-odbc/src/api/exec_common.rs:731
This comment is inaccurate: ParamSnapshot::clone clones an Arc<UdtNames>, so it increments a reference count rather than reallocating the identity strings. Please describe the avoided refcount operation instead.
Update comment to reflect Arc-based UDT ownership
mssql-odbc/src/api/execute.rs:1027
ParamSnapshot stores UDT identity in an Arc, so cloning the snapshot does not reallocate the identity; it only bumps the reference count. Please correct the comment so it does not contradict the new ownership design.
The reason will be displayed to describe this comment to others. Learn more.
Automated review — generated by GitHub Copilot on behalf of David Engel (@David-Engel). This is not an approval and does not satisfy the human review requirement. Findings may be incomplete or wrong; push back on anything that looks off.
Summary
Reviewed the full net diff at 7ada1165 against main: the SqlType::Udt wire contract and send preflight in mssql-tds, the ODBC binding path (conversion matrix row, SqlFamily::Udt arm, ColumnSize bound, data-at-execution limit), the IPD identity model (UdtNames / UdtNameClaims / ParamSnapshot), refine_ipd's per-part merge, clear_auto_filled_udt_names, the SQL_C_BINARY → sql_variant addition, the mssql-py-core declaration keying, the fuzz harness, and the new e2e suite. I also read all 57 existing review threads and did not repeat anything already raised there, resolved or not.
The design reads sound to me. The per-field provenance split (udt_name_claimed) is the right shape for the three independently writable wire parts, the Arc sharing between the statement-side describe cache and the IPD record is safe because every writer goes through Arc::make_mut, and the parameter_definition projection correctly gates on both concise_type == SQL_SS_UDT and a non-empty type_name so an assembly-only or qualification-only record cannot orphan a prepared handle. SqlType::Udt is correctly excluded from sql_variant and from TVP columns, and dae_plan dispatches on sql_type so neither a UDT nor a binary sql_variant can be mis-routed onto the varbinary(max) streaming plan.
Two suggestions, both about parity evidence rather than behavior, on the three e2e cases added on 2026-09-28 (f507f273, fa598754, 7ada1165) — after the both-legs run recorded in the PR body. Details inline.
cargo clippy --all-targets -- -D warnings in mssql-py-core (excluded from the workspace) — clean.
cargo nextest run -p mssqlodbc --lib — 1833/1833 pass.
cargo nextest run -p mssql-tds --lib — 2242/2249 pass; the 7 failures are the pre-existing certificate_validator / win_tls cases that need the gitignored *.pem fixtures, exactly as the PR body states. Unrelated to this change.
Read-only tracing of the describe → merge → snapshot → convert → serialize path, including the SQLFreeStmt(SQL_RESET_PARAMS) + cached-describe cycle and the Arc aliasing between StmtState::parameter_udt_names and DescRecord::udt_names.
Live e2e against a SQL Server was not run here, so the msodbcsql-leg assertions below are unverified from this side.
Findings
Blocking
None.
Suggestion
mssql-odbc/tests/e2e/tests/param_conversions_test.cpp — the bounded-ColumnSize case asserts msodbcsql's SQLSTATE from a source reading, with no recorded measurement. Details inline.
mssql-odbc/tests/e2e/tests/param_conversions_test.cpp — the ColumnSize ceiling case says "measured on both legs" but carries no SQL_DRIVER_VER record. Details inline.
Both new e2e cases asserted a reference-leg SQLSTATE off a source reading, and
one of them said "measured on both legs rather than read from source" - which
is the opposite of what backs it. Neither records a SQL_DRIVER_VER or tested
build, and the PR body's both-legs run is dated 2026-09-25, before either test
existed, so nothing in the PR measures them.
Section 2.1 of the ODBC instructions requires a source citation and a
measurement for a behavioral parity claim, and admits a source citation alone
only when the entry states its evidence level and names the measurement that
would close it. These now do that: each carries an EVIDENCE paragraph naming
the --compare-with-msodbcsql run that would close it, matching how
BinaryVariantPayloadPastTheCeilingIsRefused records its own.
The unconditional assertion stays rather than becoming an ODBC_TEST_TARGET
split, and the comment says why: PR validation runs this suite on both legs, so
a wrong expectation fails the reference leg instead of shipping. If that run
shows msodbcsql answering differently, it is a measured divergence needing the
split and a registry entry, not a changed expectation.
Swept the rest of the PR's added measurement claims. The dated ones belong to
tests that existed for the 2026-09-25 run; the remaining "measured on both
legs" in this file predates the branch.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The reason will be displayed to describe this comment to others. Learn more.
Copilot review overview
🔵 Needs a closer look
A nameless data-at-execution UDT requests and buffers application data before reporting the inevitable metadata error.
Review effort: Balanced Findings: None
Previously missed (3)
In code that hasn't changed since last review
Validate UDT type names before creating DAE parameters
mssql-odbc/src/api/exec_common.rs:861
SQL_SS_UDT data-at-execution bindings take this branch before bound_param_to_rpc, so a missing type name is parked and SQLExecute returns SQL_NEED_DATA. The inevitable MissingUdtTypeName error is then deferred until rebuild_deferred_params, after the application may have supplied and buffered the entire payload, unlike the materialized execute-time HY000 path. Validate the UDT identity before creating DaeParam, and cover a no-name DAE binding with a regression test.
Document Arc refcount overhead instead of obsolete Box allocation
mssql-odbc/src/api/exec_common.rs:731
ParamSnapshot::clone now clones an Arc, so it does not reallocate each UDT identity. This rationale should name the unnecessary refcount traffic instead of documenting the superseded Box behavior.
Update stale comment about Arc cloning and UDT identity allocation
mssql-odbc/src/api/execute.rs:1027
This comment is stale now that udt_names is an Arc: cloning ParamSnapshot increments the reference count but does not reallocate the identity. Describe the actual avoided cost so future performance work is not based on the old Box design.
The reason will be displayed to describe this comment to others. Learn more.
Automated review (unattended run). This review was posted without human confirmation. It is a COMMENT only, not an approval or change request.
Reviewed the net merge-base diff and the fix delta since a598754e8c5bbb7b74ccef90beceb18db129729 at head $expectedHead, after refreshing author, status, head, discussion, threads, and checks. One new Medium issue remains: the DAE UDT fix imported the materialized UDT overflow policy, but msodbcsql's SQLPutData validator uses its binary trailing-zero rule for UDT. The earlier hard-reject recommendation was too broad; the inline finding corrects it with source-path and mutation-test evidence.
Targeted validation: the existing UDT limit test passes; a focused temporary SQLPutData probe reproduces 22001 plus sequence abort for [1, 2, 0] at ColumnSize = 2, and fails after switching the DAE policy to trim padding. Temporary mutations were removed and the review worktree is clean. Retail msodbcsql behavior at this zero-overflow boundary remains unmeasured locally; the source evidence and measurement needed to close that limit are stated inline.
9b1bbb8 gave the DAE limit a hard-reject overflow policy for a UDT, reasoning
by symmetry with the materialized arm. msodbcsql is not symmetric here.
`ValidatePutDataLength`'s SQL_C_BINARY case tests IsSQLBinary, which includes
SQL_UDT_MAPPED (sqlcprot.h:1294), and on overflow calls CheckTrailingZeros and
shortens cbValue rather than failing (sqlccmd.cpp:11192-11215) - where
ParamToSQLType's SQL_UDT_MAPPED arm has no such call (sqlcfunc.cpp:2681-2696).
So a bounded DAE value with a zero-only overflow is accepted there and was
refused here.
Drop the trims_padding field rather than set it true everywhere: with the UDT
case corrected no caller wants the strict policy, and a field that is always
true is dead weight that invites the wrong conclusion about which paths differ.
The asymmetry is now recorded on both sides - on pad_unit, and on the
materialized arm - so neither gets "fixed" later to agree with the other.
The materialized hard reject from fa59875 is unchanged and still correct.
Evidence: source reading only on both arms; a retail measurement recording
SQL_DRIVER_VER would close it. Regression keeps the per-chunk bound, the
accumulated-total bound and the unlimited case, and now asserts the trim;
mutation-verified by dropping Udt from same_unit.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The reason will be displayed to describe this comment to others. Learn more.
Automated review — generated by GitHub Copilot on behalf of David Engel (@David-Engel). This is not an approval and does not satisfy the human review requirement. Findings may be incomplete or wrong; push back on anything that looks off.
Summary
Reviewed the full merge-base diff (d001edf3..e7905f87, 32 files, +4307/-231) covering the new SqlType::Udt wire type and UdtTypeName header writer in mssql-tds, the pre-send validate_parameters preflight on SqlRpc, the SQL_CA_SS_UDT_* descriptor plumbing (DescRecord::udt_names / UdtNameClaims, set_desc_field, get_desc_field, classify_field), refine_ipd's auto-fill from sp_describe_undeclared_parameters, the new ParamSnapshot split off BoundParam, the DAE bound for SQL_SS_UDT, and the SQL_C_BINARY → sql_variant row. I found nothing I would ask you to change.
Things I checked specifically rather than took on trust:
The sp_describe_undeclared_parameters column indices are right. suggested_user_type_database / _schema / _name are the 9th, 10th and 11th columns, so 8/9/10 zero-based, consistent with the pre-existing SUGGESTED_PRECISION = 5 / SUGGESTED_SCALE = 6.
The SQL_PREC_UNLIMITED-is-zero interaction in the materialized UDT arm is correct. param.column_size != SQL_PREC_UNLIMITED && bytes.len() > param.column_size.min(SQL_PREC_UDT) reads as "any non-empty payload truncates" if SQL_PREC_UNLIMITED were not literally 0; it is, and parameter_column_size_is_valid admits 0..=8000 for SQL_SS_UDT, so the SQLBindParameter(..., SQL_SS_UDT, 0, 0, ...) shape from the description binds and sends unbounded. dae_length_limit applies the same rule via (column_size != SQL_PREC_UNLIMITED).then(...).
build_positional_params swapping iter().skip(skip).copied().collect() for bound_params.get(skip..).unwrap_or_default() preserves the out-of-range behaviour — both yield an empty slice when skip > len — and drops a per-execute Vec allocation now that ParamSnapshot is no longer Copy.
ParameterDefinition's hand-written PartialEq is load-bearing and its filter matches udt_type_name's "has a wire identity" test — non-empty type_name plus concise_type == SQL_SS_UDT — so a catalog-only, schema-only or assembly-only record projects identically to a record with no identity and cannot orphan a materialized prepared handle. Arc::ptr_eq is only a fast path in front of the value comparison, so a replaced identity holding equal strings still compares equal, which is the intended outcome.
The Arc/Arc::make_mut choice over Box is justified by the snapshot-twice-per-record path in refine_ipd and keeps BoundParamCopy for the per-row for_row call, consistent with the repo's guidance about keeping long-lived async state small.
Layering holds: the name block lives in mssql-tds::datatypes::sql_udt beside sql_tvp, write_b_varchar is promoted to pub(crate) rather than duplicated, and the ODBC layer supplies identity through ParamSnapshot rather than reaching into the message layer. Every new .rs file carries the Microsoft copyright and MIT header, new items are pub(crate) except the deliberately public SqlType::Udt / UdtTypeName, and errors go through Error::UsageError / TdsResult with no broad catches or silent fallbacks.
The preflight is honestly scoped. validate_parameters runs before serialize_prefix and serialize_batch_command write anything, and both the doc comment and the tests state plainly what it does not cover: a later row of a batch, and SqlType::Table. That is the right shape — the gap is named and tracked rather than papered over.
Verification
Read the complete diff plus the surrounding unchanged code the changes depend on: RpcParameter::serialize, SqlType::write_type_info and serialize_table, PacketWriter overflow behaviour, parameter_column_size_is_valid, sql_family / dae_length_limit, classify_field's read gate in get_desc_field, and the mssql-py-coreSqlType match arms that the new variant makes non-exhaustive.
Built and ran the UDT-related unit tests in the worktree against the stable per-PR CARGO_TARGET_DIR: cargo nextest run -p mssql-tds --lib udt — 17 tests run, 17 passed. That exercises the byte-level header layout, the UTF-16 unit counting, the nothing-written-on-late-failure preflight, the PLP framing of a UDT parameter and of a NULL UDT, the declaration formatting including ] doubling and the empty-part-is-absent rule, and the TVP rejection.
I did not re-run the full workspace suite or the cross-driver e2e suite; gh pr checks 655 is the authority there and the ADO validation run plus the cross-repo mssql-python jobs were still in progress at review time, with no failures reported.
The reason will be displayed to describe this comment to others. Learn more.
Automated review — generated by GitHub Copilot on behalf of David Engel (@David-Engel). This is not an approval and does not satisfy the human review requirement. Findings may be incomplete or wrong; push back on anything that looks off.
Summary
Reviewed the complete net diff at e7905f87 against merge base d001edf3 (32 files, +4307/-231): the SqlType::Udt wire contract and UdtTypeName name block in mssql-tds, the validate_for_send / validate_parameters send preflight, the ODBC binding path (conversion-matrix row, SqlFamily::Udt arm, the SQL_PREC_UDTColumnSize bound on both the materialized and data-at-execution legs), the IPD identity model (UdtNames / UdtNameClaims / ParamSnapshot / DaeParam), refine_ipd's per-part merge and clear_auto_filled_udt_names, the SQL_C_BINARY → sql_variant addition, the mssql-py-core declaration keying, the fuzz harness, and the new e2e and mssql-tds integration suites. I read every prior review round — including the four automated markers since ae5e308f — and confirmed each open finding is addressed at this head before looking for anything new.
I found nothing new to report. The parts I specifically tried to break all held:
The Arc<UdtNames> sharing between StmtState::parameter_udt_names, DescRecord::udt_names and ParameterDefinition is sound. The Arc::ptr_eq fast path in the hand-written PartialEq cannot produce a false "equal", because both call sites (refine_ipd and DescHandle::update_definition) hold the previous snapshot alive across the mutation, so Arc::make_mut always sees a strong count of at least 2 and copies on write rather than mutating the aliased allocation. The value comparison behind the fast path is what decides in every case that matters.
The merge_udt_identity state machine reconciles with clear_auto_filled_udt_names and set_udt_name across the empty / claimed / assembly-only / nothing-described combinations, including the SQL_CA_SS_UDT_ASSEMBLY_TYPE_NAME case the ad2a5a95 round found: a record left holding only an assembly name keeps udt_name_claimed at default, so refine_ipd can still re-supply the wire parts. The parameter_definition projection gates on both concise_type == SQL_SS_UDT and a non-empty type_name, which is the same test udt_type_name applies before an execute, so the two definitions of "has a wire identity" cannot drift.
The sp_describe_undeclared_parameters ordinals are right: suggested_user_type_database / _schema / _name are 0-based columns 8/9/10, consistent with the existing SUGGESTED_PRECISION 5 and SUGGESTED_SCALE 6, and the return_status insert correctly shifts the cached UDT ordinals before the list is sorted for refine_ipd's binary search.
The two-leg ColumnSize policy is internally consistent after e7905f87: the DAE limit trims a zero-only overflow to exactly min(ColumnSize, SQL_PREC_UDT), so the materialized arm's hard reject (bytes.len() > …) can never fire on a buffer that already passed fit, and neither leg double-reports. SQL_SS_LENGTH_UNLIMITED (0) leaves both unbounded.
Invalidation coverage is complete relative to the existing set: every production site that clears parameter_metadata (prepare.rs, exec_direct.rs, get_type_info.rs) also clears parameter_udt_names, and catalog.rs clears neither — which is pre-existing, not a gap this PR introduces.
SqlType::Udt is correctly excluded from sql_variant (validate_variant_inner) and from TVP columns (TvpColumnDef::validate), dae_plan dispatches on sql_type so neither a UDT nor a binary sql_variant can be mis-routed onto the varbinary(max) streaming plan, and SqlRpc::serialize reaches the preflight through serialize_prefix, so there is no serialization entry point that bypasses validate_parameters.
cargo nextest run -p mssqlodbc -p mssql-tds --lib --no-fail-fast — 4082 run, 4075 passed, 7 failed. All 7 are the known certificate_validator / win_tls cases needing the gitignored *.pem fixtures, exactly as the checklist states. Every new UDT and sql_variant unit test passes.
Traced the Arc aliasing and Arc::make_mut copy-on-write behaviour by hand through refine_ipd, update_definition, set_udt_name and clear_auto_filled_udt_names, and re-derived the describe column ordinals against the documented sp_describe_undeclared_parameters result set rather than only the cited reference source.
Re-checked the SQL_CA_SS_UDT_* field identifiers (1218–1221) against SQL_CA_SS_BASE + 18..21, and the PLP framing assertions against the byte-level tests in sqltypes.rs and sql_udt.rs.
cargo nextest run -p mssql-tds --test test_udt_parameters needs a live SQL Server and fails here on the shared common/mod.rs credential fixture, as every mssql-tds integration test does. Not run: the ODBC e2e suite (needs a live server plus msodbcsql on both legs) and the cross-repo mssql-python suite. The msodbcsql-leg assertions are therefore unverified from this side.
Findings
Blocking
None.
Suggestion
None.
Nit
None.
This looks ready for human review. That is not an approval, and it does not substitute for one — the parity claims in particular rest on e2e runs and reference-source readings this review could not reproduce.
The reason will be displayed to describe this comment to others. Learn more.
Automated review — generated by GitHub Copilot on behalf of Vahid Beiranvand (@Vahid-b). This is not an approval and does not satisfy the human review requirement. Findings may be incomplete or wrong; push back on anything that looks off.
Unattended automated run; these findings were not checked by a human before posting.
Delta re-review. My last automated pass was at 99cdc749; this one covers the eleven commits since, up to e7905f87 — the SQL_PREC_UDTColumnSize ceiling, the bounded-payload rule on both the materialized and data-at-execution legs, the same_unit Udt arm, the fuzz UDT draw rework, the AB#48815 renumber, the new mssql-tds integration suite, and the two new UDT e2e cases.
No blocking findings, and no correctness defect in the delta. I independently reached the same conclusion David Engel (@David-Engel) recorded at this head: after e7905f87 the two legs compose correctly — fit trims a zero-only overflow down to exactly min(ColumnSize, SQL_PREC_UDT), so the materialized arm's hard reject cannot fire on a buffer that already passed the DAE limit, and SQL_SS_LENGTH_UNLIMITED leaves both unbounded. I checked the one thing that shape could have broken and it holds: describe_tds_type maps TdsDataType::Udt to parameter_size0 for every server length (892, −1, 65535), pinned by udt_parameter_size_matches_msodbcsql, so a server-described UDT is always unbounded and the new ceiling cannot surprise the describe-then-bind flow mssql-python uses.
Both findings below are about evidence rather than behaviour, which is the only kind left on a PR this worked. Neither restates an open thread — I checked both against all 122 inline comments and every review body.
Verification
Independently confirmed the target against the live API before reading the diff and again immediately before posting: microsoft/mssql-rs PR #655, author Theekshna, draft: false, head e7905f87, state: open, not merged. The head did not move during the review.
Read at worktree head e7905f87 against merge base d001edf3 (git merge-base origin/main HEAD); net diff 32 files, +4307/−231. Delta since 99cdc749 is 8 files, +552/−41. Detached worktree with CARGO_TARGET_DIR redirected, so the primary checkout's cache is untouched.
Read .github/copilot-instructions.md, .github/instructions/pr-workflow.instructions.md, .github/instructions/mssql-odbc.instructions.md, and .github/skills/code-review/SKILL.md + posting.md; all five are present at this head.
Read all three discussion endpoints in full before drafting (/pulls/655/reviews, /pulls/655/comments — 122 inline, /issues/655/comments) and built a mechanism-level claim index. Deliberately re-filed nothing already raised, in particular the bounded-DAE e2e gap, which you explicitly declined with a reason I agree with ("I would rather it be added alongside the --compare-with-msodbcsql re-run that is already outstanding… flagging that as a deliberate gap rather than leaving it implied"), and the description drift, already raised by David Engel (@David-Engel), by me, and by the Copilot sweep.
cargo nextest run -p mssqlodbc --lib --no-fail-fast → 1833 tests run, 1833 passed, 0 skipped. (The description's checklist still says 1830; cosmetic, and part of the drift already raised.)
RUSTFLAGS="--cfg fuzzing" cargo clippy -p mssqlodbc --lib → clean. Worth stating explicitly because cargo bclippy never compiles the fuzz module, and this delta reworks fuzz_bound_param. I also confirmed the two mssql-tds entry points it newly calls, RpcParameter::get_sql_name and get_value, are pub only under #[cfg(fuzzing)] and stay pub(crate) otherwise — no production API surface added.
Mutation-verified the delta's two real behaviour changes, each in isolation:
dropping Some(SqlFamily::Udt) from same_unit → dae_limit_measures_a_bounded_udt FAILED (1 of 34 selected).
deleting the materialized column_size guard → a_udt_payload_past_a_bounded_column_size_is_refused FAILED (1 of 34 selected).
a third mutation is the basis of the second finding below.
Restored after each; the worktree is clean.
All 19 checks are green at this head, including both cross-repo mssql-python legs, coverage-report, and the merged Rust+Python coverage job.
Not verified: there is no msodbcsql or dotnet/SqlClient checkout on this host — git -C C:\Users\vbeiranvand\msodbcsql rev-parse HEAD → fatal: cannot change to '…': No such file or directory. So every sqlcfunc.cpp / sqlccmd.cpp / sqlcdesc.cpp / sqlcprot.h citation in this delta rests on your reading rather than anything I re-derived, including the ParamToSQLType-has-no-CheckTrailingZeros asymmetry that the whole trim-vs-reject split turns on. I also could not run the C++ e2e suite, so the both-legs status of the three new UDT e2e cases is unmeasured here as well as in the PR body — which your own EVIDENCE: blocks already say.
Two gaps, both of which the bar this PR sets should have caught.
The e2e comment said a zero-padded overflow is "asserted here" while the body
had only a non-zero overflow and the unlimited round trip. The zero case was
covered by a unit test that never runs on the msodbcsql leg - and it is
precisely the case read against the reference's *neighbouring* varbinary arm
rather than the UDT arm itself, so it is the least certain reading and the one
most worth measuring. Adds a third phase binding the payload plus a trailing
0x00 at ColumnSize = payload length, expecting 22001 on both legs.
The min(column_size, SQL_PREC_UDT) clamp on both the materialized and DAE arms
was unpinned: removing it from both left the whole crate green. It is not dead
code - SQLBindParameter caps ColumnSize at 8000, but SQL_DESC_LENGTH is
writable on an IPD (classify_field) and set_desc_field stores it without
calling parameter_column_size_is_valid, so bind at 0, set SQL_DESC_LENGTH to
20000, and the clamp is the only thing holding the 8000 ceiling. Covered now on
both arms; that mutation fails them.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The reason will be displayed to describe this comment to others. Learn more.
Automated review — generated by GitHub Copilot on behalf of David Engel (@David-Engel). This is not an approval and does not satisfy the human review requirement. Findings may be incomplete or wrong; push back on anything that looks off.
Unattended run: no human checked these findings before posting.
Summary
Reviewed the complete net diff at a62a3d70 against merge base d001edf3 (32 files, +4345/−231): the SqlType::Udt wire contract, UdtTypeName name block and validate_for_send preflight in mssql-tds; the ODBC conversion-matrix row, SqlFamily::Udt arm and the SQL_PREC_UDTColumnSize clamp on both the materialized and data-at-execution legs; the IPD identity model (UdtNames / UdtNameClaims / ParamSnapshot / DaeParam), refine_ipd's per-part merge and clear_auto_filled_udt_names; the SQL_C_BINARY → sql_variant addition; the mssql-py-core declaration keying; the reworked fuzz draw; and the new e2e and mssql-tds integration suites. I read every prior round — all 124 inline comments, every review body including the collapsed low-confidence blocks, and the PR-level comments — and confirmed the two open suggestions from the e7905f87 round are closed by this head before looking for anything new.
I found nothing new to report. The delta since e7905f87 is two commits of coverage: the third e2e phase binding payload + 0x00 at ColumnSize = payload.size() and expecting 22001 on both legs, and the min(column_size, SQL_PREC_UDT) clamp now pinned on both arms. I re-derived the route that makes the clamp live rather than dead — classify_field gives SQL_DESC_LENGTH(Record, !is_ird) so it is writable on an IPD, and set_desc_field stores it without calling parameter_column_size_is_valid, whose only production caller is bind_param.rs — and confirmed the clamp is the sole thing holding the 8000 ceiling on that path.
The things I specifically tried to break all held. dae_plan dispatches on sql_type with _ => DaePlan::Buffer, so neither SQL_SS_UDT nor a binary SQL_SS_VARIANT can be mis-routed onto the varbinary(max) streaming plan and land a max type inside a variant (server error 529). validate_parameters is reachable from every serialization entry point — serialize delegates to serialize_prefix, and serialize_batch_command calls it directly — so there is no path that writes a parameter name and status flags before the per-type metadata checks run. SqlType::Udt is excluded from validate_variant_inner and from TvpColumnDef::validate. variant_column_size(_, SQL_VARBINARY) takes the narrow branch of character_max_length, which is also varbinary's own non-max bound, so the binary variant clamps at 8000 like the character ones. write_record_field does not touch explicitly_bound, so a SQL_CA_SS_UDT_* write does not freeze the record against a later describe — provenance is carried entirely by UdtNameClaims, per part. format_udt_sql_name doubles ] on every part it quotes, including the server-supplied catalog and schema, so neither source can break out of the bracketed name in the sp_executesql declaration.
One item is already on record from earlier rounds and is not re-filed here: the PR description's "Notable design point" snippet still shows Option<Box<UdtNames>> where the field is Option<Arc<UdtNames>>, and the checklist still says 1830 mssqlodbc tests where the crate now has 1833.
Verification
Confirmed the target against the live API before reading the diff and again immediately before posting: microsoft/mssql-rs PR #655, author Theekshna, head a62a3d70, open, not draft. The head did not move during the review.
Read at worktree head a62a3d70 against git merge-base origin/main HEAD = d001edf3, in a detached worktree with CARGO_TARGET_DIR redirected, so the primary checkout is untouched.
mssql-py-core handled separately because it is outside the workspace and this diff touches it: cargo fmt -- --check exit 0, cargo clippy --all-features --all-targets -- -D warnings exit 0.
cargo nextest run -p mssqlodbc -p mssql-tds --lib --no-fail-fast — 4082 run, 4075 passed, 7 failed. All 7 are the known certificate_validator / win_tls cases needing the gitignored *.pem fixtures, exactly as the checklist states; every new UDT and sql_variant unit test passes.
Mutation-verified the delta rather than trusting the commit message: replaced param.column_size.min(SQL_PREC_UDT) with param.column_size in bound_param_to_value_with_outcome's SqlFamily::Udt arm andcolumn_size.min(SQL_PREC_UDT) with column_size in dae_length_limit's SQL_SS_UDT arm, in one mutation. a_udt_payload_past_a_bounded_column_size_is_refused and dae_limit_measures_a_bounded_udt both FAILED (2 of 34 selected). Restored with git checkout --; the worktree is clean.
Not verified: there is no msodbcsql or dotnet/SqlClient checkout on this host, so every sqlcfunc.cpp / sqlccmd.cpp / sqlcdesc.cpp / sqlcmisc.cpp / sqlcprot.h citation rests on the author's reading rather than anything re-derived here — including the ParamToSQLType-has-no-CheckTrailingZeros asymmetry the whole trim-vs-reject split turns on. The C++ e2e suite and the mssql-tds integration suite both need a live server, so the new third e2e phase and the msodbcsql-leg expectations are unmeasured from this side; the EVIDENCE: blocks in the test file already say so.
Findings
Blocking
None.
Suggestion
None.
Nit
None.
This looks ready for human review, but it is not approved — a human still needs to sign off, particularly on the msodbcsql parity claims, deviation 20, and the newly asserted zero-padded overflow expectation, all of which rest on source reading and runs this review cannot reproduce.
The reason will be displayed to describe this comment to others. Learn more.
Automated review — generated by GitHub Copilot on behalf of David Engel (@David-Engel). This is not an approval and does not satisfy the human review requirement. Findings may be incomplete or wrong; push back on anything that looks off.
Summary
Reviewed the full diff at a62a3d7 against the merge base with origin/main (d001edf): 32 files, +4345/-231. The change adds SqlType::Udt with a UdtTypeName three-part identity and a WriteUDTHeader-shaped TYPE_INFO block in mssql-tds, hoists per-parameter metadata validation into a pre-serialization validate_parameters preflight, adds the SQL_CA_SS_UDT_* IPD fields with per-part provenance tracking (UdtNameClaims), wires SQLDescribeParam's suggested_user_type_* columns into refine_ipd, and opens SQL_C_BINARY → sql_variant and SQL_C_BINARY → SQL_SS_UDT in the conversion matrix.
I found no new actionable defects. The layering holds up on the points I checked most closely:
Every non-test mutation of DescRecord::udt_names goes through Arc::make_mut (set_desc_field.rs:714, desc.rs:580, describe_param.rs:567), so the Arc shared from StmtState::parameter_udt_names into the descriptor at describe_param.rs:565 cannot be aliased-mutated. That was the one place a copy-on-write slip would have been a real bug.
refine_ipd's cached path is gated on parameter_metadata.len() == marker_count, so an empty cache can never replay as "no UDT described" and wipe an identity.
parameter_definition()'s projection and ParamSnapshot's filter apply the same "is a SQL_SS_UDT record with a non-empty type name" test that udt_type_name applies before execute, so the three definitions cannot drift.
dae_length_limit returns Ok(None) for SqlFamily::Variant at the same_unit gate, so the newly-reachable SQL_C_BINARY → SQL_SS_VARIANT DAE binding never falls through to the other => UnsupportedSqlType arm.
The sp_describe_undeclared_parameters ordinals 8/9/10 line up with suggested_user_type_database/_schema/_name, leaving suggested_assembly_qualified_type_name at 11 as the code comment says.
character_max_length(SQL_VARBINARY) takes the narrow branch (8000), which is what the updated variant_column_size doc claims.
Everything else I flagged while reading was already covered by one of the existing review threads on this PR, so I have not restated it.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
David Engel (David-Engel)
left a comment
Shiwani Gupta (shiwanigupta0809)
left a comment
Vahid Beiranvand (Vahid-b)
left a comment
Description
Adds two extended SQL Server parameter targets: CLR user-defined types (
SQL_SS_UDT) andSQL_C_BINARY→sql_variant.The problem
Neither target could be bound at all.
1. A UDT parameter had no way in.
SQL_SS_UDThad no row in the conversion matrix, soSQLBindParameterrejected it outright:Even past the matrix there was nowhere to put the type name. A UDT's TDS parameter header carries a mandatory three-part name (
CRPCPolicy::WriteUDTHeader,tds/tdsrpc.cpp:1099— three B_VARCHARs, then the PLP body length), andDescRecordhad no field for it, so theSQL_CA_SS_UDT_*descriptor fields were unwritable.2.
SQL_C_BINARYcould not reachsql_variant.SQL_C_CHARandSQL_C_WCHARalready did; binary was missing, so avarbinaryvalue could not be sent inside a variant.What this PR does
UDT payload passes through untouched. A UDT's bytes are already its
IBinarySerializeform, so there is nothing to convert — only to name.SQL_C_BINARYis the accepted C type.fValidConversionalso admitsSQL_C_CHARandSQL_C_WCHAR, but msodbcsql does not pass those through:rgbTRANSTYPE*givesSQL_UDT_MAPPEDaSQL_C_BINARYtransfer type (sqlcmisc.cpp:67), so a character source missesConvertLongData's pass-through guard (sqlccnvt.cpp:874-877) and reaches the hex decode atsqlccnvt.cpp:1017— "CHAR/WCHAR ->binary (2 chars are converted to only one single binary byte)". Implementing that decode is tracked separately;only_a_binary_buffer_reaches_udt_todayanda_character_buffer_does_not_reach_a_udtpin the current row.The identity comes from one of two places, the same two msodbcsql uses:
(b) is
refine_ipdmirroring msodbcsql'sAutoFillIPD(sqlcdesc.cpp:9358), whichGetIPDRecruns forfSQLDESCRIBEPARAMwithout requiringSQL_ATTR_ENABLE_AUTO_IPD(sqlcdesc.cpp:7877). An identity the application set explicitly always wins, and an already-bound record is left alone — the analogue ofAutoFillIPD's "no parameters bound yet" condition.With neither source, execute fails with
HY000"At least 3-parts name of a UDT type should be present", the text msodbcsql'sIDS_S1_000_95carries (sqlccmd.cpp:9977).AUdtParameterWithoutATypeNameFailsasserts the SQLSTATE on both legs and msodbcsql answersHY000too; the message text is not compared, since theIDS_S1_000_95literal is not in this checkout.A UDT is declared by its own type name.
sp_executesqlcannot resolve the TDS type nameudt:So
get_sql_name_implnow emits the quoted one-, two-, or three-part name ([db].[schema].[type]), mirroring msodbcsql's three branches atsqlccmd.cpp:7485, the same special-casing TVPs already had.sql_variantpicks its inner type from the C type, not the value —SQL_C_BINARYdeclaresvarbinary, matching msodbcsql'sCTypeToSqlType(sqlcprot.h).Notable design point
The UDT identity lives on a new per-ordinal
ParamSnapshot, not onBoundParam:BoundParam::for_rowruns once per parameter per row. An owned name inside it would costBoundParamitsCopyand re-allocate the same three strings for every row of a parameter array. msodbcsql splits it the same way:PARAMETER_INFO(per ordinal, on the IPD) holds aDWORDoffset into a per-descriptor name pool, whileBIND_INFO(APD side) carries the row dimension and no names at all.Parity
Every functional aspect is pinned by e2e tests that run against both drivers.
sp_describe_undeclared_parameterscolumn indices, theWriteUDTHeaderblock layout, the missing-name SQLSTATE, and the declaration format are each cited to msodbcsql source in the code.One measured divergence, asserted per-leg rather than skipped so the reference stays measured: an oversized binary
sql_variantpayload is refused client-side here with22001, where msodbcsql sends it and surfaces the server's42000. This comes fromvariant_column_size, which predates this PR and already governed character variants, so it is not a new decision — but binary payloads make it reachable for a second family of C types, so it is now recorded as parity deviation 20. Only the binary leg is measured, and the entry says so. Refusing locally is the same call .NET SqlClient makes, at the same threshold:SqlParameter.GetActualSize()throwsParameterInvalidVariantwhenisSqlVariant && coercedSize > TdsEnums.TYPE_SIZE_LIMIT, withTYPE_SIZE_LIMIT = 8000and the comment "don't even send big values over to the variant".SQL_CA_SS_UDT_ASSEMBLY_TYPE_NAMEis accepted and echoed back but never sent: the parameter header has no field for it, and msodbcsql stores it without writing it either.Reaching this from mssql-python
mssql-python drives the UDT path through
setinputsizes:microsoft/mssql-python#818 (AB#48445) supplies the identity:
PreResolveUdtTypescallsSQLFreeStmt(SQL_RESET_PARAMS)and thenSQLDescribeParamon eachSQL_SS_UDTmarker before binding, so no Python API change is needed. Both halves of that sequence depend on this PR — the describe is what fills the IPD identity, and the reset is what keepsrefine_ipdfrom skipping a record left over from an earlier bind.UdtParamLiveTest.ResetThenDescribeRecoversTheUdtNameOnStatementReuseandASecondResetDescribeCycleKeepsTheUdtNameFromTheCachepin that sequence on both legs.The C type is not the application's to choose there:
setinputsizesderives it from the SQL type alone (_get_c_type_for_sql_type), and_SQL_TO_C_TYPEmapsSQL_SS_UDTtoSQL_C_BINARY. The binary-only row above therefore costs mssql-python nothing. Temp tables and table variables may not support metadata discovery, which is a server-sidesp_describe_undeclared_parameterslimit rather than a driver one.Known gaps (tracked, not in scope)
HYC00; msodbcsql answers07006. The conversion matrix is still an implementation-progress list rather than a legality table. AB#48249.Related Issues
https://sqlclientdrivers.visualstudio.com/mssql-rs/_workitems/edit/48248
Checklist
cargo bfmtpassescargo bclippypasses — run viascripts/bclippy.ps1, which lints the workspace andmssql-py-core; a barecargo clippy --workspacemisses the latter, which is how theSqlType::Udtmatch break reached CIcargo btestpasses —mssqlodbc1830/1830 andmssql-tds2242/2249 pass locally; the 7 failures arecertificate_validator/win_tlstests that need the gitignored*.pemfixtures fromgenerate_certs.sh, unrelated to this changeValidation
e2e run against live SQL Server, both drivers, 2026-09-25:
All 18 UDT and 8
sql_variante2e tests pass on both legs (115 tests inparam_conversions_test, 0 failures on each).Running these found a real bug that compilation and unit tests both missed: the
udtdeclaration above broke every prepared UDT statement.AUdtParameterReachesAStoredProcedurewas added from msodbcsql'sTCLargeUDT.cpp variation_11and is the one path that would have kept working despite it, since a procedure call never consults@params.Unit: 1830
mssqlodbc, plus byte-levelmssql-tdstests pinning theWriteUDTHeaderblock layout, UTF-16 length-unit counting, and the declaration format.Caveat: the e2e run used Windows auth against a local SQL Server on
master;SQL_DRIVER_VERwas not recorded.