Add bounded typed SQLGetData conversions for max-length text

[David Engel (David-Engel)]

Description

Add SQLGetData conversions from varchar(max) and nvarchar(max) to supported numeric and date/time C targets. Reuse the bound-fetch 1 MiB source-data cap, draining oversized values and returning HYC00 without returning a truncated numeric prefix. After a character read, apply the cap to the unread suffix rather than the original PLP length. Typed reads and rejection drains reuse an 8 KiB scratch buffer inside one application call.

Typed decoding reserves output before consuming carry or decoder input and avoids temporary UTF-16 vectors/strings. Narrow decoder construction is fallible, with its nonzero allocation-size precondition enforced at compile time; allocation errors are retained as HY001 while draining. Empty final chunks flush incomplete input, without re-finalizing a decoder already finished during earlier character reads. Comments distinguish typed finalization from the direct character helpers' empty-final-input guard.

Preserve unread characters across target switches, including UTF-8 and raw UTF-16LE carry. Conversion errors retain decoded text for character/typed retries and a separate, source-cap-bounded copy of original unread wire bytes for binary retries and length probes. Binary and decoded-text retries advance independent offsets. Character switches and typed retries use the unread decoded suffix. Character recovery caches the requested encoding once, so repeated small WCHAR/CHAR reads use direct chunk delivery. Retry storage is cleared when the column is consumed, skipped, or its row is reset.

Match msodbcsql18 empty-text behavior for numeric/GUID retrieval while retaining 22018 for empty date/time literals. Nonempty text-to-GUID parsing remains an existing shared-converter gap for both max and non-max text (07006), not a capability introduced by this PR.

Unsupported typed binary-PLP requests retain their stream for supported retries; advancing to a later column drains the unread value through the TDS cursor. Added wire-level coverage for this existing behavior rather than changing it.

Merged main through aa28361b in cc59d7f1, preserving both main's NULL-buffer fixes and this branch's binary-PLP mock serialization.

Related Issues

AB#47238

Testing

CI: All reported checks passed on 16c634b7, including validation build 177806 and coverage reporting. Merge commit cc59d7f1 requires fresh CI; earlier green results are not claimed for the new head.

Latest local validation on Windows:

• cargo nextest run --frozen -p mssqlodbc -p mssql-mock-tds --lib: 1,805 passed after merging main, including NULL-buffer and binary-PLP recovery regressions.
• cargo clippy --frozen -p mssqlodbc -p mssql-mock-tds --all-features --all-targets -- -D warnings, cargo bfmt, and staged diff checks passed.

Earlier local validation:

• On 16c634b7, cargo nextest run --frozen -p mssqlodbc -p mssql-tds --lib -E 'test(typed_plp) | test(datatypes::sql_string)': 72 passed, including character-to-typed finalization and repeated empty-final completion coverage. Both crates' all-feature/all-target Clippy, formatting, and diff checks passed.
• On 0ea9eeea, all 1,779 ODBC unit tests and 22 mock-server tests passed, along with both crates' all-feature/all-target Clippy, formatting, and diff checks.
• Binary-PLP rejection coverage includes repeated HYC00, unchanged outputs, BINARY/CHAR/WCHAR retries before and after a partial binary read, skipping to a later column, subsequent rows, and a newly allocated statement on the same connection. Mock responses use actual varbinary(max) metadata and PLP framing.
• On 93f46f36, all 1,778 ODBC unit tests plus 46 core string/codec tests passed. Both crates' all-feature/all-target Clippy, ODBC DLL build, formatting, and diff checks passed.
• Allocation-failure coverage includes raw/decoded buffer reservations, UTF-16 carry, narrow decoder construction, wire decoding, and recovery rendering. Tests verify HY001, unchanged outputs, completed drain, and subsequent SQL_NO_DATA where applicable. UTF-16 output is compared against existing lossy decoding at every split in valid/malformed samples. Empty-final-chunk and previously-finalized decoder paths are covered.
• WCHAR recovery coverage verifies stable UTF-16 storage and no further reserve calls across 8,192 small reads, including split surrogates. This is structural/read-count evidence, not a latency benchmark.
• Exported-API retry coverage includes binary/CHAR/WCHAR switches, independent probes/offsets, unread-suffix typed conversion, malformed bytes, known/unknown totals, consumed prefixes, allocation recovery, and row/column cleanup. Earlier regressions reproduced the byte-representation, offset, suffix, and rendering-cache defects before their fixes.
• The terminal-error mock test executes, fetches, reads, and exhausts a newly allocated statement after the original statement releases the connection.
• Live SQL Server probes through the Rust DLL built at 93f46f36 passed for binary retries after typed errors, oversized-prefix switches, and terminal-error connection reuse for both varchar(max) and nvarchar(max).
• Windows C++ Driver Manager prefix-switch and terminal-error tests passed against msodbcsql 18.06.0001. The Rust Driver Manager run required elevated driver registration; direct Rust-export probes do not substitute for that CI leg.
• Initial Linux validation passed 1,705 ODBC unit tests, measured 91.7% changed production-line coverage, and passed complete get_data_test and fetch_scroll_test binaries against Rust and msodbcsql 18.06.0001 using an isolated SQL Server 2025 container.
• Python-core formatting was checked during earlier review fixes.

