Skip to content

Sans-I/O TDS core (4/N): invert the non-PLP column decode to a column-atomic step() - #195

Draft
Saurabh Singh (saurabh500) wants to merge 8 commits into
dev/saurabh/sans-io-l3-token-decoderfrom
dev/saurabh/sans-io-l4a-fixed-width-step
Draft

Sans-I/O TDS core (4/N): invert the non-PLP column decode to a column-atomic step()#195
Saurabh Singh (saurabh500) wants to merge 8 commits into
dev/saurabh/sans-io-l3-token-decoderfrom
dev/saurabh/sans-io-l4a-fixed-width-step

Conversation

@saurabh500

@saurabh500 Saurabh Singh (saurabh500) commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

⚠️ Perf-neutral layer. Part 4 of the green-at-every-step sans-I/O stack. Base: dev/saurabh/sans-io-l3-token-decoder (#194). Public API is unchanged; mssql-odbc and mssql-py-core build unchanged.

Description

Inverts the entire non-PLP column decode path into a column-atomic, re-drivable synchronous step(), moving the .await refill point out of the middle of a column to the column boundary. This removes the last class of mid-value suspension in the row decode path (the risk L2's NeedBytes + L3's sync token decode were built to eliminate) without introducing any new resumable state machine — RowPauseState.next_column_index remains the only resume granularity and NeedBytes { shortfall } the only suspension signal.

What this lands (6 commits, green at each)

  • edc7aa83 — invert non-PLP fixed-width column decode to a column-atomic step() over column_wire_len (peek) + whole-cell ensure + decode_column_body (infallible take_*).
  • 50dae22d — extend the step to variable-length non-PLP cells (char/varchar/nchar/nvarchar non-max, binary/varbinary non-max, decimal/numeric).
  • 952db191 — extend the step to SQL_VARIANT cells.
  • 0d5e9619 — converge the async row decoder onto the single shared decode_column_body so no non-PLP leaf logic is duplicated between decoder.rs and sync_decoder.rs.
  • ca80c4ab — fold the Always-Encrypted non-PLP ciphertext whole-cell buffer into the sync step.
  • 1d46dd27 — mandatory refill-boundary tests (below).

Design

decode_or_decrypt_column owns buffering for all non-PLP cells, so it is inverted as one unit — there is no fixed-width-only seam inside it, and a partial inversion would fork it into a half-sync/half-async mid-column path. Per column the driver runs: column_wire_len (peek-only length) → ensure(total) (whole cell) → decode_column_body (pure sync, exactly one write_*). Because steps 1–2 only peek, a NeedBytes re-drive after refill re-runs from the column start with nothing consumed and nothing written → atomic.

decode_column_body (+ column_wire_len + the sync leaf ports) has exactly one definition in sync_decoder.rs; two refill drivers (sync PacketReader, async GenericDecoder::decode_into) wrap it and differ only by the .await on refill.

Out of scope (unchanged)

  • PLP/LOB streaming (PlpColumnStream / PlpChunkStreamReader, incl. AE over varbinary(max)) — L4b, behind the existing pause branch.
  • NBCROW body — L4c; the sync step is written so it can be reused verbatim there.

Tests

Five mandatory boundary tests drive the buffer-owning test PacketReader, so they genuinely exercise decode_column_body:

  1. non-PLP → PLP transition at every refill boundary (nonplp_to_plp_transition_is_byte_identical_across_refill_boundary) — a [int4][nvarchar(max) PLP] row swept with the packet boundary at each interior offset; the inverted step fully consumes the int4, then the PLP column resumes byte-identically across every split.
  2. mid-cell re-drive (var_length_cell_redrive_is_byte_identical_across_refill_boundary) of an inverted var-length cell, including inside the 2-byte length prefix (peek-only re-drive) and mid-data.
  3. hybrid mid-row seam (mixed_row_inverted_then_plp_is_byte_identical_across_refill_boundary) — a [int4][varchar][nvarchar(max) PLP] row that mixes two inverted non-PLP cells with a not-yet-inverted (legacy async) PLP cell in ONE row, swept across every offset (int4→varchar seam, inside the varchar length prefix, and the varchar→PLP handoff). Proves the inverted step and the async PLP path share one coherent row cursor (next_column_index) regardless of where the packet boundary lands — the L4a→L4b hybrid-window correctness guarantee.
  4. exact whole-column non-PLP → PLP transition (refill_boundary_at_nonplp_to_plp_column_transition_resumes_into_async_plp) — same [int4][varchar][nvarchar(max) PLP] row, but the packet boundary is forced to land exactly at the varchar→PLP column transition: the inverted step fully consumes the varchar cell, ensure on the PLP column's first bytes returns NeedBytes, refill happens, and the still-async PLP path resumes at next_column_index. Asserts byte-identical to the single-packet baseline — the precise whole-column seam the hybrid window depends on.
  5. inverted var-length cell own re-drive (refill_boundary_within_inverted_varchar_cell_resumes_coherently) — the boundary is swept inside the inverted varchar cell (within its 2-byte length prefix and mid-data), proving the inverted step's own re-drive from the column start is coherent independent of the PLP handoff.

Making these meaningful required fixing the test PacketReader to be faithful to the production transport: receive_packet now validates the header length, returns the single packet's length, and carries surplus bytes forward via record_pending (previously it returned the raw coalesced byte count and stripped only the first header). TestPacketBuilder now writes the TDS total length (header + payload) into the header field, matching the production serializer and validate_packet_length.

Related Issues

N/A — internal sans-I/O refactor, no tracking issue (consistent with #189/#191/#194).

Checklist

  • cargo bfmt passes
  • cargo bclippy passes
  • cargo btest passes (lib green; failing-name set byte-for-byte identical to base = 359 pre-existing env/cert failures)
  • New/changed functionality has tests
  • Public API changes are documented (no public API change)

Lift the .await out of the middle of a non-PLP column to the column
boundary for the fixed-width / N-prefixed numeric, temporal, money, and
GUID families. The row path now decodes these cells with a re-drivable,
side-effect-free synchronous step over PacketBuffer::ensure() + take_*.

- New datatypes/sync_decoder.rs owns the single decode_column_body plus
  column_wire_len: column_wire_len peeks (never consumes) the length
  prefix to compute total wire width; once the whole cell is buffered
  decode_column_body consumes it with infallible take_* and writes
  exactly one value/null. Peek-only length means a NeedBytes shortfall
  re-drives from the column start with nothing consumed or written, so
  RowPauseState.next_column_index stays the sole resume point (no new
  pause/step machine).
- PacketBuffer gains peek_bytes() (non-consuming length-prefix view) and
  from_bytes() (transient buffer for the async assembly driver).
- TdsPacketReader gains decode_column_into with a default async-assembly
  driver (buffer-less readers) reusing the same decode_column_body;
  PacketReader and NetworkTransport override it to drive the sync core in
  place over their owned buffer (zero copy) using the existing
  forward-progress ensure guard.
- decode_or_decrypt_column routes supported non-PLP cells through
  reader.decode_column_into; PLP and not-yet-ported types keep the legacy
  async decode_into, so decoded bytes are byte-identical.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 560cc263-b4bd-4a00-964f-0205dab149b8
Widen the inverted non-PLP column step to cover variable-length,
non-PLP strings and binary: Char/VarChar, BigChar/BigVarChar,
NChar/NVarChar (non-max), and BigBinary/BigVarBinary (non-max). These
carry a 2-byte USHORT length prefix with 0xFFFF as the NULL marker.

- is_supported and column_wire_len gain the USHORT-prefixed family:
  column_wire_len peeks 2 bytes and returns 2 for the NULL marker, else
  2 + length. Long-length LOB strings (Text/NText) stay on the legacy
  async path, and PLP cells remain excluded by the is_plp() gate.
- decode_column_body decodes the cell in place: strings via
  SqlString::new(bytes, get_encoding_type(meta)); binary via write_bytes.
- take_bytes copies the (fully-buffered) cell body with a
  forward-progress debug_assert.

The prefix is peek-only, so a shortfall still re-drives from the column
start with nothing consumed or written; RowPauseState.next_column_index
stays the sole resume point. All three refill drivers are generic over
column_wire_len, so they pick up the new family unchanged. Decoded bytes
are byte-identical to the async path (359==359 gate held).

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 560cc263-b4bd-4a00-964f-0205dab149b8
SQL_VARIANT is bounded-peekable: a 4-byte ULONG length prefix gives the
total wire width (4 + length), so it folds into the same peek-then-ensure
column step with no new resume need. Port read_sql_variant and its
prop-byte sub-decoders (0/1/2/7) to sync take_* helpers over PacketBuffer,
mirroring the async wire format exactly. Make MAX_ALLOC_SIZE pub(crate)
so the sync port can share the same allocation guard.

Add focused unit tests for take_sql_variant covering the zero-, one-, and
two-prop-byte paths plus the invalid-prop-count error, since the baseline
workload gives SQL_VARIANT no green-gate coverage.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 560cc263-b4bd-4a00-964f-0205dab149b8
decode_into duplicated every non-PLP leaf decode that now lives in the
sync column-atomic path. Route all sync-supported non-PLP cells through
reader.decode_column_into (the shared decode_column_body), leaving
decode_into to own only PLP strings/binary, Text/NText LOB, and the rare
decode() fallback. No non-PLP leaf logic survives in two places, so the
sync and async row paths cannot drift.

The writer parameter becomes a concrete &mut (dyn RowWriter + Send); all
callers already pass a Send writer, and the async leaf helpers stay in use
via the ColumnValues-returning decode() path.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 560cc263-b4bd-4a00-964f-0205dab149b8
The AE ciphertext arrives as non-PLP varbinary. Route it through the same
column-atomic sync step (reader.decode_column_into) to buffer + decode the
cipher bytes at the column boundary, then run the already-synchronous cell
decryptor and write the plaintext. PLP ciphertext (varbinary(max)) stays on
the async path (L4b).

Split decrypt_encrypted_column into the async cipher decode and a shared,
synchronous decrypt_cipher_value so both the sync and async cipher paths
converge on one decrypt + plaintext step.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 560cc263-b4bd-4a00-964f-0205dab149b8
Cover the two seams the fixed-width-heavy baseline never exercises:

- non-PLP -> PLP transition at every packet refill boundary: a mixed
  [int4][nvarchar(max) PLP] row is swept with the boundary landing at
  each interior offset. The inverted sync step fully consumes the int4,
  then the PLP column resumes byte-identically to the single-packet
  decode across every split.
- mid-cell re-drive of an inverted variable-length non-PLP cell: an
  nvarchar cell is split at every offset, including inside the 2-byte
  length prefix (peek-only re-drive from the column start) and mid-data
  (whole-cell ensure across the boundary).

Both drive the buffer-owning test PacketReader, so they genuinely
exercise decode_column_body rather than an async delegate.

Make the test PacketReader faithful to the production transport so the
boundary sweeps are meaningful: receive_packet now validates the header
length, returns that single packet's length, and carries any surplus
bytes forward via record_pending (previously it returned the raw
coalesced byte count and stripped only the first header, embedding a
following packet's header as data). TestPacketBuilder now writes the
TDS total length (header + payload) into the header field, matching the
production serializer and validate_packet_length.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 560cc263-b4bd-4a00-964f-0205dab149b8
During the L4a->L4b window a single row can mix inverted non-PLP cells (new
sync step() over ensure()+take_*) with a not-yet-inverted PLP cell (legacy
async decode_into). Cover the seam directly: a [int4][varchar][nvarchar(max)
PLP] row swept with the refill boundary at every interior offset — including
the int4->varchar seam, inside the varchar USHORT length prefix, and the
varchar->PLP handoff — asserting byte-identical decode to the single-packet
baseline. This proves the inverted step and the async PLP path share one
coherent row cursor (next_column_index) regardless of packet boundary.

Test-only; no production change. Extends the existing 2-column
nonplp_to_plp boundary test with a variable-length inverted cell adjacent to
the PLP cell.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 560cc263-b4bd-4a00-964f-0205dab149b8
Refactor the mixed-row seam test into module-level helpers and add two
explicitly-named targeted tests that land the refill/packet boundary at the
exact hybrid seam the coordinator flagged:

- refill_boundary_at_nonplp_to_plp_column_transition_resumes_into_async_plp:
  splits EXACTLY at the varchar->PLP whole-column transition. The inverted
  non-PLP step fully consumes the varchar cell, ensure() on the PLP column's
  first bytes returns NeedBytes, and the still-async PLP path resumes at
  next_column_index. Asserts byte-identical to the single-packet baseline.
- refill_boundary_within_inverted_varchar_cell_resumes_coherently: sweeps the
  boundary inside the inverted varchar cell (length prefix + mid-data) to prove
  the inverted step's own re-drive is coherent.

Test-only change; production decode path untouched. Both paths share one
RowPauseState.next_column_index column cursor; zero new resumable machines.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 560cc263-b4bd-4a00-964f-0205dab149b8

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Moves non-PLP TDS column decoding into a shared synchronous, column-atomic core while retaining async refill drivers.

Changes:

  • Adds synchronous decoding for fixed, variable-length, encrypted, and sql_variant cells.
  • Routes test and production packet readers through whole-cell decoding.
  • Adds refill-boundary and buffer atomicity tests.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
mssql-tds/src/datatypes.rs Registers the synchronous decoder module.
mssql-tds/src/datatypes/decoder.rs Shares decoding and decryption logic.
mssql-tds/src/datatypes/sync_decoder.rs Implements column-atomic decoding.
mssql-tds/src/io/token_stream.rs Routes row columns through the new path.
mssql-tds/src/io/packet_reader.rs Adds the column-decoding driver and boundary tests.
mssql-tds/src/io/packet_buffer.rs Adds non-consuming peeks and staged buffers.
mssql-tds/src/connection/transport/network_transport.rs Drives synchronous decoding over production buffering.
Suppressed comments (1)

mssql-tds/src/datatypes/sync_decoder.rs:302

  • The body decoder also reads legacy Char/VarChar with a two-byte prefix, while these wire types use a one-byte BYTELEN prefix (sqldatatypes.rs:467-470). Split them from the modern Big*/Unicode arm and decode their one-byte framing with the appropriate BYTELEN null semantics; otherwise every valid value consumes the wrong bytes.
        TdsDataType::Char
        | TdsDataType::VarChar
        | TdsDataType::BigChar
        | TdsDataType::BigVarChar
        | TdsDataType::NChar
        | TdsDataType::NVarChar => {

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment on lines +113 to +123
if matches!(
meta.data_type,
TdsDataType::Char
| TdsDataType::VarChar
| TdsDataType::BigChar
| TdsDataType::BigVarChar
| TdsDataType::NChar
| TdsDataType::NVarChar
| TdsDataType::BigBinary
| TdsDataType::BigVarBinary
) {
Comment on lines +140 to +144
return match buf.peek_bytes(4) {
Some(prefix) => {
let length = u32::from_le_bytes([prefix[0], prefix[1], prefix[2], prefix[3]]);
Ok(4 + length as usize)
}
Comment on lines +1103 to +1106
match sync_decoder::column_wire_len(&self.tds_read_buffer, meta) {
Ok(total) => {
self.ensure_or_refill(total).await?;
return sync_decoder::decode_column_body(
Comment on lines +288 to +291
match sync_decoder::column_wire_len(&self.buffer, meta) {
Ok(total) => {
self.ensure(total).await?;
return sync_decoder::decode_column_body(&mut self.buffer, meta, col, writer);
Comment on lines +489 to +490
let length = buf.take_u32_le()?;
let variant_base_type = buf.take_u8()?;
Comment on lines +162 to +167
pub(crate) fn decode_column_body(
buf: &mut PacketBuffer,
meta: &ColumnMetadata,
col: usize,
writer: &mut (dyn RowWriter + Send),
) -> TdsResult<()> {
@saurabh500
Saurabh Singh (saurabh500) marked this pull request as draft August 10, 2026 06:49
@saurabh500
Saurabh Singh (saurabh500) marked this pull request as ready for review August 10, 2026 07:39
@saurabh500
Saurabh Singh (saurabh500) marked this pull request as draft August 10, 2026 13:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants