Convert varchar(max)/text PLP chunks from the server collation to UTF-8 (AB#47566) - #533
Conversation
SQL_C_CHAR output is UTF-8, but PLP delivery of a SingleByteText column copied the wire bytes verbatim, so a non-UTF-8 collation handed the caller raw code page bytes labelled UTF-8. The non-PLP path already decoded through the collation via SqlString::to_utf8_string, so the same value delivered inline and streamed disagreed. Decode SingleByteText through the column's collation on both fetch paths. SQLGetData reuses the stream decoder that already served the SQL_C_WCHAR widening, so a multi-byte DBCS sequence split across a chunk boundary is carried rather than corrupted, and pending_utf8 holds back output the caller's buffer had no room for. The bound path in fetch_scroll.rs carried the identical verbatim copy and is converted the same way. Because converted length no longer equals wire length, this arm now reports SQL_NO_TOTAL like the other transcoding arms, and it can no longer read straight into the caller's buffer: the wire bytes are the decoder's input, not its output. A UTF-8 collation stays on the verbatim path so json and UTF-8 varchar(max) are not double-converted. Fixes the mssql-python failure in AB#47875, where the raw CP1252 bytes made that driver's strict UTF-8 decode fall back to returning bytes. AB#47566 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
Target-type switching can strand pending output, and the bound tests do not reach the changed streaming path.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Converts streamed non-UTF-8 varchar(max) data to UTF-8 across SQLGetData and bound fetch paths.
Changes:
- Adds incremental collation-aware transcoding.
- Preserves overflow output and reports
SQL_NO_TOTAL. - Adds unit and end-to-end regression tests.
File summaries
| File | Description |
|---|---|
mssql-odbc/src/api/get_data.rs |
Implements streamed UTF-8 transcoding. |
mssql-odbc/src/api/fetch_scroll.rs |
Adds transcoding for bound PLP columns. |
mssql-odbc/src/handles/stmt.rs |
Generalizes incremental decoder state. |
mssql-odbc/tests/e2e/tests/get_data_test.cpp |
Adds SQLGetData regressions. |
mssql-odbc/tests/e2e/tests/fetch_scroll_test.cpp |
Adds bound-fetch regressions. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 4
- Review effort level: Balanced
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Vahid (Vahid-b)
left a comment
Vahid (Vahid-b)
left a comment
There was a problem hiding this comment.
Summary
This fixes a genuine data-corruption defect — streamed varchar(max)/text PLP chunks delivered as SQL_C_CHAR were copied verbatim, handing back raw code-page bytes labelled UTF-8 — and correctly fixes the byte-identical latent twin in the bound path. The transcoding core is well-built and well-tested; four mutation runs confirm the unit tests actually guard narrow_max_read and the decoder carry.
One change rides along that I believe is wrong and will fail PR validation: making this arm report SQL_NO_TOTAL. AB#47566 scope item 4 asked for it, but the msodbcsql source says the opposite for narrow→narrow, and two existing unskipped e2e tests assert the opposite — one of them explicitly as a cross-leg parity assertion. Details inline at get_data.rs:2462 and fetch_scroll.rs:1817.
Verification
Worktree at e4366e34, BASE=b951a2d8, CARGO_TARGET_DIR redirected so the main worktree's cache is untouched.
cargo nextest run -p mssqlodbc --lib --no-fail-fast→ 1400 passed, 0 failed (2.2 s)cargo fmt -- --check→ clean;cargo clippy -p mssqlodbc --all-features --all-targets -- -D warnings→ clean- Mutations run:
| Mutation | Result |
|---|---|
(remaining / 3).max(1) → remaining.max(1) |
1 FAIL — narrow_max_read_floors_to_whole_characters ✅ |
decode_to_utf8(..., reached_end) → ..., true) (kills the carry) |
3 FAIL — converts_cp1252_to_utf8, carries_a_character_across_a_chunk_boundary, is_chunk_size_invariant ✅ |
drop the UTF-8 exemption at get_data.rs:1739 |
1400 passed ❌ |
neutralize the whole fetch_scroll.rs bound arm |
1400 passed ❌ |
- Probe (measured):
encoding_rs::GBK's decoder is the gb18030 decoder, so with a 3-byte carry one wire byte emits 4 UTF-8 bytes ([94,39,FC,36]→ 😀). - msodbcsql read at
Sql/Ntdbms/sqlncli/odbc/sqlcdata.h:1230-1250. - AB#47566 rev 12 and AB#47875 rev 10 fetched and read in full.
Not verified: the CI database's default collation is inferred (no MSSQL_COLLATION anywhere in .pipeline/** or conf/mssql.conf, so the containers take SQL Server's SQL_Latin1_General_CP1_CI_AS default) — I did not query a server. No e2e or msodbcsql compare leg was executed; validation build 173839 was still pending at review time.
Findings not anchored to a diff line
Suggestion — nothing in the unit suite guards either wiring site (measured). Reverting the UTF-8 exemption at get_data.rs:1739 leaves 1400/1400 green; so does neutralizing the entire fetch_scroll.rs arm. Both rest wholly on e2e legs that have not run. That matches your draft note, but it means VarcharMaxUtf8CollationToCharIsNotDoubleConverted and ABoundVarcharMaxUsesItsCollationForChar are the only things standing between a future refactor and silent mojibake. A mock-tds or unit-level test over the (target_type, plp_encoding, collation) → transcode? decision would pin it without a server.
Suggestion — AB#47566 Verification addendum item 7 has no test. The work item asks for "an assertion that the terminator-derived chunk length is correct under SQL_NO_TOTAL … i.e. a value whose chunk boundary lands on payload that must not be trimmed." None of the eleven new tests places a CHAR(0) at a chunk boundary. Moot if the SQL_NO_TOTAL finding lands and the indicator stays concrete — worth saying so explicitly in the work item either way.
Nit — the description overstates the indicator change. "StrLen_or_Ind now reports SQL_NO_TOTAL on this arm" — plp_indicator returns the produced length when the value is not truncated, so it is only on truncation.
Correction to the instructions file, not to this PR. .github/instructions/mssql-odbc.instructions.md:687-696 says a max column only reaches deliver_bound_plp "around a megabyte". That is stale: try_read_row_column tries try_begin_buffered_plp first for any is_plp() column and yields PlpStreaming off an 8-byte header (tds_client.rs:6136), a fast path that arrived in #446 after the bullet was written. The existing unskipped ABoundVarcharMaxTruncatedToWcharReportsNoTotal uses 5,000 wire bytes and asserts SQL_NO_TOTAL, which only plp_indicator inside deliver_bound_plp can produce. Worth its own PR against the instructions file.
Deferred / to file
- Mid-stream target-type switch strands
pending_utf8/pending_units— pre-existing crate-wide, needs its own work item (see the thread atget_data.rs:1666). .github/instructions/mssql-odbc.instructions.md:687-696staleness, above.- AB#47566 scope item 4 and item 7 need correcting against
sqlcdata.h:1230if theSQL_NO_TOTALfinding is accepted. narrow_max_readsizing — throughput only, fine to defer with a tracked item.
Things I could not check
- The CI database's default collation. Inferred from the absence of
MSSQL_COLLATIONin.pipeline/**andconf/mssql.conf; I ran no query against a server. This is the one input the test-breakage half of theSQL_NO_TOTALfinding turns on — the parity-divergence half stands regardless, and validation build 173839 will answer it directly. - Any e2e or msodbcsql compare leg. No reachable SQL Server on this machine; the harness needs a live instance.
- The mssql-python cross-repo leg (AC #5). Pipeline-only by nature, as you noted.
Review feedback on the PLP collation conversion. The indicator change was wrong. msodbcsql keys StrLen_or_Ind on the C types, not on whether a conversion happens: sqlcdata.h:1230 takes CHAR->CHAR on its "assume a 1:1 conversion ratio" branch and only reaches SQL_NO_TOTAL on the CHAR<->WCHAR branch below it. Reporting SQL_NO_TOTAL for a converted varchar(max) broke two pre-existing unskipped cross-leg parity tests, PlpKnownLengthIndicatorCountsDown and ABoundVarcharMaxTruncatedReportsFullLength, which measure that behaviour against msodbcsql and failed on build 173839 with ind = -4. Drop the flag from both indicator sites; the file's own comment already documented the correct rule. The decoder was keyed on the first call's target type, so a stream opened by a SQL_C_BINARY probe left narrow_decoder unset and a later SQL_C_CHAR call fell through to the verbatim copy -- reintroducing the raw code page bytes this change exists to eliminate. Store the resolved encoding on ActivePlpStream, which is a property of the column, and build the decoder on first need. One decoder now serves both directions, so a target switch mid-stream reuses a carry that is still meaningful. Also: give the bound DBCS test an 11-byte token, since an 8-byte one against the 8 KiB PLP_BOUND_CHUNK put every read on a character boundary and never exercised the carry it is named for; drop three SKIP_IF_COMPARING_MSODBCSQL() calls that asserted parity divergences no compare run had measured; and cover the binary-probe-then-char sequence. AB#47566 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…ests Build 173873 ran the three previously-skipped cases unskipped against the pinned msodbcsql leg, which is the evidence the instructions file requires before a SKIP_IF_COMPARING_MSODBCSQL() may be kept. mssql-odbc passed all three; msodbcsql failed all three. Recording what was measured. The two DBCS cases get their skip back, with the observed corruption quoted. msodbcsql returned "...你好世界abc?愫檬澜鏰bc你好世界abc..." where an unbroken repetition of "你好世界abc" was expected: it drops a GBK lead byte at a chunk boundary and the following bytes decode shifted by one. That is a different mechanism from the '?' best-fit documented on the SQL_C_WCHAR twin, so the comments now describe what was actually seen rather than what was assumed. The indicator case keeps running on both legs. The load-bearing claim is shared -- CHAR->CHAR never degrades to SQL_NO_TOTAL on either driver -- and skipping would delete exactly the assertion that pins this PR's behaviour. Only the exact number diverges: msodbcsql's estimate is cbDataAvail + dwDataOffset + cbTruncatedCharsInConvBuf, so it reports 5008 where this driver reports the 5000 wire bytes still available. Neither is the true converted length of 10,000, which is what msodbcsql means by calling it an assumption. Asserted per-leg using the ODBC_TEST_TARGET pattern already used in attributes_test.cpp. Also confirmed by the same run: the CP1252 round trip, the chunk-size invariance, the bound collation case and the binary-probe-then-char case all pass on both drivers, so the central parity claim of this PR is now measured rather than asserted. PlpKnownLengthIndicatorCountsDown and ABoundVarcharMaxTruncatedReportsFullLength are green again on both legs after the indicator revert. AB#47566 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Resolves an import conflict in mssql-odbc/src/api/fetch_scroll.rs: #511 moved is_typed_c_target out of the api::get_data import list, and this branch added transcode_narrow_into_pending to it. Kept both changes -- is_typed_c_target now comes from the conversion::fetch_convert import at line 68, which #511 added. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Build 173890 ran the new conversion tests against msodbcsql on Windows, where that driver converts SQL_C_CHAR to the client ANSI code page rather than UTF-8: ABoundVarcharMaxUsesItsCollationForChar got "\xE9" with indicator 1 where "\xC3\xA9" with indicator 2 was expected. mssql-odbc passed every test on both platforms; only the msodbcsql leg diverges, and only on Windows. This is the known AB#47564 client-code-page difference, already documented on the nvarchar(max) SQL_C_CHAR tests in the same files. Add SKIP_IF_COMPARING_MSODBCSQL_ON_WINDOWS() and apply it to the five affected cases. The existing SKIP_IF_COMPARING_MSODBCSQL() would also work, but it suppresses the comparison on every platform, and build 173873 measured these same cases passing on both drivers on Linux -- that agreement is the parity claim this change rests on, so it is kept where it holds and dropped only where it was measured to fail. VarcharMaxCp1252ToCharChunkSizeDoesNotChangeValue needs no skip: it compares one driver against itself across buffer sizes, so it is encoding-agnostic and passed on all four legs. AB#47566 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
📊 Code Coverage Report
Diff CoverageDiff: main...HEAD, staged and unstaged changes
Summary
mssql-odbc/src/api/fetch_scroll.rsmssql-odbc/src/api/get_data.rsmssql-odbc/src/handles/stmt.rs🔗 Quick Links |
David Engel (David-Engel)
left a comment
David Engel (David-Engel)
left a comment
There was a problem hiding this comment.
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
This is a well-constructed fix for a genuine data-corruption defect (AB#47566 / AB#47875): streamed varchar(max)/text PLP chunks delivered as SQL_C_CHAR were copied verbatim, handing back raw code-page bytes labelled UTF-8, and the bound path carried the byte-identical latent twin. The decomposition is right — transcode_narrow_into_pending is a pure decode-and-accumulate step mirroring widen_into_pending, so the chunk-boundary carry is unit-testable without a live wire, and moving the resolved encoding onto ActivePlpStream (with ensure_narrow_decoder deferring the decoder to first need) is the correct fix for the SQL_C_BINARY-probe-then-SQL_C_CHAR sequence that mssql-python actually performs. The UTF-8-collation exemption is right and is pinned by VarcharMaxUtf8CollationToCharIsNotDoubleConverted.
I read the full prior discussion first. The SQL_NO_TOTAL question, the narrow_max_read throughput cost, the mid-stream target-type switch (AB#48046), the stale 1.1 MiB guidance (AB#48048), and the two unmeasured skips are all already raised, answered, or filed — I am not re-filing any of them. The two items below are the ones I could not find in the existing threads.
Verification
Reviewed at head c5a292d8, merge base 05437152, in a dedicated detached worktree with CARGO_TARGET_DIR redirected, so no other checkout was touched.
cargo nextest run -p mssqlodbc --lib -E 'test(narrow_)' → 15 passed, 0 failed. The crate compiles clean at head. I did not re-run the full suite or clippy: CI already covers both, and gh pr checks 533 shows no failing required check at this head — the ADO PR validation legs were still pending at review time, so the e2e and msodbcsql compare legs that carry most of this PR's evidence are unconfirmed from here. No live SQL Server was reachable, so no e2e case was executed; both findings below are derived by reading, not measured.
Blocking
None.
Suggestion
remaining_indicator ignores pending_utf8, so a drain-only call can report 0 alongside 01004. get_data.rs:2473-2479 computes the streaming indicator as known_total - (total_read - read) — wire bytes still available — which is the right unit for the msodbcsql 1:1 CHAR→CHAR estimate you argued for at sqlcdata.h:1230. But that formula is cbDataAvail + dwDataOffset + cbTruncatedCharsInConvBuf, and cbTruncatedCharsInConvBuf is precisely the analogue of pending_utf8: already-decoded output the caller's buffer had no room for. This path omits it. Once the wire is exhausted, total_read == known_total, so any subsequent call has read == 0 and reports a remaining count of 0 while converted_data_still_held is true and the call returns SQL_SUCCESS_WITH_INFO with 01004 — "truncated, and nothing remains", which is self-contradictory and would strand an application that sizes its next buffer from the indicator rather than looping on the return code. ReadCharDataInChunks loops on rc, so none of the new e2e cases would see it.
Reachability is narrow, and I want to be honest about that rather than overstate it: it needs pending_utf8 to still exceed the caller's payload room after the final wire read, which the remaining / 3 sizing in narrow_max_read mostly prevents. Working the arithmetic through, it needs a payload capacity below 3 bytes (a buffer_length of 2 or 3 for SQL_C_CHAR) combined with the 4-bytes-out-for-1-byte-in gb18030 case you measured on the encoding_rs::GBK decoder. So this is a robustness fix, not a live corruption. Adding pending_utf8.len() to the count would both close it and move the arm closer to the msodbcsql formula you are matching — and it is worth a one-line note in the comment block above remaining_indicator either way, since that block currently explains the wire-byte choice without mentioning that decoded output can outlive the wire.
A truncated UTF-8-collation varchar(max) in a bound SQL_C_CHAR slot can end mid-character. In deliver_bound_plp, fetch_scroll.rs:1954 gates trim_partial_utf8 on matches!(encoding, PlpEncoding::Utf8Text), so a json column truncating at capacity_elements has its partial tail trimmed, while a SingleByteText column under a UTF-8 collation — which this PR now deliberately routes to the same verbatim byte copy at line 1963 — does not, and the caller can receive a buffer whose last character is a partial UTF-8 sequence. This is pre-existing: the condition is byte-for-byte identical on 05437152, so it is a legitimate deferral rather than something this PR broke. I raise it because the PR is what makes the two shapes explicitly equivalent ("A UTF-8 collation is already in the target encoding, so it must stay on the verbatim path"), and because VarcharMaxUtf8CollationToCharIsNotDoubleConverted establishes the column shape without exercising truncation on it. Widening the condition to cover a UTF-8-collated SingleByteText column is a one-line change; filing it against the existing bound-path work rather than here is equally reasonable.
Nit
None.
…n on the bound path Two findings from review, both derived by reading rather than measured; I reproduced the first arithmetically before changing it. remaining_indicator reported only the wire bytes still available, but converting fills pending_utf8 faster than a small caller buffer drains it, so decoded output can outlive the wire. Once the wire is exhausted the wire term is 0, and a drain-only call reported "0 bytes remaining" alongside the 01004 truncation warning it returns -- self-contradictory, and it strands a caller that sizes its next buffer from the indicator instead of looping on the return code. Add the carry, which is also the term msodbcsql carries as cbTruncatedCharsInConvBuf in the same sqlcdata.h:1230 expression. pending_units needs no equivalent: only the widening path fills it, and that path reports SQL_NO_TOTAL. Reachability is narrow, as the reviewer said: it needs pending_utf8 to still exceed the caller's payload room after the final wire read, which the remaining/3 sizing mostly prevents. It takes a payload capacity below 3 bytes together with the 4-bytes-out-for-1-byte-in gb18030 case. A robustness fix, not a live corruption. On the bound path, trim_partial_utf8 was gated on Utf8Text alone, so a json column truncating at the slot boundary had its partial tail trimmed while a SingleByteText column under a UTF-8 collation did not -- and this change is what routes the latter to the same verbatim copy. The condition now covers both, which is what makes the "already in the target encoding" claim actually hold. Pre-existing (byte-for-byte identical on 0543715) and narrowed by this branch, since non-UTF-8 collations now go through the character-wise decode instead. Covered by ABoundUtf8CollationVarcharMaxTruncatesOnACharacterBoundary: a 3-byte character against an 8-byte slot, asserting 6 bytes of whole characters rather than 8 ending mid-sequence. AB#47566 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
Both suggestions were right and are fixed in d75ac8f. Thanks for reading the prior discussion first and scoping to what was genuinely unraised — that made this easy to act on. 1.
|
David Engel (David-Engel)
left a comment
David Engel (David-Engel)
left a comment
There was a problem hiding this comment.
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
Re-review at d75ac8f4, scoped to what changed since the c5a292d8 pass. Both fixes from that pass read correctly: adding the carry to remaining_indicator closes the self-contradictory "0 remaining alongside 01004" case, and generalizing the trim gate to verbatim_utf8_text puts a UTF-8-collated SingleByteText column on the same character-boundary trim as json, which is what makes the "already in the target encoding" claim hold. I re-read the whole diff against origin/main, not just the delta, and I am not re-filing anything already raised or filed (SQL_NO_TOTAL, narrow_max_read throughput, AB#48046, AB#48048).
One new item, and it is about CI rather than the code: Build Stage Build Linux is red at this head, and that is the only leg that runs the msodbcsql comparison.
Verification
Reviewed at head d75ac8f4, merge base 05437152, in the assigned worktree with CARGO_TARGET_DIR redirected. cargo nextest run -p mssqlodbc --lib --no-fail-fast → 1417 passed, 0 failed. I did not run fmt/clippy or the full workspace: CI covers both and neither is what is failing.
gh pr checks 533 at this head: everything green or pending except mssql-rs Pull request validation (Build Stage Build Linux) (build 173919), which failed. That leg was green at abfc87de (173890) and failed at the two heads before it. I could not read the ADO log — no auth from this run — so the attribution below is inferred from the pipeline definition and the commit history, not measured.
Blocking
Build Stage Build Linux is failing at this head, and it is the msodbcsql compare leg. .pipeline/templates/build-template-container.yml sets ODBC_E2E_COMPARE=1 only on the Linux x64 job (the ARM leg at the same template runs the driver alone, and Windows runs no ODBC e2e at all), which is exactly why that one leg is red while Linux ARM, macOS and both Windows legs are green. The only e2e-visible change since the last green run on that leg is the single new case this commit adds, and it runs unskipped against msodbcsql there. Detail inline at fetch_scroll_test.cpp:964. Please confirm from build 173919 before acting on my attribution — this needs to be green (or explained) before the PR goes to human review.
Suggestion
None.
Nit
The new indicator term double-counts this call's own read. One-word fix, detail inline at get_data.rs:2493.
…ion test per leg Two review findings, both confirmed before changing anything. The indicator read pending_utf8 after the drain instead of as it stood at entry, so this call's own read was counted twice: wire_remaining already includes the bytes read by this call, and the post-drain carry holds output decoded from those same bytes. utf8_carry_len is already in scope and is exactly the entry-time value the indicator is defined in terms of, so use it and drop the re-read of active_plp. The two agree whenever the carry drains fully, which narrow_max_read's remaining/3 makes the normal case, so this is a nit rather than a defect -- but it makes the "bytes still available before this call's copy" sentence above it true. ABoundUtf8CollationVarcharMaxTruncatesOnACharacterBoundary was the sole failure on the msodbcsql compare leg of build 173919. Confirmed from the log rather than inferred: msodbcsql returned "\xE4\xBD\xA0\xE4\xBD\xA0\xE4\xBD", filling all 8 payload bytes and ending mid-character, where this driver stops at 6. That is the same deliberate deviation already registered as item 8 of mssql-odbc.instructions.md (AB#47767) for the SQL_C_WCHAR surrogate-pair case: this driver trims a bound max column to a whole character where msodbcsql fills the slot. Split per-leg on ODBC_TEST_TARGET rather than skipping outright, so the shared contract -- both truncate, report 01004, and deliver a prefix of the value -- stays measured against msodbcsql, and only the tail diverges. Registered alongside item 8 with the measuring build, since leaving a case on the compare leg is itself a parity claim. AB#47566 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Saurabh Singh (saurabh500)
left a comment
Saurabh Singh (saurabh500)
left a comment
There was a problem hiding this comment.
This review came from an unattended sweep; the findings were not checked by a human first.
Verdict: approve in substance; no blocking findings. This is a comment-only review, not a GitHub approval; latest-head validation is still running.
The change converts server-collation narrow PLP input on both SQLGetData and bound fetch. SQLGetData retains decoder state and pending UTF-8 output across reads, including after wire input ends. I checked the msodbcsql source conversion callers and concrete CHAR-to-CHAR indicator policy: sqlcdata.cpp:912-1034, sqlcdata.h:1230-1234, and bound dispatch in sqlccurs.cpp:1737-1750 under Sql/Ntdbms/sqlncli/odbc. Transcoding alone does not require SQL_NO_TOTAL. The documented Windows client-encoding difference and measured DBCS/reference differences remain explicitly scoped; this is not a claim of identical behavior on every platform.
Blocking
No blocking findings.
Suggestion
No suggestions.
Nit
No nits.
Premature decoder finalization failed three focused tests; discarding pending UTF-8 output failed four. Restoring the code made the 15-test narrow filter pass. These are Rust mechanism checks, not a local rerun of the C++ reference comparisons.
Independent critique found no additional actionable findings. Its indicator-accounting caution was checked against the reference's carry-drain and conversion-update sites: the agreement claimed here is a concrete CHAR-to-CHAR estimate, not identical arithmetic or identical numeric estimates in every case. No findings were added or removed, and the existing follow-ups were not duplicated.
David Engel (David-Engel)
left a comment
David Engel (David-Engel)
left a comment
There was a problem hiding this comment.
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
Re-review at 08384258. Both items from the d75ac8f4 pass landed as agreed and read correctly: remaining_indicator now uses utf8_carry_len, the entry-time carry, so the sentence three lines above it ("the bytes still available before this call's copy") is true of the code beneath it, and held_converted_bytes and its re-read of active_plp are gone. The per-leg split on ABoundUtf8CollationVarcharMaxTruncatesOnACharacterBoundary is the better of the two options I offered — holding msodbcsql to "truncates, reports 01004, delivers a prefix" keeps more measured than a bare skip would, and your correction that "no partial sequence delivered" cannot be the shared assertion is right, since that is exactly what msodbcsql does here. Registering it next to item 8 (AB#47767) as the SQL_C_CHAR case of the same deviation is the right home for it.
I re-read the whole diff against origin/main, not only the delta, and I am not re-filing anything already raised, answered or filed: the SQL_NO_TOTAL question, narrow_max_read throughput (AB#48047), the mid-stream target-switch stranding (AB#48046), the stale bound-PLP threshold (AB#48048), and the two measured msodbcsql skips.
The Build Stage Build Linux failure I raised last time is resolved: that leg is green at this head (32m27s, build 173938), and the compare-leg summary it carries is the evidence the parity claims in the description rest on.
Verification
Reviewed at head 0838425822cd7033cf5efb7afbd212e2de052ee7, merge base 05437152, in the assigned detached worktree with CARGO_TARGET_DIR redirected, so no other checkout was touched. cargo nextest run -p mssqlodbc --lib --no-fail-fast → 1417 passed, 0 failed. I did not re-run fmt/clippy or the workspace suite: CI covers both and gh pr checks 533 shows nothing failing at this head (Test MacOS, the macOS cross-repo build, and coverage-report were still pending at review time). No live SQL Server was reachable from here, so no e2e case was executed; both notes below are derived by reading, not measured.
Blocking
None.
Suggestion
None.
Nit
The new carry term is added on every known_total path, not just the transcoding one, so a mid-stream target switch can mix units into the indicator. Detail and a one-line scoping at get_data.rs:2494. Only reachable through the switch already filed as AB#48046, which I am not re-filing.
A stale comment paragraph survives its own correction in VarcharMaxCp1252ToCharChunkedRoundTrip — the "Deliberately NOT skipped on the msodbcsql leg" sentence contradicts the SKIP_IF_COMPARING_MSODBCSQL_ON_WINDOWS() the test now opens with, and the paragraph immediately below it already says the accurate thing. Detail at get_data_test.cpp:1107.
… comment The carry was added on every known_total path, but pending_utf8 is only ever filled by a SQL_C_CHAR call and it outlives that call. The compatibility gate admits a different target on a continuation -- (SQL_C_BINARY, _) unconditionally, and SQL_C_WCHAR on a Utf16Text column takes this same branch -- so a mid-stream target switch could report wire bytes plus decoded UTF-8 bytes to a binary or UTF-16 delivery, a number in neither unit. Those targets now keep the plain wire count they have on main. Reaching either shape needs the mid-stream target switch tracked in AB#48046, and in that state the stream is stuck on 01004 anyway because converted_data_still_held never clears; this line simply does not need to depend on that being fixed. Also delete three lines above VarcharMaxCp1252ToCharChunkedRoundTrip that survived an earlier edit and said the case is "deliberately NOT skipped on the msodbcsql leg", immediately above the SKIP_IF_COMPARING_MSODBCSQL_ON_WINDOWS() call and the accurate build-referenced paragraph that replaced them. AB#47566 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
ttk (Theekshna)
left a comment
ttk (Theekshna)
left a comment
There was a problem hiding this comment.
Automated review (unattended hourly sweep)
First pass from this automation on PR #533 (no prior marker). Three qualified reviews already ran here (Vahid-b's own self-review with 4 mutation runs, three rounds of David-Engel's automated review, and saurabh500's independent msodbcsql-source check) and all 18 review threads are resolved. I read all three discussion endpoints (inline comments, reviews, issue comments) plus gh pr checks before drafting anything, and I am not re-filing any of: the SQL_NO_TOTAL question, the remaining_indicator/pending_utf8 carry fix, the bound-path UTF-8-collation truncation fix, the transient Build Stage Build Linux failure, narrow_max_read throughput (AB#48047), the mid-stream target-switch stranding (AB#48046), or the stale bound-PLP threshold (AB#48048) — all already raised, answered, and fixed.
My incremental work was the one commit no one had reviewed yet (f1a109bd, scoping the indicator carry to transcode_narrow_to_utf8 and dropping a superseded comment), plus independently re-deriving the central parity claim.
Verified, not just read:
- Read
f1a109bd's diff directly:held_converted_bytesis gated ontranscode_narrow_to_utf8, which is exactly the arm that fillspending_utf8(get_data.rs:1825). TheSQL_NO_TOTALbranch above it already special-casestranscode_utf16_to_utf8 || widen_narrow_to_utf16, so the newelseonly ever sees a verbatim-copy or narrow-transcode target — no path double-counts or mixes units. The.cpphalf of the commit only deletes a stale comment sentence that contradicted the paragraph beneath it; confirmed the replacement paragraph is accurate against the currentSKIP_IF_COMPARING_MSODBCSQL_ON_WINDOWS()call. - Re-derived the central msodbcsql claim myself rather than trusting the citation:
Sql/Ntdbms/sqlncli/odbc/sqlcdata.haround line 1230 does takeSQL_C_CHAR->SQL_C_CHARon thecbDataAvail + dwDataOffset + cbTruncatedCharsInConvBuf(1:1 estimate) branch, separately from theSQL_C_CHAR<->SQL_C_WCHARbranch that returns the unlimited/no-total sentinel a few lines below. That confirms both the SQL_NO_TOTAL revert and thepending_utf8/cbTruncatedCharsInConvBufanalogy the fix relies on. - Confirmed the divergence for
ABoundUtf8CollationVarcharMaxTruncatesOnACharacterBoundaryis now recorded in.github/instructions/mssql-odbc.instructions.md(extension of item 8), and the new bound e2e test exists infetch_scroll_test.cpp. - Attempted
cargo nextest run -p mssqlodbc --libmyself; blocked by the pre-existingcrc32fast/AVX512pclmulqdqcodegen failure this environment cannot build past (same wall Vahid-b hit in his own review). CI already ran the suite at this head (gh pr checks 533: all legs green or pending, none failing, includingBuild Stage Build Linux, the msodbcsql-compare leg) and David-Engel independently confirmed 1417/1417 at the prior two commits, unchanged by this delta.
| Severity | Count |
|---|---|
| Blocking | 0 |
| Suggestion | 1 |
| Nit | 0 |
Six required checks
- msodbcsql parity: audited the CHAR->CHAR vs CHAR<->WCHAR split in
sqlcdata.h:~1230myself (see above) — confirms both the indicator arithmetic and thattranscode_narrow_to_utf8/transcode_utf16_to_utf8/widen_narrow_to_utf16are the complete, correctly-partitioned set for this table; no arm left unaudited. - Test sufficiency: the new regression tests (
narrow_transcode_converts_cp1252_to_utf8and friends,ABoundUtf8CollationVarcharMaxTruncatesOnACharacterBoundary) cover the fixes; already mutation-tested by Vahid-b (4 runs) and David-Engel across the review. The one gap — no unit test pins theheld_converted_bytesscoping in this delta — is unreachable without AB#48046 landing first, which the commit message states explicitly; consistent with SKILL's unreachable-branch guidance, not a fresh gap. - Divergences documented: yes — item 8 extension in the instructions file, confirmed above.
- PR description currency: gap found, see Suggestion below.
- Verbose slop: none. The new comments at
get_data.rs:2489-2498and aroundverbatim_utf8_textexplain non-obvious invariants (why binary/UTF-16 targets must not inherit the carry, why the UTF-8-collation and json cases must share a trim gate) rather than restating code. - Audit of PR's own evidence: re-derived the CHAR->CHAR estimate formula myself (above) rather than accepting the description's citation at face value. Did not re-run the Linux/msodbcsql e2e legs myself (no live SQL Server here); relying on the green
Build Stage Build Linuxcheck plus Vahid-b's and David-Engel's independent runs of the same comparison. One neighbor genuinely goes unmeasured across all reviews so far: the cross-comparison only runs on the Linux x64 leg (David-Engel's finding,ODBC_E2E_COMPARE=1), so macOS never exercises the msodbcsql-agreement claim — that's inherent to the CI topology rather than this PR's gap, and already surfaced in the existing discussion, so not re-filed as new.
Suggestion
- PR description currency: the description's indicator section only covers the initial
SQL_NO_TOTAL->concrete-count revert (746fbc3f). It does not mention the two real correctness fixes review turned up afterward — theremaining_indicatorignoringpending_utf8(self-contradictory "0 remaining with 01004", fixed ind75ac8f4/08384258) or the bound-path UTF-8-collation truncation gap (verbatim_utf8_textgeneralization, alsod75ac8f4) — nor the newABoundUtf8CollationVarcharMaxTruncatesOnACharacterBoundarytest or the instructions-file addition that came with it. All are real, review-discovered, in-scope changes; a reader of the description would not know either shipped. Worth a line each, the same pattern already applied to the AB#48046/47/48 follow-ups lower in the same description.
This review was produced by an unattended scheduled run; findings were not checked by a human before posting.
|
Good catch on the description — it was stale. Updated. You're right that a reader would not have known three review-discovered changes shipped. Added:
Two more staleness bugs I found while editing, which your suggestion prompted:
Also renamed the section from "a correction" to "one correction and two follow-on fixes", since it now covers three separate things. On your Your note that the cross-comparison only runs on Linux x64, so macOS never exercises the msodbcsql-agreement claim, is a fair characterization of the CI topology — and I agree it belongs against the pipeline rather than this PR. |
David Engel (David-Engel)
left a comment
David Engel (David-Engel)
left a comment
There was a problem hiding this comment.
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
Re-review at f1a109bd. Both nits from the 08384258 pass are resolved, and each was resolved the way it was raised rather than papered over:
held_converted_bytesis now scoped totranscode_narrow_to_utf8(get_data.rs:2478), so a mid-stream target switch toSQL_C_BINARYorSQL_C_WCHARkeeps the plain wire count it has onmaininstead of having UTF-8 bytes folded into a number in neither unit. The comment block above it explains the scoping rather than just asserting it.- The contradictory "Deliberately NOT skipped on the msodbcsql leg" paragraph in
VarcharMaxCp1252ToCharChunkedRoundTripis gone; what remains ("Not skipped on Linux/macOS ... Skipped only on Windows, where msodbcsql uses the client ANSI code page") matches theSKIP_IF_COMPARING_MSODBCSQL_ON_WINDOWS()the test opens with.
I re-read the whole diff against origin/main (merge base 05437152, 7 files, +1003/-183), not only the delta since the last pass, and I traced the pieces the diff depends on but does not contain: get_encoding_type, EncodingType::encoding, lcid_encoding_or_fallback, plp_indicator, copy_with_nul, and the prefetch path in stream_active_plp_chunk. The parts I checked most closely and found correct:
- UTF-8 collations cannot double-convert on either path. The bound arm keys off
column_info.text_encoding, andget_encoding_typealready returnsEncodingType::Utf8for acollation.utf8()column, sonarrow_wire_encoding == Some(UTF_8)andtranscode_narrow_to_utf8is false. The streaming arm reaches the same answer by its own route (collation.utf8()checked beforeLcidBased(...).encoding()), and the two agree for everySingleByteTextshape except a missing collation, where the streaming side yieldsNoneand stays verbatim.jsonnever reaches the transcode branch at all, since it requiresSingleByteText. - The read-ahead really is disabled for the converting path, not just claimed to be.
prefetch_lenis filtered ondirect_wire_output, whichwire_shaped_outputnow clears fortranscode_narrow_to_utf8, so wire bytes cannot land in a buffer that is the decoder's input. - The indicator arithmetic does not double-count.
wire_remainingisknown_total - (total_read - read), which includes this call's read, andutf8_carry_lenis the entry-time carry decoded from earlier reads, so the sum is exactly "available before this call's copy". - Buffer safety on the new emit path.
emit ≤ payload_capacity = buffer_length - 1, socopy_with_nulwrites payload plus terminator insidebuffer_length. The zero-capacity probe shapes (buffer_length0 or 1) reachemit == 0, so thepending_utf8.drain(..emit)cannot discard undelivered bytes on a probe. ABoundVarcharMaxTruncatedReportsFullLengthandPlpKnownLengthIndicatorCountsDown, cited in the new comment blocks as the cross-leg tests holding msodbcsql to a concrete CHAR→CHAR indicator, both exist and are unskipped.
Nothing new to raise. The items I deliberately did not re-file, because they are already raised, answered, or tracked: the SQL_NO_TOTAL decision and its sqlcdata.h:1230 grounding, narrow_max_read throughput (AB#48047), the mid-stream target-switch carry stranding (AB#48046), the stale bound-PLP threshold in the instructions file (AB#48048), and the two measured msodbcsql skips.
Verification
Reviewed at head f1a109bd5d96ac9b02118d06fa2f81986832ebc8, merge base 05437152, in the assigned detached worktree with CARGO_TARGET_DIR redirected, so no other checkout was touched.
Compiled mssqlodbc and ran the unit tests covering the new logic:
cargo nextest run -p mssqlodbc --lib -E 'test(narrow_transcode) or test(narrow_max_read) or test(plp_indicator) or test(utf8_truncation)'
9 tests run: 9 passed, 1408 skipped
That covers the CP1252 conversion, the DBCS chunk-boundary carry, the output hold-back, chunk-size invariance, the post-wire drain, the zero-capacity probe, narrow_max_read sizing, plp_indicator, and the partial-UTF-8 trim. I did not re-run fmt/clippy or the full workspace suite: CI covers both, and gh pr checks 533 shows nothing failing at this head (check, coverage-report, Merge Coverage, and the ADO rollup were still pending at review time; every completed leg is green). No live SQL Server was reachable from here, so no e2e case was executed — those claims rest on the build numbers in the description, not on anything I measured.
Blocking
None.
Suggestion
None.
Nit
None.
David Engel (David-Engel)
added
the
ready for human review
Review noted that the UTF-16 path pins this behaviour
(utf16_chunk_trailing_odd_byte_at_end_is_replacement and its lone-surrogate
sibling) while the narrow path only claims it in a doc comment. Add the
counterpart: a GBK lead byte with no trail byte, which is producible from SQL as
CAST(0xD0 AS VARCHAR(MAX)) under a DBCS collation, must decode to U+FFFD rather
than vanish or panic.
Probing that behaviour before asserting it turned up a second shape the comment
was overclaiming. The flush only happens when the final wire bytes and
reached_end arrive on the same call: the decode is guarded by
!(payload.is_empty() && reached_end), so if reached_end first arrives on an empty
read the decoder is never flushed and a sequence it still holds is dropped
instead of replaced. Measured:
same call: ("abc\xD0", end=true) -> "abc\uFFFD" correct
split: ("\xD0", end=false) then ("", end=true) -> "" byte lost
The guard is not gratuitous -- encoding_rs forbids using a decoder after a
last=true call, so repeated drain-only calls must not re-flush -- and
widen_into_pending has carried the identical guard since before this path
existed, so the varchar(max) -> SQL_C_WCHAR widening has the same exposure on
main. Filed as AB#48073 with the probe output and a suggested fix (hold the
decoder as an Option and take() it on flush, making "already flushed" explicit
rather than inferred from an empty payload) rather than changing a shared
contract used by both helpers and both call sites on this PR.
The new test covers the same-call shape, which is the one the current guard
admits; the comments on both the helper and the test now say so instead of
claiming an unconditional flush.
AB#47566
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
David Engel (David-Engel)
left a comment
David Engel (David-Engel)
left a comment
There was a problem hiding this comment.
Automated review — PR #533 @ d3fd73da
Verdict: clean. No blocking issues found. Two non-blocking suggestions and one nit below.
What I verified
Reviewed the full substantive diff (get_data.rs, fetch_scroll.rs, handles/stmt.rs, the e2e tests, and the instructions update), plus the adjacent code the change depends on (copy_with_nul, plp_indicator, get_encoding_type/lcid_encoding_or_fallback, try_read_active_plp_chunk).
Locally against the head commit: cargo fmt -- --check clean, cargo clippy --workspace --all-features --all-targets clean, and the 45 targeted mssqlodbc unit tests (narrow|plp|transcode|widen|utf8) all pass. CI diff coverage reports 89%, above the 85% gate.
Specific correctness properties I checked and believe hold:
- UTF-8 collations are correctly excluded from double-conversion on both paths.
get_data.rscheckscollation.utf8()explicitly before falling back toEncodingType::LcidBased(...).encoding(), which matters becauselcid_encoding_or_fallbackmaps LCID 1033 to WINDOWS_1252 and would have mojibake'd a UTF-8-collation column.fetch_scroll.rsgets there viaget_encoding_type, which returnsEncodingType::Utf8forcollation.utf8(). Both land on the verbatim path, andVarcharMaxUtf8CollationToCharIsNotDoubleConvertedpins it. - The spent-decoder hazard is handled.
reached_endcomes fromactive_plp_reached_end()(stream state), not from the read, so it staystrueon every later drain-only call. Combined with&payload[..read]and the!(payload.is_empty() && reached_end)guard,decode_to_utf8(.., last: true)is never followed by another decode on the same decoder. - Termination. Every call either consumes ≥1 wire byte or drains ≥1 carry byte, and
narrow_max_readonly returns 0 when the carry already fillspayload_capacity— so the drain-only loop always converges.narrow_transcode_drains_pending_after_the_wire_is_exhaustedcovers the pure-function half. - Buffer safety.
emit ≤ payload_capacity == buffer_length - 1, socopy_with_nulnever truncates belowemitand always has room for the terminator. Null pointer andbuffer_length == 0are both short-circuited insidecopy_with_nul.direct_wire_outputis correctly forced off for the transcoding path, so the decoder input never aliases the caller's buffer. - Indicator accounting.
held_converted_bytesuses the entry-time carry, so the bytes read by this call are not counted twice (once inwire_remaining, once in the post-drain carry). Scoping it totranscode_narrow_to_utf8keeps binary/UTF-16 continuations on their existing unit.
The e2e coverage is strong — CP1252 round-trip, GBK chunk-boundary carry, chunk-size invariance, UTF-8 collation, binary-probe-then-char, bound truncation on both legs — and the per-leg splits are backed by cited build numbers rather than blanket skips, per the repo's own instructions file. The comments are load-bearing rather than decorative.
Suggestions (non-blocking)
1. Throughput on the default server collation. narrow_max_read budgets the worst case (1 wire byte → 3 UTF-8 bytes), so an 8 KiB caller buffer reads only ~2730 wire bytes per SQLGetData. For SQL_Latin1_General_CP1_CI_AS — the default collation, and near-1:1 in practice — that is roughly 3x more SQLGetData calls and 3x more 01004 returns than main delivered on this path. The read-ahead prefetch is also now disabled here, since wire_shaped_output excludes the transcoding case.
This mirrors the utf16le_max_read precedent exactly, so it is a consistent choice rather than a defect, and none of it is a correctness problem. Two things might still be worth a follow-up: a DBCS encoding's true worst case is 2 bytes in → 3 bytes out, so a per-encoding ratio would roughly double the read for GBK/SHIFT_JIS/BIG5/EUC-KR; and since the carry absorbs overshoot anyway, a more generous ratio would only grow pending_utf8 by a bounded amount. Given mssql-odbc-bench exists in this repo, a quick before/after on a varchar(max) fetch under the default collation would settle whether this is worth pursuing.
2. Mid-stream target switch is now admitted where it used to be refused. On main, widening_ready was narrow_to_wide.is_some(), and the decoder was only built when the first call asked for SQL_C_WCHAR. So SQLGetData(SQL_C_BINARY, ...) followed by SQLGetData(SQL_C_WCHAR, ...) returned HYC00. Now decoder_ready is narrow_encoding.is_some(), which is a property of the column, so that sequence is admitted and ensure_narrow_decoder builds a decoder starting at whatever wire offset the binary read left behind.
For the motivating case — the zero-length binary probe in VarcharMaxBinaryFirstStillConvertsOnLaterCharRead, which consumes nothing — this is exactly right and is a genuine fix. But if the binary read actually consumed bytes, decoding now resumes mid-character under a DBCS collation and produces shifted output with no diagnostic, where main returned a hard error. That is caller misuse either way, and you already reference AB#48046, so I am not asking for a change here. It may be worth deciding explicitly whether "silently best-effort" is the direction you want, versus gating the decoder on total_read == 0 and keeping HYC00 when wire has already been consumed. A one-line note in the AB#48046 item would be enough.
Nit
3. The new branch inside stream_active_plp_chunk is only exercised end-to-end by the live-server e2e tests; the in-process tests cover transcode_narrow_into_pending and narrow_max_read as pure functions. That split is reasonable and coverage is comfortably above the gate — just noting that the branch wiring (carry across calls, indicator, 01004) is not reproducible without a server, unlike the mock-server tests elsewhere in the file.
Also noted
The AB#48073 case added in d3fd73da — reached_end first arriving on an empty-payload call drops a held partial sequence instead of flushing it to U+FFFD — is correctly identified as pre-existing behaviour shared with widen_into_pending, and documenting it in place plus tracking it separately is the right call for this PR's scope.
Automated review. Findings are advisory; no approval or change request is implied.
|
Clean verdict noted, and both suggestions plus the nit are fair. Tracked rather than changed, with reasoning below. 1. Throughput — added to AB#48047The per-encoding refinement is a genuinely better idea than what I'd filed. AB#48047 originally proposed reading 2. Mid-stream target switch now admitted — added to AB#48046You're right, and I verified it against So This was a deliberate trade, but you've identified that I made it wider than the problem required. The case I needed is mssql-python's zero-length The discriminator is whether the stream has advanced — refusing the switch only when wire has been consumed would restore Worth stating plainly: previously a clean error, now a possible mis-decoded leading character. Neither is obviously correct and msodbcsql's behaviour on that sequence is unmeasured, which is exactly why it wants its own evaluation rather than a quick patch. 3. Nit — agreedThe branch wiring (carry across calls, indicator, |
|
Pls add a parent user story for all the newly created tasks. Easier to pick it up later. Can use a pre-existing appropriate one under https://sqlclientdrivers.visualstudio.com/mssql-rs/_workitems/edit/42845/ or create a new one under it |
|
Done — all four follow-ups now have parents. I matched each to the User Story where its siblings already live rather than putting all four under one, since that mirrors how the related items are already organised (AB#47566 → 46383, AB#47875 → 47505, AB#47767/47564 → 46375):
All four parents are Active under Feature AB#42845, so they'll surface on the board rather than sitting loose. One deliberate choice worth flagging: I left the iteration at the backlog root ( |
Description
SQL_C_CHARoutput is UTF-8 on this driver, but PLP delivery of aSingleByteTextcolumn copied the wire bytes verbatim. Under a non-UTF-8 collation the caller got raw code page bytes labelled UTF-8. The non-PLP path was already correct — it decodes through the collation viaSqlString::to_utf8_string— so the same value delivered inline and streamed disagreed byte for byte.The offending line documented its own defect:
What changed
get_data.rs(SQLGetData) —SingleByteTextis decoded through the column's collation. Reuses the per-streamencoding_rs::Decoderthat already served theSQL_C_WCHARwidening, so a multi-byte DBCS sequence split across a chunk boundary is carried rather than corrupted.pending_utf8holds back output the caller's buffer had no room for — decoding expands, e.g. CP12520x80is 3 UTF-8 bytes.fetch_scroll.rs(SQLBindCol+SQLFetch) — carried the byte-for-byte identical verbatim copy. Converted the same way. This arm was latent: no test read avarchar(max)under a non-UTF-8 collation through a bound fetch, so nothing failed on it.ActivePlpStream, with the decoder built lazily on first need. Keying it on the first call's target type meant aSQL_C_BINARYprobe — the shape mssql-python issues per column — left a laterSQL_C_CHARread on the verbatim path, reintroducing the very defect being fixed.max_readsized for expansion vianarrow_max_read, mirroringutf16le_max_read.jsonstay verbatim, explicitly not double-converted — and now share a truncation gate.trim_partial_utf8was keyed onjsonalone, so a UTF-8-collatedvarchar(max)truncating in a bound slot could end mid-character. Pre-existing, but this change is what makes the two shapes equivalent, so they are generalized to oneverbatim_utf8_textcondition.SQL_NO_TOTAL, and now includes held converted output. See below — the first revision got this wrong twice.Verification
Build 173899 — full PR validation green, all legs, at
c5a292d8. Commits after that are review fixes; build 173938 re-confirmed the compare legs at08384258withSummary: 40 parity, 0 divergence(s), 0 shared failure(s)(up from 38/2).The bug this fixes:
tests.test_017_varchar_cp1252_boundary::test_varchar_cp1252_lob_with_collation→ Passed (test run 3372229), after failing on 6/6 consecutive prior runs.No regressions, measured rather than asserted: the full mssql-python failure set went 159 → 158 against the pre-fix baseline (build 173751, run 3368660). Newly failing: none. Newly passing: that one test, and only that test. The remaining 158 are pre-existing and unrelated (decimal/money binding,
getinfo,executemany).Diff coverage 89% (target 85%).
Parity, as measured
Linux (build 173873) — both drivers agree, which is the central claim of the PR, now measured rather than argued:
VarcharMaxCp1252ToCharChunkedRoundTripVarcharMaxCp1252ToCharChunkSizeDoesNotChangeValueVarcharMaxUtf8CollationToCharIsNotDoubleConvertedABoundVarcharMaxUsesItsCollationForCharVarcharMaxBinaryFirstStillConvertsOnLaterCharReadDiverging, each now carrying its measurement in-comment rather than an assumption:
SQL_C_CHARto the client ANSI code page there, returning\xE9with indicator 1 where\xC3\xA9with indicator 2 is expected. Known AB#47564. A newSKIP_IF_COMPARING_MSODBCSQL_ON_WINDOWS()scopes the skip to Windows rather than suppressing the comparison everywhere — the Linux agreement above is real and worth keeping....你好世界abc?愫檬澜鏰bc你好世界abc...where an unbroken repetition of你好世界abcwas expected. It drops a GBK lead byte at a chunk boundary and the following bytes decode shifted by one. This is not the?best-fit documented on theSQL_C_WCHARtwin — a different mechanism, and this driver is on the correct side of it.SKIP_IF_COMPARING_MSODBCSQL()retained with build 173873 and the observed output quoted.ABoundVarcharMaxTruncatedToCharKeepsConcreteLength— kept running on both Linux legs. The load-bearing claim is shared: CHAR→CHAR never degrades toSQL_NO_TOTAL. Only the number differs — msodbcsql's estimate iscbDataAvail + dwDataOffset + cbTruncatedCharsInConvBuf, so it reports 5008 where this driver reports the 5000 wire bytes still available. Neither is the true converted length (10,000), which is what msodbcsql means by "assume a 1:1 conversion ratio". Asserted per-leg via theODBC_TEST_TARGETpattern already used inattributes_test.cpp; skipping would have deleted the one assertion that pins this PR's behaviour.On the indicator — one correction and two follow-on fixes
The first revision reported
SQL_NO_TOTALfor the converted arm, on the strength of AB#47566's scope item 4. That was wrong, and build 173839 proved it by failing two pre-existing unskipped cross-leg parity tests withind = -4:msodbcsql keys the indicator on the C types, not on whether a conversion happens:
sqlcdata.h:1230takes CHAR→CHAR on the 1:1 branch and only reachesSQL_NO_TOTALon CHAR↔WCHAR. Reverted at both sites; the file's own pre-existing comment had the rule right. AB#47566's scope item 4 has been corrected, since the work item mandated the wrong behaviour.Review then surfaced two real defects in that same arm, both fixed:
remaining_indicatorignoredpending_utf8. Decoded output can outlive the wire, so once the wire was exhausted a drain-only call reported "0 bytes remaining" alongside the01004truncation warning it returns — self-contradictory, and it strands a caller that sizes its next buffer from the indicator instead of looping on the return code. The carry is now added, using the entry-timeutf8_carry_len(reading it post-drain double-counts the current read), and scoped totranscode_narrow_to_utf8so aSQL_C_BINARYorSQL_C_WCHARcontinuation cannot inherit UTF-8 bytes into a count of a different unit. This is the term msodbcsql carries ascbTruncatedCharsInConvBufin the samesqlcdata.h:1230expression — I had used half the formula, and initially at the wrong time.Background
SQLCHARto the process locale encoding, defaulting to UTF-8, having already converted from the column collation (programming guidelines).BIGVARCHARTYPEis the same type whether framed asUSHORTLEN_TYPEorPARTLENTYPE. COLLATION arrives once inCOLMETADATA; PLP chunk boundaries are byte-aligned framing with no character semantics. No basis for decoding max and non-maxvarchardifferently.Why this shipped
Every existing CP1252/DBCS PLP test targeted
SQL_C_WCHAR. Every existingSQL_C_CHARPLP test was eithernvarchar(max)(UTF-16 source, correctly transcoded) or ASCIIvarchar(max)— where verbatim happens to equal UTF-8. Nothing coveredvarchar(max)+ non-UTF-8 collation intoSQL_C_CHAR.Related Issues
https://sqlclientdrivers.visualstudio.com/mssql-rs/_workitems/edit/47566
Also fixes https://sqlclientdrivers.visualstudio.com/mssql-rs/_workitems/edit/47875 —
tests.test_017_varchar_cp1252_boundary::test_varchar_cp1252_lob_with_collation. mssql-python'sGetEffectiveCharDecodinghardcodes"utf-8"on Linux/macOS (it assumes the driver already converted), so our raw CP1252 bytes failed its strict decode and hit theexceptbranch returningbytes. The character count matched only because CP1252 is single-byte.Follow-ups filed from review: AB#48046 (pre-existing target-switch carry stranding), AB#48047 (
narrow_max_readround-trip cost), AB#48048 (stale ~1 MiB bound-PLP threshold in the instructions file), and AB#48073 (pre-existing: the PLP decoder flush is skipped whenreached_endarrives on an empty read, dropping a partial sequence instead of replacing it — affects theSQL_C_WCHARwidening onmainas well).Tests
Unit (no live server):
narrow_transcode_converts_cp1252_to_utf8asserts the CP1252 wire bytes differ from the UTF-8 output — the exact confusion that hid this bug; plus chunk-boundary carry, output hold-back, chunk-size invariance, post-wire drain, zero-capacity probe,narrow_max_readsizing, andnarrow_transcode_malformed_sequence_at_end_is_replacement(a dangling GBK lead byte, reachable asCAST(0xD0 AS VARCHAR(MAX))under a DBCS collation, must become U+FFFD).E2E: the five agreeing cases above, the two DBCS carry cases, the truncation/indicator case, and
ABoundUtf8CollationVarcharMaxTruncatesOnACharacterBoundary— a 3-byte character against an 8-byte slot, asserting 6 bytes of whole characters rather than 8 ending mid-sequence. That last one is split per-leg onODBC_TEST_TARGET: msodbcsql fills the slot and ends mid-character (measured, build 173919), which is the same deviation already registered as item 8 of.github/instructions/mssql-odbc.instructions.md(AB#47767), so it is recorded there rather than skipped — the shared contract (both truncate, report01004, deliver a real prefix) still runs on both legs.Checklist
cargo bfmtpassescargo bclippypassescargo btestpasses — 1418/1418mssqlodbcunit testsActivePlpStreamispub(crate)