Full-workspace clippy and btest were not run locally. The checklist below records local execution, not CI status; prior successful CI is recorded separately above.

Breaking Changes / Migration

No breaking API, schema, or configuration changes. Adds a fallible narrow-decoder constructor in mssql-tds for ODBC materialization. Empty character values retrieved as numeric/GUID targets now return success with indicator 0 and leave the value buffer unchanged. Empty date/time literals still return 22018. No migration required.

Checklist

• cargo bfmt passes
• cargo bclippy passes
• cargo btest passes
• New/changed functionality has tests
• Public API changes are documented

[Copilot]

Copilot review overview

🟡 Changes recommended

Typed PLP conversion has unresolved allocation diagnostics, target-switch handling, and GUID conversion defects.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 2

Open (2)

• Base typed PLP size checks on remaining bytes after target switch · New
• Report PLP reserve failures as HY001, not materialization-limit HYC00 · New

What changed in this PR

Adds bounded typed SQLGetData conversions for max-length text, with updated diagnostics, tests, and documentation.

Changes:

• Supports numeric, GUID, and date/time PLP conversions.
• Enforces a shared 1 MiB materialization limit.
• Defines empty-text behavior and documents parity deviations.

Required fixes:

• Report allocation failures as HY001, not the size-limit HYC00.
• Apply the cap to the unread suffix after target switches.
• Implement non-empty text-to-GUID conversion with 22018 for invalid literals.
• Preserve pending_bytes when switching from character to typed targets.
• Add successful PLP GUID end-to-end coverage.

File	Description
mssql-odbc/​tests/​e2e/​tests/​get_data_test.cpp	Adds PLP conversion and boundary tests; successful GUID coverage is missing.
mssql-odbc/​src/​api/​sqlstate.rs	Adds the typed PLP limit diagnostic.
mssql-odbc/​src/​api/​get_data.rs	Implements typed PLP accumulation and conversion; contains the blocking issues above.
mssql-odbc/​src/​api/​fetch_scroll.rs	Exposes the shared PLP size-limit helper.
mssql-odbc/​README.md	Documents typed retrieval behavior.
mssql-odbc/​docs/​parity-deviations.md	Records the measured size-limit deviation.
CHANGELOG.md	Records the feature and behavior changes.

---

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[github-actions]

📊 Code Coverage Report

🔥 Diff Coverage 98%

🎯 Overall Coverage 94.3%

📦 Project: mssql-tds + mssql-odbc + mssql-py-core ℹ️ Note: diff coverage is reported, not enforced.

---

Diff Coverage

Diff: main...HEAD, staged and unstaged changes

• mssql-mock-tds/src/protocol.rs (100%)
• mssql-mock-tds/src/query_response.rs (66.7%): Missing lines 53
• mssql-odbc/src/api/fetch_scroll.rs (100%)
• mssql-odbc/src/api/get_data.rs (98.5%): Missing lines 1086,1133,1144,1155,1191,1272,1730-1732,1822-1823,2133,2160-2161,2474-2476,2486,2501-2502,3136,3153,3159,9266
• mssql-odbc/src/handles/stmt.rs (100%)
• mssql-tds/src/datatypes/sql_string/encoding.rs (96.7%): Missing lines 118

