microsoft / microsoft/mssql-rs
Add exhaustive `decode`/`decode_into` behavior-parity tests
- Dominant language
- Rust
- Stars
- 53
- Forks
- 14
- Avg merge
- 1d 15h
- Merged PRs (30d)
- 137
Description
### Problem statement
`GenericDecoder::decode` and `GenericDecoder::decode_into` are separate hand-maintained implementations of the same TDS type matrix. A divergence can silently return wrong data rather than fail compilation.
PR #269 attempted to remove that risk structurally by defining `decode` through `decode_into`, but the required post-#264 measurements found a 19–22% regression on the integer-heavy row path. A single bounded cold-partition attempt reduced but did not eliminate the cost (+8.76% discard, +5.88% materialization), so #269 was closed without landing. The divergence risk remains and needs a zero-runtime-cost mitigation.
This issue is **tests only**. It is not a re-attempt at decoder convergence, a per-column decode plan, or any production-path refactor.
### Proposed solution
Expand the existing `assert_decode_equivalence` test approach into an exhaustive parity matrix for the top-level `TdsDataType` enum.
For every supported result type, construct representative `ColumnMetadata` and wire bytes, run both `decode` and `decode_into`, and assert that they:
1. consume exactly the same bytes;
2. produce the same `ColumnValues` value, including typed NULLs;
3. return the same error category and expected message where decoding is rejected;
4. preserve behavior for both value and NULL encodings where the wire format supports NULL.
Make the case enumeration compile-time exhaustive over `TdsDataType` (no wildcard), so adding a new enum variant forces an explicit parity decision.
The matrix should cover all 46 top-level variants:
- 39 implemented decode variants;
- `Decimal` and `Numeric`, which are explicitly rejected legacy fixed-length IDs (`DecimalN`/`NumericN` are the supported result forms);
- the five terminal variants `Void`, `VarBinary`, `Binary`, `SqlTable`, and `None`, with their unreachable/unsupported result-column rationale pinned in the tests.
Pin the edge cases most likely to drift:
- `Image` with an empty payload produces NULL, while `Text`/`NText` with an empty payload produce an empty string;
- PLP NULL, known-length, unknown-length, and malformed/non-PLP metadata paths, including expected error strings;
- LONGLEN pointer-absent, empty, normal, and oversize/error paths;
- length-discriminated `IntN`, `FltN`, `MoneyN`, `BitN`, and date/time-N forms;
- `Guid` length validation;
- `SsVariant` recursive decoding and `Vector` decoding.
Reuse the existing in-crate reader, metadata builders, writer fixtures, and `assert_decode_equivalence` pattern. Do not add a benchmark or modify production decoder code.
**Definition of done**
- Every `TdsDataType` has an explicit test-matrix disposition.
- Every supported result type has value parity; nullable forms also have NULL parity.
- Explicit and terminal rejection behavior is documented and pinned.
- The matrix fails to compile when a new `TdsDataType` variant is added without a case.
- Targeted `mssql-tds` nextest coverage passes.
### Affected crate
mssql-tds
### Alternatives considered
Structural convergence was implemented and tested in #269. It was neutral before #264, but materially regressed the optimized production decoder after #264 and remained 6–9% slower on the common integer-heavy shape after the one allowed rescue attempt. It was therefore closed rather than trading measured throughput for maintainability.
The precomputed-plan approach from #249 was also implemented and benchmarked; its performance premise was refuted. Relying only on code review leaves the original silent-divergence risk intact.
Behavior-parity tests preserve current runtime/codegen while making drift observable in CI.
### Additional context
- Full #269 conclusion and measurements: https://github.com/microsoft/mssql-rs/pull/269#issuecomment-5294473728
- Tracking context and post-#264 methodology: #247
- Precomputed-plan experiments: #249
Residual top-level map verified during #269:
| Variant | Disposition |
|---|---|
| `Decimal` (`0x37`) | Explicit `UnimplementedFeature`; supported result metadata uses `DecimalN` |
| `Numeric` (`0x3F`) | Explicit `UnimplementedFeature`; supported result metadata uses `NumericN` |
| `Void` (`0x1F`) | Server does not emit `NULLTYPE` as a result-column type |
| `VarBinary` (`0x25`) | Legacy ID; result metadata uses `BigVarBinary` |
| `Binary` (`0x2D`) | Legacy ID; result metadata uses `BigBinary` |
| `SqlTable` (`0xF3`) | Outbound TVP parameter type, not a result-column type |
| `None` (`0x00`) | Internal sentinel; raw `0x00` fails `TdsDataType::try_from` |
**`StringDecoder` footnote — recorded, not proposed.** Its ~80-line `decode`
duplicate is technically separable: `main` already has `decode_string_into`, so
it could be defined through that method plus `CaptureWriter` without touching
`GenericDecoder::decode_into`. That means the measured hot-body growth from #269
does not directly apply, and the change is plausibly free.
It should still not be included here or treated as queued work. The parity matrix
addresses the same silent-drift hazard at zero runtime cost and without another
four-round benchmark/review cycle. This decoder path has repeatedly invalidated
changes that looked plausibly free; removing ~80 lines does not justify reopening
that measurement burden. Preserve this only as context for future readers.
`TdsDataType::Vector` is implemented. `V1`, `Float32`, and `Float16` are nested vector layout/base-type variants, not residual top-level `TdsDataType` variants.
Contributor guide
Research direction
Start with the TdsDataType enum, GenericDecoder::decode and decode_into, and the existing assert_decode_equivalence pattern in the mssql-tds crate. Reuse the in-crate reader, metadata builders, and writer fixtures, then run targeted mssql-tds nextest coverage. Done means every top-level variant has an explicit disposition, supported values and NULLs match, rejection behavior is pinned, and new enum variants require a test case.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- rust
- Domain
- databases, testing
- Issue type
- Feature
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Quiet
- Clarity
- Clearly specified
- Newbie friendliness
- 62/100