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
Conversation
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
Saurabh Singh (saurabh500)
marked this pull request as ready for review
August 10, 2026 06:38
Copilot started reviewing on behalf of
Saurabh Singh (saurabh500)
August 10, 2026 06:38
View session
Contributor
There was a problem hiding this comment.
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_variantcells. - 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/VarCharwith a two-byte prefix, while these wire types use a one-byte BYTELEN prefix (sqldatatypes.rs:467-470). Split them from the modernBig*/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<()> { |
Saurabh Singh (saurabh500)
marked this pull request as draft
August 10, 2026 06:49
Saurabh Singh (saurabh500)
marked this pull request as ready for review
August 10, 2026 07:39
Saurabh Singh (saurabh500)
marked this pull request as draft
August 10, 2026 13:29
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description
Inverts the entire non-PLP column decode path into a column-atomic, re-drivable synchronous
step(), moving the.awaitrefill 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'sNeedBytes+ L3's sync token decode were built to eliminate) without introducing any new resumable state machine —RowPauseState.next_column_indexremains the only resume granularity andNeedBytes { shortfall }the only suspension signal.What this lands (6 commits, green at each)
edc7aa83— invert non-PLP fixed-width column decode to a column-atomicstep()overcolumn_wire_len(peek) + whole-cellensure+decode_column_body(infallibletake_*).50dae22d— extend the step to variable-length non-PLP cells (char/varchar/nchar/nvarcharnon-max,binary/varbinarynon-max,decimal/numeric).952db191— extend the step toSQL_VARIANTcells.0d5e9619— converge the async row decoder onto the single shareddecode_column_bodyso no non-PLP leaf logic is duplicated betweendecoder.rsandsync_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_columnowns 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 onewrite_*). Because steps 1–2 only peek, aNeedBytesre-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 insync_decoder.rs; two refill drivers (syncPacketReader, asyncGenericDecoder::decode_into) wrap it and differ only by the.awaiton refill.Out of scope (unchanged)
PlpColumnStream/PlpChunkStreamReader, incl. AE overvarbinary(max)) — L4b, behind the existing pause branch.Tests
Five mandatory boundary tests drive the buffer-owning test
PacketReader, so they genuinely exercisedecode_column_body: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.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.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.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,ensureon the PLP column's first bytes returnsNeedBytes, refill happens, and the still-async PLP path resumes atnext_column_index. Asserts byte-identical to the single-packet baseline — the precise whole-column seam the hybrid window depends on.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
PacketReaderto be faithful to the production transport:receive_packetnow validates the header length, returns the single packet's length, and carries surplus bytes forward viarecord_pending(previously it returned the raw coalesced byte count and stripped only the first header).TestPacketBuildernow writes the TDS total length (header + payload) into the header field, matching the production serializer andvalidate_packet_length.Related Issues
N/A — internal sans-I/O refactor, no tracking issue (consistent with #189/#191/#194).
Checklist
cargo bfmtpassescargo bclippypassescargo btestpasses (lib green; failing-name set byte-for-byte identical to base = 359 pre-existing env/cert failures)