Summary

• Total: 1681 lines
• Missing: 26 lines
• Coverage: 98%

mssql-mock-tds/src/query_response.rs

  49             SqlDataType::SmallInt => 2,
  50             SqlDataType::Int => 4,
  51             SqlDataType::BigInt => 8,
  52             SqlDataType::NVarChar | SqlDataType::NVarCharMax => 255, // Handled specially
! 53             SqlDataType::VarCharMax | SqlDataType::VarBinaryMax => 255,
  54         }
  55     }
  56 }

---

mssql-odbc/src/api/get_data.rs

  1082     if value.encoding_type() == &encoding {
  1083         return Ok(());
  1084     }
  1085     let mut bytes = Vec::new();
! 1086     if target_type == SQL_C_WCHAR {
  1087         let text = std::str::from_utf8(&value.bytes).map_err(|_| ERR_INVALID_CHARACTER_VALUE)?;
  1088         reserve_typed_plp_bytes(&mut bytes, text.encode_utf16().count() * 2)?;
  1089         bytes.extend(text.encode_utf16().flat_map(u16::to_le_bytes));
  1090     } else {

---

  1129     // Normalize only on a target switch; same-target chunking keeps its offset
  1130     // and avoids repeatedly moving the remainder of a large value.
  1131     if offset != 0 {
  1132         let encoding = if previous_target == SQL_C_WCHAR {
! 1133             EncodingType::Utf16
  1134         } else {
  1135             EncodingType::Utf8
  1136         };
  1137         if value.encoding_type() != &encoding {

---

  1140         let byte_offset = if previous_target == SQL_C_WCHAR {
  1141             offset.saturating_mul(2)
  1142         } else {
  1143             offset
! 1144         };
  1145         value.bytes.drain(..byte_offset.min(value.bytes.len()));
  1146         // A byte/surrogate fragment is still readable in its original target.
  1147         // A different encoding or typed conversion must validate that fragment.
  1148         state.direct_text_target = Some((col_index, previous_target));

---

  1151     if let Some(wire) = state.captured_plp_wire.as_mut() {
  1152         wire.text_target = None;
  1153     }
  1154     Ok(())
! 1155 }
  1156 
  1157 fn write_captured_column(
  1158     stmt_state: &mut crate::handles::stmt::StmtState,
  1159     col_index: usize,

---

  1187 
  1188     if let Err(diag) = prepare_captured_plp_text(stmt_state, col_index, target_type) {
  1189         post_diag(stmt_state, diag);
  1190         return SQL_ERROR;
! 1191     }
  1192 
  1193     // Output buffer capacity in element units (u8 for SQL_C_CHAR, SqlWChar for
  1194     // SQL_C_WCHAR). buffer_length is always in bytes per the ODBC spec.
  1195     let buf_elements = if target_type == SQL_C_WCHAR {

---

  1268         // Measured on msodbcsql 18.6.2.1: `binary(9)` answers
  1269         // `SQLGetData(SQL_C_BINARY, NULL, 0)` with `SQL_SUCCESS_WITH_INFO` /
  1270         // `01004` / indicator 9, while an empty `varbinary` answers
  1271         // `SQL_SUCCESS` and reports `SQL_NO_DATA` on a repeat.
! 1272         let offset = captured_binary_offset(stmt_state, col_index);
  1273         let available = captured_column_bytes(stmt_state, col_index).map_or_else(
  1274             || remaining_binary_length(value, offset),
  1275             |bytes| SqlLen::try_from(bytes.len().saturating_sub(offset)).unwrap_or(SqlLen::MAX),
  1276         );

---

  1726     carry_before: usize,
  1727     retry_bytes: usize,
  1728     completing_surrogate: bool,
  1729     completing_narrow: bool,
! 1730     typed_bytes: Vec<u8>,
! 1731     typed_wire: Vec<u8>,
! 1732     typed_scratch: Vec<u8>,
  1733     typed_error: Option<DiagMsg>,
  1734 }
  1735 
  1736 /// Reads and returns one SQLGetData chunk directly from the active PLP stream.

---

  1818         if let Some((encoding, widen_carry_len, utf8_carry_len, narrow_encoding)) = prepared_stream
  1819         {
  1820             let decoder_ready = narrow_encoding.is_some();
  1821             let compatible = match (target_type, encoding) {
! 1822                 (_, PlpEncoding::Utf16Text | PlpEncoding::Utf8Text) if typed_target => true,
! 1823                 (_, PlpEncoding::SingleByteText) if typed_target => decoder_ready,
  1824                 (SQL_C_WCHAR, PlpEncoding::Utf16Text) => true,
  1825                 (SQL_C_WCHAR, PlpEncoding::SingleByteText | PlpEncoding::Utf8Text) => decoder_ready,
  1826                 (
  1827                     SQL_C_CHAR,

---

  2129     let carried_bytes = utf8_carry_len.saturating_add(widen_carry_len.saturating_mul(2));
  2130     if retry_bytes == 0 {
  2131         progress.carry_before = carried_bytes;
  2132     }
! 2133     let prefix_bytes = if !typed_target && target_type != SQL_C_BINARY && carried_bytes > 0 {
  2134         let mut state = match retained_stmt_state.take() {
  2135             Some(state) => state,
  2136             None => {
  2137                 let Ok(state) = stmt.inner.lock() else {

---

  2156             carried_bytes.min(capacity)
  2157         };
  2158         if emit > 0 {
  2159             if !stream.pending_units.is_empty() {
! 2160                 debug_assert!(stream.pending_bytes.is_empty() || stream.pending_bytes_utf16);
! 2161                 stream.pending_bytes_utf16 = true;
  2162             }
  2163             stream
  2164                 .pending_bytes
  2165                 .extend(stream.pending_units.drain(..).flat_map(u16::to_le_bytes));

---

  2470                 let Ok(state) = stmt.inner.lock() else {
  2471                     return SQL_ERROR;
  2472                 };
  2473                 state
! 2474             }
! 2475         };
! 2476         if progress.typed_error.is_none() {
  2477             // The PLP header includes bytes consumed by earlier character reads.
  2478             let consumed_before = total_read
  2479                 .saturating_sub(read)
  2480                 .saturating_sub(progress.wire_read);

---

  2482                 .and_then(|total| total.checked_sub(u64::try_from(consumed_before).ok()?));
  2483             if typed_plp_chunk_fits(progress.wire_read, read, remaining_total) {
  2484                 progress.wire_read += read;
  2485                 let Some(stream) = state.active_plp.as_mut() else {
! 2486                     post_diag(&mut state, ERR_INTERNAL_CONVERSION);
  2487                     return SQL_ERROR;
  2488                 };
  2489                 progress.typed_error = reserve_typed_plp_bytes(&mut progress.typed_wire, read)
  2490                     .and_then(|()| {

---

  2497                         )
  2498                     })
  2499                     .err();
  2500             } else {
! 2501                 progress.typed_error = Some(ERR_PLP_TYPED_LIMIT);
! 2502             }
  2503         }
  2504         progress.typed_scratch = heap_payload;
  2505         if !reached_end {
  2506             progress.retry_bytes = TYPED_PLP_CHUNK_BYTES;

---

  3132             let base = bytes.len();
  3133             bytes.resize(base + bound, 0);
  3134             let (result, consumed, written, _) =
  3135                 decoder.decode_to_utf8(payload, &mut bytes[base..], reached_end);
! 3136             bytes.truncate(base + written);
  3137             if result != encoding_rs::CoderResult::InputEmpty || consumed != payload.len() {
  3138                 return Err(ERR_INTERNAL_CONVERSION);
  3139             }
  3140             // Unlike the character helpers, typed decoding flushes empty final

---

  3149 fn append_typed_utf16(
  3150     payload: &[u8],
  3151     reached_end: bool,
  3152     pending_byte: &mut Option<u8>,
! 3153     pending_high: &mut Option<u16>,
  3154     output: &mut Vec<u8>,
  3155 ) -> Result<(), DiagMsg> {
  3156     let bound = payload
  3157         .len()

---

  3155 ) -> Result<(), DiagMsg> {
  3156     let bound = payload
  3157         .len()
  3158         .checked_add(2)
! 3159         .and_then(|len| len.checked_mul(3))
  3160         .ok_or(ERR_MEMORY_ALLOCATION)?;
  3161     reserve_typed_plp_bytes(output, bound)?;
  3162     let mut emit = |ch: char| {
  3163         output.extend_from_slice(ch.encode_utf8(&mut [0; 4]).as_bytes());

---

  9262                 unsafe { crate::api::fetch::sql_fetch(next_stmt) },
  9263                 SQL_NO_DATA
  9264             );
  9265             assert_eq!(
! 9266                 unsafe { crate::api::fetch::sql_fetch(handles.stmt) },
  9267                 SQL_NO_DATA
  9268             );
  9269         }
  9270     }

---

mssql-tds/src/datatypes/sql_string/encoding.rs

  114                 // and layout.
  115                 let decoder = unsafe {
  116                     let ptr = std::alloc::alloc(layout).cast::<Decoder>();
  117                     if ptr.is_null() {
! 118                         return None;
  119                     }
  120                     ptr.write(encoding.new_decoder_without_bom_handling());
  121                     Box::from_raw(ptr)
  122                 };

---

---

🔗 Quick Links

View Azure DevOps Build · Coverage Report

[David Engel (David-Engel)]

Implement non-empty text-to-GUID conversion with 22018 for invalid literals; preserve pending_bytes; add successful PLP GUID coverage.

The pending-byte path already preserves converted carry; 2b1dd65 adds an exported-entry-point regression to pin that behavior. Nonempty text-to-GUID parsing is an existing gap in the shared converter for both max and non-max text, so that converter work and successful GUID e2e coverage are deferred rather than introduced in this PLP-stream fix. I corrected the README, changelog, and PR description to remove the unsupported capability claim and added a regression showing that both paths retain the existing 07006 result without modifying outputs.

[Copilot]

Copilot review overview

🟡 Changes recommended

Unresolved carry-encoding, decoder-finalization, draining-performance, and allocation-test issues affect correctness and reliability.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1

Open (1)

• Prevent raw UTF-16 carry bytes from corrupting UTF-8 accumulation · New

Resolved since last review (2)

• Report PLP reserve failures as HY001, not materialization-limit HYC00
• Base typed PLP size checks on remaining bytes after target switch

[Copilot]

Copilot review overview

🟡 Changes recommended

A critical stream-state bug and two moderate allocation/performance issues remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1

Open (1)

• Terminal typed PLP errors leave the final column unconsumed · New

Resolved since last review (1)

• Prevent raw UTF-16 carry bytes from corrupting UTF-8 accumulation

[Copilot]

Copilot review overview

🟡 Changes recommended

Typed PLP handling still has scope, allocation-failure, regression-coverage, and drain-performance issues.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1

Open (1)

• Test HY001 propagation from an actual append failure · New

Resolved since last review (1)

• Terminal typed PLP errors leave the final column unconsumed

[Copilot]

Copilot review overview

🔵 Needs a closer look

Three moderate performance, correctness, and regression-test issues remain unresolved.

Review effort: Balanced
Findings: None

Resolved since last review (1)

• Test HY001 propagation from an actual append failure

Previously missed (1)

In code that hasn't changed since last review

256-byte PLP draining causes extreme CPU and latency costs

mssql-odbc/​src/​api/​get_data.rs:1931

Typed retrieval and its terminal-error drain are limited to 256 bytes per loop (and line 2365 repeats that size until EOF). A legal 2 GiB MAX value therefore makes one synchronous SQLGetData call perform roughly 8.4 million client polls and mutex cycles even when the known-length check rejects it on the first chunk. This makes the new bounded-failure path vulnerable to extreme CPU/latency costs. Please drain with a substantially larger reusable scratch buffer (or a dedicated PLP drain routine) once conversion is rejected, and avoid 256-byte reads for normal materialization as well.

[ttk (Theekshna)]

Unattended hourly review sweep (first pass on this PR from me). Fresh worktree verified at bca039e7 before diffing; merge-base b0cfd182 against main, 8 files, +1234/-33.

Read all prior discussion first (5 rounds of Copilot bot review, David-Engel's replies, and commits 2b1dd652/181aa299/861550f3/bca039e7). All four bot-flagged correctness defects are fixed and each has a targeted regression: the suffix-limit fast-rejection now bases the cap on bytes remaining after target switch (typed_plp_chunk_fits), HY001 vs HYC00 are correctly split by cause, wide carry is transcoded before typed accumulation instead of being appended as raw bytes, and terminal typed errors mark the column consumed so finish_get_data still releases the connection. I did not re-file any of these.

msodbcsql parity (check 1): the size-cap divergence (deviation 7 in docs/parity-deviations.md) cites odbc/sqlcdata.h's IsFixedOrBinaryWithFixedServerType gate before FetchDataWithCopy, with an explicit 18.06.0001 re-measurement ('0'×1048576+'1' → 01004/indicator 4) - meets the repo's "citation + measurement" bar in mssql-odbc.instructions.md §2.1. The new "empty text → success, indicator 0" claim (README/CHANGELOG) has no separate manual measurement, but EmptyTextTypedConversionsLeaveOutputUntouched carries no SKIP_IF_COMPARING_MSODBCSQL(), so per §2.1 it asserts on both the Rust and msodbcsql legs under --compare-with-msodbcsql in CI - that's the standard parity evidence path here, not a gap.

Test sufficiency (check 2): the 4 coverage-report misses (get_data.rs:2335,2350-2351,2948) are mutex-poison and internal-invariant guards matching the file's existing defensive style elsewhere (e.g. :2331), and :2948's PlpEncoding::Binary arm is already gated dead by the same compatibility check the comment above it describes. Not a new gap introduced by this PR.

Divergences documented (check 3): deviation 7 covers the size cap; the GUID gap is called out in both the README and a dedicated regression, consistent with existing convention rather than a new undocumented divergence.

Verbose slop (check 5): none found - new comments state real invariants (e.g. get_data.rs:2952's carry-representation note, the Binary-column dead-arm rationale).

Evidence audit (check 6): re-derived the boundary case myself by reading PlpTypedNullsAndChunkBoundaries (lengths 255/256/257/511 bracket the 256-byte read granularity) rather than accepting the description's count at face value; the local unit-test claim (1,763 passing) I could not re-run in this sandbox's time budget, so I'm not disputing it, only noting it's unverified here.

Severity	Count
Blocking	0
Suggestion	2
Nit	0

[Suggestion] PR description/checklist currency: it says "Full-workspace clippy and btest were not run locally; their checklist items remain unchecked. CI must validate the latest commit" - but gh pr checks 643 is now fully green at this exact head (bca039e7), including both cross-repo mssql-python legs and the coverage report. Worth a follow-up edit so a reviewer doesn't discount CI that has already validated this commit.

This is an unattended automated review; findings may be incomplete or wrong, push back on anything that looks off.

[Copilot]

Copilot review overview

🟡 Changes recommended

Critical allocation handling and malformed-suffix decoding issues remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1

Open (1)

• Typed materialization uses infallible allocations before failure handling · New

[Copilot]

Copilot review overview

🟡 Changes recommended

The critical binary PLP stream-state issue can desynchronize subsequent row reads.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1

Open (1)

• Validate PLP encoding before publishing streams to prevent row desynchronization · New

Resolved since last review (1)

• Typed materialization uses infallible allocations before failure handling

[Copilot]

Copilot review overview

🔵 Needs a closer look

The streaming conversion and retry-state changes require final human review, and a stale test rationale remains.

Review effort: Balanced
Findings: None

Resolved since last review (1)

• Validate PLP encoding before publishing streams to prevent row desynchronization

[ttk (Theekshna)]

Unattended hourly review sweep. Fresh worktree verified at 0ea9eeea before diffing; merge-base cc7bb82e against main. Re-reviewed the 7 commits pushed since my last pass (bca039e7): 58122a94..0ea9eeea, all on the typed-PLP-retry/recovery path in get_data.rs plus the new CapturedPlpWire state and try_new_decoder_without_bom_handling.

Read the full discussion first, including my own prior review and the bot/David-Engel back-and-forth on the reserve-hook regression, the 8 KiB chunk reuse, the WCHAR-recovery quadratic-cost finding, and the infallible-decoding-allocations finding. Each has a commit that fixed it and a targeted regression; I did not re-file any of them.

msodbcsql parity (check 1): N/A for this incremental diff — no new user-visible SQLSTATE/behavior change; this is materialization/retry bookkeeping (CapturedPlpWire, fallible decoder construction) behind behavior already covered by deviation 7 / the empty-text README note from the prior round.

Test sufficiency (check 2): the new typed_binary_plp_rejection_preserves_retry_and_following_columns test is parametrized over 4 retry targets × prefix-read/no-prefix, and asserts on a second row/following column — real coverage, not a single happy-path case. I did not rebuild+mutate it in this pass (budget); flagging that as unverified rather than asserting it's sufficient.

Divergences documented (check 3): none newly introduced.

PR description (check 4): matches the diff; AB#47238 linked. Checklist still shows bclippy/btest unchecked and CI (gh pr checks 643) is not yet fully green on this head (coverage-report timed out waiting on the cross-repo artifact — looks like the known ADO-artifact-latency infra issue, not a code problem; Build/Test legs otherwise passed). That's consistent with the description's own caveat, not drift.

Verbose slop (check 5): none found.

Evidence audit (check 6): re-derived the 8 KiB vs 256 B chunk-size claim (TYPED_PLP_CHUNK_BYTES = 8*1024) — 8192/256 = 32×, matching David-Engel's "32× fewer iterations" reply. Did not re-derive the 1,824/1,801-test counts locally.

Two new source-read-only findings on the new code (not mutation-tested, so kept as Suggestions per this run's confidence cap):

Severity	Count
Blocking	0
Suggestion	2
Nit	0

This is an unattended automated review; findings may be incomplete or wrong, push back on anything that looks off.

[Copilot]

Copilot review overview

🔵 Needs a closer look

The extensive stateful conversion and retry behavior requires final human review.

Review effort: Balanced
Findings: None

[ttk (Theekshna)]

Unattended review sweep. The single commit since my last pass (16c634b7, "Enforce decoder allocation and document finalization invariants") resolves both outstanding Suggestions from that review: a compile-time size_of::<Decoder>() > 0 assertion now backs the SAFETY comment in encoding.rs, and the narrow_decoder_finished asymmetry between the direct and typed paths is documented and pinned by a new regression (typed_plp_decoders_flush_empty_final_chunks_without_allocating_temporaries) that verifies repeated completion doesn't decode twice. Covered this round: msodbcsql parity (N/A — no user-visible behavior change in this delta), divergence docs (none new), PR description/CI currency (checklist caveats still match gh pr checks 643), and slop (none — the new comments record a real invariant, not restated code).

Severity	Count
Blocking	0
Suggestion	0
Nit	0

This is an unattended automated review; findings may be incomplete or wrong, push back on anything that looks off.

[Copilot]

Copilot review overview

🔵 Needs a closer look

Stateful streaming conversion, retry recovery, and allocation-error handling require final human review.

Review effort: Balanced
Findings: None

[ttk (Theekshna)]

Unattended review — PR #643

Delta since my last marked pass (16c634b7, findings=0): the single new commit cc59d7f1 merges main through aa28361b (PR #645, "Preserve character output buffers when returning NULL data"). Verified this merge did not introduce a hidden interaction with PR 643's own typed-conversion code in get_data.rs/fetch_scroll.rs:

• The merged NULL early-return in write_captured_column (get_data.rs) fires before this PR's typed-target dispatch and does not touch target_value_ptr at all, so the "leave the buffer untouched" behavior is target-type-agnostic by construction — the existing get_data_null_reports_null_data_for_any_valid_target test (SQL_C_BINARY) already covers this class; no separate typed-target NULL test is needed.
• query_response.rs's own diff on top of the merge is purely additive (VarBinaryMax), doesn't touch the merged NVarCharMaxNull variant or its write_to_buffer arm.
• No textual or semantic conflict between the two branches in any of the three shared files (get_data.rs, fetch_scroll.rs, query_response.rs).

msodbcsql parity: N/A for this delta — the merged NULL-handling change is already-reviewed code from #645 (cites sqlcdata.h's InternalGetColData Return2/Return3 split), and this PR's own addition here is a mock-only VarBinaryMax variant with no parity surface. Divergence docs, description currency, and slop: no new issues — the description accurately states CI must re-run fresh at cc59d7f1 rather than claiming the prior green result still applies.

[Suggestion] gh pr checks 643 (build 177898) currently shows mssql-rs Pull request validation (Build Stage Test MacOS) as failed, while the same stage passed at the previously-reviewed head (16c634b7). Several other stages (Linux, Linux ARM, Windows, SQL host build_arm, mssql-python macOS cross-repo) are still pending, so the pipeline hasn't fully concluded. I don't have access to the ADO build log to determine whether this is a real regression from the merge or a MacOS-runner flake — please check build 177898 before merging.

Severity	Count
Blocking	0
Suggestion	1
Nit	0

This is an unattended automated review; findings may be incomplete or wrong, push back on anything that looks off.

[ttk (Theekshna)]

Unattended review sweep. No new commits since my last pass (cc59d7f1); re-triggered because all prior review threads on this PR are now marked resolved. Re-verified rather than re-diffed, since the code at this head is unchanged from what I already reviewed.

Confirmed in source (not re-asserted from memory): the size_of::<Decoder>() > 0 compile-time assertion backing the SAFETY comment is present at encoding.rs:109; the narrow_decoder_finished asymmetry between the direct (|= reached_end && read != 0) and typed (= reached_end) paths now carries an explanatory comment at get_data.rs:3141-3142 ("Unlike the character helpers, typed decoding flushes empty final input too; the guard above prevents finalizing that decoder twice") and is pinned by typed_plp_decoders_flush_empty_final_chunks_without_allocating_temporaries. Both were the two open Suggestions from my earlier rounds; neither is being re-filed.

The only outstanding item from my previous pass (findings=1, this same head) was CI status: mssql-rs Pull request validation (Build Stage Test MacOS) showed as failed on build 177898. Rechecked now — that stage is pending (build re-ran it rather than reporting a hard failure), and every other stage on this build (Linux, Linux ARM, Windows, Windows ARM, SQL host build_arm, Kerberos, both mssql-python cross-repo legs, CodeQL, coverage-report) is green. The overall mssql-rs Pull request validation check is still pending on the Test MacOS leg, so this isn't fully concluded — flagging as informational, not as a new finding, since it's a status improvement over my last review rather than a regression.

Attempted a local mutation-style verification this round (cargo nextest run --offline -p mssqlodbc --lib) but the sandbox toolchain failed to compile crc32fast (E9012/E9013: codegen not yet implemented for llvm.x86.pclmulqdq/avx512 intrinsics) — a pre-existing environment/toolchain limitation unrelated to this PR's code, not a build regression. Did not attempt to work around it; relying on CI's own build/test legs, which are green as noted above.

msodbcsql parity, divergence docs, PR description currency, slop: no change from my prior passes — nothing here to re-flag.

Severity	Count
Blocking	0
Suggestion	0
Nit	0

This is an unattended automated review; findings may be incomplete or wrong, push back on anything that looks off.

[ttk (Theekshna)]

Unattended hourly re-review found no findings for this delta. Covered the merge of main (aa28361b, "Preserve character output buffers when returning NULL data") plus 16c634b7 since my last pass — the NULL-terminator-suppression behavior change across get_data.rs, fetch_scroll.rs, and output_params.rs, and the decoder-size compile-time assert in encoding.rs.

msodbcsql parity independently verified, not accepted from the PR's own comment: sqlcdata.h InternalGetColData sets the NULL indicator and jumps to Return2: (line 529) when fIsNullData, skipping the StringType NUL-terminator write that only happens at Return3: (lines 1177-1196). The new Rust behavior (leave the buffer untouched on NULL) matches; the removed behavior (writing an empty terminator) was the actual divergence.

Test coverage for the new behavior is parametrized across SQL_C_CHAR/SQL_C_WCHAR, multiple capacities, and buffered/captured/bound/output-param delivery paths, and separately asserts split indicator/octet-length pointers stay independent. Could not mutate-test locally: cargo nextest run -p mssqlodbc --lib fails to build in this environment on a pre-existing AVX-512 codegen error in crc32fast (reproduced on unmodified main in an earlier pass on PR #599, not caused by this PR); deferred to gh pr checks 643, now fully green on this head. CHANGELOG documents the behavior change with a rationale and issue reference (#555).