odbc: mandatory source-type conversions for typed SQLGetData (P1a) [AB#47107]

[David Engel (David-Engel)]

What

ODBC Appendix D requires every supported SQL type to convert to every supported C type. P1 (#146) implemented the C-target side: integers, floats, GUID and the date/time structs. This is the source-type side, so those targets now accept the column types that were previously rejected.

Stacked on P1, rebased onto main now that #146 has merged.

Numeric targets

• New NumericSource abstraction keeps exact sources exact, so an integer target can report truncation instead of silently dropping a fraction: Int, Scaled { mantissa, scale }, Float.
• decimal and numeric parse from their own exact rendering rather than reassembling base-2^32 limbs; money and smallmoney use their 10^4-scaled wire value.
• convert_integer_c and convert_float_c now accept decimal, numeric, money, smallmoney, real, float and character sources.

Date/time targets

• Character sources parse YYYY-MM-DD, HH:MM[:SS[.fffffff]], and a combined datetime with either a T or a space separator, plus an optional trailing offset.
• The offset is matched only in the exact 6-character [+-]HH:MM shape, so the hyphens inside a date are never mistaken for one.
• Range validation on every field (year 1-9999, month 1-12, day 1-31, hour < 24, minute/second < 60, offset <= 14:59).

Diagnostics

Situation	Result
Lossy but well-defined conversion (float 1234.99 into SQL_C_SLONG)	value truncated toward zero, 01S07 + SQL_SUCCESS_WITH_INFO, matching msodbcsql18
Character text that is not a valid literal for the target	22018
Source with no numeric or temporal interpretation (binary, guid) into that target	07006, since the pairing is illegal rather than unimplemented

Notes for reviewers

• This removes the ConvError::NotHandledHere carve-out that odbc: typed SQLGetData conversion core (P1) [AB#46578] #146 left in convert_datetime_c. That branch existed only to report "not implemented yet" for character sources, and is what this PR implements, so character input now goes through parse_datetime_literal instead.
• Consequently get_data_character_into_date_target_is_not_implemented (a placeholder from odbc: typed SQLGetData conversion core (P1) [AB#46578] #146 asserting the deferral) is replaced by two tests: one asserting the conversion succeeds, and one asserting an invalid literal is 22018 rather than a silent zero value. The diagnostic coverage the old test provided is preserved.
• sql_string_to_text already moved into fetch_convert as part of odbc: typed SQLGetData conversion core (P1) [AB#46578] #146, so the equivalent move in this branch's first commit collapsed away during the rebase.

Known limitation — deliberately deferred (AB#47238)

A varchar(max) / nvarchar(max) source into a numeric or date/time target is not covered. Non-max character columns are unaffected. This is deferred rather than overlooked, for three reasons:

• Max-length columns arrive as PLP and are delivered incrementally, so parsing would require the ODBC layer to accumulate chunks. That inverts the invariant SQLFetch and SQLGetData update - column wise fetch and incremental PLP support #153 established — stream_active_plp_chunk documents "This never buffers the full PLP payload in ODBC-layer memory" — and that exception should be Shiwani Gupta (@shiwanigupta0809)'s call rather than a quiet one-off.
• Rewire mssql-odbc fetch hot path onto the reactor-free sync core (L5) #204 and Make the ODBC fetch path 2.3-3.9x cheaper on CPU (beats msodbcsql18 on varchar(max)/nvarchar) #215 are actively rewriting this code. Make the ODBC fetch path 2.3-3.9x cheaper on CPU (beats msodbcsql18 on varchar(max)/nvarchar) #215 changes read_active_plp_chunk itself and moves its call site from block_on to pin!; Rewire mssql-odbc fetch hot path onto the reactor-free sync core (L5) #204 rewires get_data.rs onto the reactor-free sync core. Anything written here now would conflict on a performance-critical hot path and would likely need rewriting once the sync core lands.
• There is a policy question to settle first: varchar(max) holds up to 2 GB, but a valid numeric or datetime literal is under ~64 bytes, so draining an unbounded payload to produce a SQL_C_SLONG is a memory-exhaustion risk driven by server-side data. The likely answer is a bounded prefix that fails with 22018 / 22003 past the cap, but that should be confirmed against msodbcsql18's actual behaviour before being encoded.

Tracked as AB#47238, sequenced after #204 and #215.

Testing

509 unit tests pass, workspace clippy and cargo fmt clean.

AB#47107

[Copilot]

Pull request overview

Extends typed SQLGetData to support mandatory numeric and temporal source conversions.

Changes:

• Adds exact numeric-source conversion and truncation reporting.
• Parses character values into numeric and date/time targets.
• Maps invalid literals to 22018 and updates tests/documentation.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.

File	Description
mssql-odbc/src/api/fetch_convert.rs	Implements source conversions and tests.
mssql-odbc/src/api/get_data.rs	Maps conversion errors and tests diagnostics.
mssql-odbc/docs/typed-columnar-fetch-plan.md	Marks P1a implemented.

Suppressed comments (2)

mssql-odbc/src/api/fetch_convert.rs:586

• Fractions longer than seven digits are silently truncated by min(7), but the conversion still returns ConvOk::Exact. Thus 12:00:00.12345678 loses data without 01S07, despite the documented grammar allowing at most seven digits. Reject excess digits (and a bare decimal point) as invalid input, or explicitly report truncation.

    if !frac_digits.is_empty() && !frac_digits.bytes().all(|b| b.is_ascii_digit()) {
        return None;
    }
    // Normalize the fraction to 100 ns resolution (7 digits).
    let scale = frac_digits.len().min(7);

mssql-odbc/src/api/fetch_convert.rs:607

• This byte-offset string slice can panic when the final six bytes begin inside a multi-byte UTF-8 character (for example, é12345). SQLGetData calls this while holding the statement mutex, so the panic poisons the handle instead of returning the intended 22018; obtain the suffix through str::get so a non-boundary offset is treated as no offset.

    if s.len() >= 6 {
        let tail = &s[s.len() - 6..];

---

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

[github-actions]

📊 Code Coverage Report

🔥 Diff Coverage 98%

🎯 Overall Coverage 91.2%

📦 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-odbc/src/api/fetch_convert.rs (98.0%): Missing lines 149,196,215,566,599,610,641,673,677
• mssql-odbc/src/api/get_data.rs (100%)

Summary

• Total: 475 lines
• Missing: 9 lines
• Coverage: 98%

mssql-odbc/src/api/fetch_convert.rs

  145                 Some((mantissa / divisor, mantissa % divisor != 0))
  146             }
  147             NumericSource::Float(f) => {
  148                 if !f.is_finite() || !(-1.7e38..=1.7e38).contains(&f) {
! 149                     return None;
  150                 }
  151                 Some((f.trunc() as i128, f.fract() != 0.0))
  152             }
  153         }

---

  192 /// numeric interpretation.
  193 fn numeric_source(value: &ColumnValues) -> Option<NumericSource> {
  194     match value {
  195         ColumnValues::TinyInt(x) => Some(NumericSource::Int(i128::from(*x))),
! 196         ColumnValues::SmallInt(x) => Some(NumericSource::Int(i128::from(*x))),
  197         ColumnValues::Int(x) => Some(NumericSource::Int(i128::from(*x))),
  198         ColumnValues::BigInt(x) => Some(NumericSource::Int(i128::from(*x))),
  199         ColumnValues::Bit(b) => Some(NumericSource::Int(i128::from(*b))),
  200         ColumnValues::Real(x) => Some(NumericSource::Float(f64::from(*x))),

---

  211         ColumnValues::SmallMoney(m) => Some(NumericSource::Scaled {
  212             mantissa: i128::from(m.int_val),
  213             scale: 4,
  214         }),
! 215         ColumnValues::String(_) => None,
  216         _ => None,
  217     }
  218 }

---

  562             } else {
  563                 28
  564             }
  565         }
! 566         _ => 0,
  567     }
  568 }
  569 
  570 /// Parses `YYYY-MM-DD`.

---

  595     let hour: u16 = it.next()?.parse().ok()?;
  596     let minute: u16 = it.next()?.parse().ok()?;
  597     let sec_part = it.next().unwrap_or("0");
  598     if it.next().is_some() {
! 599         return None;
  600     }
  601     let (sec_digits, frac_digits) = match sec_part.split_once('.') {
  602         Some((a, b)) => (a, b),
  603         None => (sec_part, ""),

---

  606     if hour > 23 || minute > 59 || second > 59 {
  607         return None;
  608     }
  609     if !frac_digits.is_empty() && !frac_digits.bytes().all(|b| b.is_ascii_digit()) {
! 610         return None;
  611     }
  612     // Normalize the fraction to 100 ns resolution (7 digits).
  613     let scale = frac_digits.len().min(7);
  614     let mut hundred_ns: u32 = 0;

---

  637             let sign: i16 = if tb[0] == b'+' { 1 } else { -1 };
  638             let hh: i16 = tail[1..3].parse().ok()?;
  639             let mm: i16 = tail[4..6].parse().ok()?;
  640             if hh > 14 || mm > 59 {
! 641                 return None;
  642             }
  643             p.tz_hour = sign * hh;
  644             p.tz_minute = sign * mm;
  645             p.has_tz = true;

---

  669         p.scale = scale;
  670         p.has_time = true;
  671     }
  672     if !p.has_date && !p.has_time {
! 673         return None;
  674     }
  675     // An offset is only meaningful alongside a date and time.
  676     if p.has_tz && !(p.has_date && p.has_time) {
! 677         return None;
  678     }
  679     Some(p)
  680 }

---

---

🔗 Quick Links

View Azure DevOps Build · Coverage Report

[David Engel (David-Engel)]

Review — P1a source-type conversions

Overall this is good work. The NumericSource abstraction is the right call — keeping exact-decimal sources exact so an integer target can report 01S07 instead of silently dropping a fraction is what Appendix D wants, and the 22018 vs. 07006 split (bad text for a legal pairing vs. an illegal pairing) is correct and well covered by tests. Docs and the phase table are updated, and the e2e HYC00 anchor was re-pointed to SQL_C_NUMERIC with a rationale for why that anchor is durable.

Verified locally on 37434368: 512 unit tests pass, cargo clippy -p mssql-odbc --all-targets is clean.

One blocker below.

---

🔴 Blocking — panic on non-ASCII character input

fetch_convert.rs#L633-L634

if s.len() >= 6 {
    let tail = &s[s.len() - 6..];

s.len() is bytes, so s.len() - 6 can land inside a multi-byte UTF-8 sequence and &s[..] panics. A panic unwinding through the ODBC extern "C" boundary is UB / process abort in the Driver Manager, driven by server-side data.

Repro, confirmed against this branch:

SELECT N'日aaaa'  ->  SQLGetData(1, SQL_C_TYPE_DATE, ...)

panicked at mssql-odbc/src/api/fetch_convert.rs:634:22:
start byte index 1 is not a char boundary; it is inside '日' (bytes 0..3) of `日aaaa`

The offset shape is pure ASCII, so the cheapest fix is to work on bytes and never slice the str:

if let Some(tail) = s.as_bytes().get(s.len() - 6..) {
    if (tail[0] == b'+' || tail[0] == b'-')
        && tail[3] == b':'
        && tail[1..3].iter().chain(&tail[4..6]).all(u8::is_ascii_digit)
    {
        // ...
    }
}

as_bytes().get(..) returns None rather than panicking, and the explicit digit check also removes the tail[1..3].parse() slicing. Worth a regression test that drives a non-ASCII literal into each date/time target.

🟡 'inf' / 'NaN' text is accepted as a number

fetch_convert.rs#L223-L231

The fallback is text.trim().parse::<f64>(), and Rust's parser accepts inf, infinity and nan case-insensitively. Confirmed behaviour:

• 'inf' into SQL_C_DOUBLE returns Ok(Exact) and writes inf into the application's buffer.
• 'NaN' does the same, writing NaN.
• Either into SQL_C_SLONG reports 22003 (to_i128_truncating returns None) where msodbcsql would report 22018.

SQL Server has no inf / nan literal, so this should be InvalidCharacterValue. An f.is_finite() guard on the fallback covers it.

🟡 Stale doc comment

fetch_convert.rs#L305-L308 — deleting numeric_source_as_f64 left its doc comment stranded on top of is_float_c_target, which now carries two contradictory doc blocks:

/// Widen a numeric column (integer or floating) to `f64`. Returns `None` for
/// non-numeric sources.
/// Returns `true` if `target_type` is one of the floating-point C types handled
/// by [`convert_float_c`].
pub(crate) fn is_float_c_target(target_type: SqlSmallInt) -> bool {

Nits

• fetch_convert.rs#L187 — money_scaled duplicates the (lsb & 0xFFFF_FFFF) | (msb << 32) assembly already written inline at get_data.rs#L1023. One shared helper keeps the two from drifting.
• fetch_convert.rs#L215 — ColumnValues::String(_) => None immediately followed by _ => None is redundant. If it is there for documentation, a comment pointing at numeric_source_or_parse carries that better than a duplicate arm.
• fetch_convert.rs#L613 — fractional seconds past 7 digits are silently discarded: 12:34:56.123456789 yields 123456700 with no ConvOk::Truncated. That is fractional truncation and arguably deserves 01S07, same as the other paths this PR adds.
• fetch_convert.rs#L205 — parse_decimal_literal(&d.to_string()) formats and re-parses per value on the fetch path. The exactness argument is sound; a short comment noting the allocation is accepted for now (or a follow-up item) would help whoever profiles this later.
• convert_integer_c now gates on is_integer_c_target at entry, so the trailing _ => return Err(NotHandledHere) inside the match is unreachable by construction. Harmless, but if the gate and the match ever diverge the gate wins silently.
• Edge case: '0.5' into SQL_C_BIT writes 0 + 01S07. Plausible, but msodbcsql likely returns 22003 — worth confirming on the parity leg at some point.

CI

ADO validation reports "had test failures": TestAuthTcClash::test_row27_tc_yes_ad_interactive and test_tc_yes_ad_default, both failing with mssql_python.auth does not have the attribute 'get_auth_token'. That is cross-repo mock drift, unrelated to this change.

[David Engel (David-Engel)]

All addressed in 2a91adb. 516 tests pass (+4 regression tests), workspace clippy and cargo fmt clean.

🔴 Panic on non-ASCII character input

Fixed, and thank you — this was the right call to block on. The probe now works on bytes and never slices the str:

if let Some(tail) = s.len().checked_sub(6).and_then(|i| s.as_bytes().get(i..))
    && (tail[0] == b'+' || tail[0] == b'-')
    && tail[3] == b':'
    && tail[1..3].iter().chain(&tail[4..6]).all(u8::is_ascii_digit)

checked_sub also removes the underflow path, the digit check retires the tail[1..3].parse() slicing, and the surviving s[..s.len() - 6] is now only reachable once the tail is known to be ASCII, so that boundary is guaranteed to be a char boundary. Added non_ascii_character_input_does_not_panic, which drives 日aaaa into SQL_C_TYPE_DATE, SQL_C_TYPE_TIME and SQL_C_TYPE_TIMESTAMP.

🟡 'inf' / 'NaN'

Fixed — the f64 fallback now requires is_finite(), so these are 22018 rather than writing a non-finite value into the caller's buffer. Covered for inf, -inf, Infinity, NaN and nan.

🟡 Stale doc comment

Removed. That is the third orphaned doc block this PR has produced, all from deleting or inserting items next to a doc comment — worth a glance in review whenever a helper is removed.

Nits

• money_scaled duplication — now shared; get_data.rs calls it instead of reassembling the wire value inline. Changed its return to i64 so both callers use it directly.
• Redundant String(_) => None — dropped, replaced with a comment pointing at numeric_source_or_parse as you suggested.
• Fractional seconds past 7 digits — agreed, that is truncation. DateTimeParts now carries frac_truncated and convert_datetime_c reports 01S07. 12:34:56.123456789 yields 123456700 + 01S07.
• d.to_string() allocation — comment added recording that the format-and-reparse is accepted in exchange for exactness.
• Unreachable _ arm — kept as a backstop with a comment saying the is_integer_c_target gate already rejected everything else.

'0.5' into SQL_C_BIT — checked against the msodbcsql source, and the hypothesis is half right

Worth spelling out, because the answer is narrower than expected. From sqlccnvt.cpp:

if (!fUnsignedIn && Error == CVT_FRACT_TRUNC && fTypeOut == SQL_C_BIT)
    return CVT_PREC;

with CVT_PREC = IDS_22_003 = "Numeric value out of range", and fUnsignedIn set when the text carries no -. So:

• '0.5' into SQL_C_BIT — positive, so this branch does not fire; it truncates to 0 with CVT_FRACT_TRUNC (IDS_01_S07). We already matched.
• '-0.5' into SQL_C_BIT — negative, so msodbcsql returns 22003, while we returned 01S07. That is the real divergence, and the double path agrees (dTemp < 0 && fTypeOut == SQL_C_BIT → CVT_PREC).

Fixed by rejecting a negative source for a bit target before the truncation check, with negative_into_bit_target_is_out_of_range covering it.

CI

Agreed, unrelated. TestAuthTcClash fails on mssql_python.auth does not have the attribute 'get_auth_token', which is a mock in the cross-repo mssql-python leg; nothing in this PR touches auth. Re-running validation on the new head.

[David Engel (David-Engel)]

Re-review — all previous findings addressed

Confirmed fixed on 2a91adb:

• Non-ASCII panic is gone. The byte-slice + let-chain rewrite is clean, the "matched tail is all ASCII, so this boundary is a char boundary" comment justifies the one remaining str slice, and non_ascii_character_input_does_not_panic covers all three targets.
• inf / NaN text now rejected, money_scaled shared with get_data.rs, stale doc comment removed, redundant String arm replaced with a useful comment, allocation trade-off documented, unreachable arm annotated.
• The SQL_C_BIT negative case you added is a real msodbcsql behaviour I had only guessed at, and the citation is accurate.

516 unit tests pass, cargo clippy -p mssql-odbc --all-targets clean.

Parity pass against msodbcsql

I read Sql/Ntdbms/sqlncli/odbc/sqlccnvt.cpp for this round. Verified matching, worth recording so nobody re-litigates them:

Behaviour	msodbcsql	this PR
Float to integer rounds or truncates	modf(dTemp, &dTemp) != 0. — truncate toward zero	same
Trailing fractional zeros ('42.00' to SQL_C_SLONG)	if (c != '0') Error = CVT_FRACT_TRUNC — no warning	mantissa % divisor != 0 — no warning
money / smallmoney to integer	% MONEYMULT sets truncation, / MONEYMULT is the value	Scaled { scale: 4 }
Exact vs f64 path for character	scans for e/E, then CharToBigint else CharToDouble	parse_decimal_literal then f64 fallback
Integer target bounds	MIN_LIMIT_ADJUST is 0 in the driver, so the exact target range	try_from
Negative into SQL_C_BIT	CVT_PREC via two separate guards	is_negative() covers both
Calendar validation	ValidateDateTimeOffsetStruct	days_in_month
Time literal to SQL_C_TYPE_DATE, date literal to SQL_C_TYPE_TIME	CVT_CAST_ERROR	22018

The divergences I found are inline. None are regressions from the previous round except the first, which is fallout from my own suggestion — see the note on numeric_source_or_parse.

Not blocking, no good inline anchor

SQL_C_TINYINT bound for the newly-allowed sources. msodbcsql caps SQL_C_TINYINT at SCHAR_MAX (127) whenever fTypeIn is not itself a tinyint C type (sqlccnvt.cpp, the case SQL_C_TINYINT: guard fTypeIn != SQL_C_TINYINT && fTypeIn != SQL_C_STINYINT && fTypeIn != SQL_C_UTINYINT && ... Temp > SCHAR_MAX). So CAST('200' AS varchar(3)) into SQL_C_TINYINT is 22003 there; here it is Ok(200) (confirmed by probe). The existing u8 mapping is well-reasoned for a real tinyint column and I would not change it, but this PR is what first opens character / decimal / float sources into that target, which is exactly the case where msodbcsql applies the signed limit. Worth a line in the plan doc so the parity leg does not surprise someone later.

Whitespace. str::trim() trims all Unicode whitespace; FindSigNumber / SkipWSSequence handle ASCII blanks. Permissive, almost certainly harmless.

[Saurabh Singh (saurabh500)]

Review summary

Went through this one file at a time against the ODBC spec (Appendix D, "Converting Data from SQL to C Data Types") rather than only against the msodbcsql citations in the comments, and ran the suite locally: 537/537 mssql-odbc tests pass, clippy clean at -D warnings.

No blocking issues. The conversion matrix is right, the SQLSTATE choices match Appendix D everywhere I checked independently, and the re-readability guarantee after a failed conversion genuinely holds (write_captured_column only clears last_captured when rc != SQL_ERROR).

Findings

#	Where	Finding
1	fetch_convert.rs:211-216	The decimal arm format-and-reparses: 3-6 heap allocations per cell in the columnar fetch path, and it newly exposes an unguarded << (i * 32) in to_decimal_string. Suggestion inline; it is exact, allocation-free and bounds-checked.
2	get_data_test.cpp:577 + plan doc	The SQL_C_NUMERIC permanent non-goal is recorded only in a C++ test comment, and it contradicts Appendix D. It belongs in the divergence table.
3	fetch_convert.rs:604, :622	Leading + is accepted in date and time components -- '+123-01-01' parses as year 123. Undocumented divergence.

Everything else I found was cosmetic and not worth your time, so I have left it out.

On finding 1

Worth being precise, because it is the one that could bite in production. read_decimal_data derives the limb count from a wire length byte and caps it at 64, not 4 (decoder.rs:670), so a DecimalParts with more than 4 limbs is reachable from a malformed server payload. to_decimal_string then shifts by 128 or more. I ran it both ways:

• debug: panics, attempt to shift left with overflow -- which aborts across the extern "C" boundary
• release: silently returns garbage; a [1,0,0,0,1] payload yields mantissa 2

This is pre-existing, not introduced here -- P1's SQL_C_CHAR path already called into it. But P1a adds a second call site, so it seemed worth flagging now rather than filing quietly. The suggested rewrite closes it at this call site as a side effect. Happy for the underlying to_decimal_string fix to be a separate PR.

I checked the suggested replacement against the current code across zero, negative zero, 123.45, -0.01, the 0.005 zero-padding branch, a 2-limb value and the 4-limb near-10^38 maximum at scale 0 and 38: identical results, zero allocations, and fmt/clippy/537 tests green with it applied.

Open questions

1. Build Stage Build MacOS is red at 1h00m while Test MacOS passed in 36m -- that reads like a timeout rather than a real failure, but it should be green or explained before merge.
2. Is SQL_C_NUMERIC -> HYC00 genuinely permanent, or just out of P1a? The C++ comment says permanent; if so it needs a divergence-table row (suggestion inline).
3. A character literal with no offset going into SQL_C_SS_TIMESTAMPOFFSET writes offset 0. Deliberate, or should it be 22018?

What is good here

The byte-wise offset probe in parse_datetime_literal is the standout -- probing the trailing [+-]HH:MM through as_bytes().get(i..) instead of slicing by character index removes a real panic on input like N'日aaaa', and a panic in this crate is a process abort for the host application. That is exactly the right instinct for FFI code.

Beyond that: every parity decision cites the specific msodbcsql source file it came from, which made this reviewable in a way that "matches msodbcsql" never is; the unit tests are genuinely exhaustive rather than happy-path; the varchar(max) deferral is argued honestly instead of being quietly half-implemented, with the PLP invariant it would violate spelled out; and the PR corrects two claims the P1 doc made about itself rather than letting them stand. The new divergence table is the right artifact to have built -- my main note on it is that it should be more complete, which is a good problem for a doc to have.

[Saurabh Singh (saurabh500)]

Walked the four files against Appendix D and ran the suite locally (537/537 mssql-odbc tests, clippy clean at -D warnings). No blocking findings -- the conversion matrix and SQLSTATE choices hold up. Leaving this as a comment rather than an approval only because the macOS build leg is red and the questions in my summary comment are still open; happy to flip to approve once those settle.

Six comments, each with an applyable suggestion. The three that matter:

1. fetch_convert.rs:211 -- the decimal arm format-and-reparses (3-6 allocations per cell), and newly exposes an unguarded << (i * 32) in to_decimal_string that panics in debug / corrupts in release for >4-limb payloads. Pre-existing, but this PR adds a second call site.
2. get_data_test.cpp:577 + plan doc -- the SQL_C_NUMERIC permanent non-goal lives only in a C++ comment and contradicts Appendix D.
3. fetch_convert.rs:604, :622 -- leading + accepted in date/time components ('+123-01-01' parses as year 123).

Plus a request for TODO(convergence): markers on the temporary SKIP_IF_COMPARING_MSODBCSQL() sites, following the convention already used at line 819.

One correction to my summary comment, on the divergence table: it says these are recorded because "GetDataLiveTest skips the msodbcsql comparison leg for these cases." That is not quite what is happening -- none of the five table rows has a GetDataLiveTest case at all (there is no SQL_C_TINYINT or SQL_C_SS_TIMESTAMPOFFSET anywhere in the e2e suite). A skipped test still asserts on the Rust leg; a missing one pins nothing, so those five are covered only by Rust unit tests, which cannot see msodbcsql. Not worth holding the PR for, but the sentence is misleading as written.

[Saurabh Singh (saurabh500)]

Nice

[Saurabh Singh (saurabh500)]

This arm renders the decimal to a String and parses it straight back. Two problems with that.

The allocation cost is higher than the comment implies. I measured it with a counting allocator: 3-6 heap allocations per cell (6 for negative or zero-padded values), because Display::fmt calls to_decimal_string() -- which allocates internally -- and to_string() then allocates again to receive the result. This is the per-cell path of a columnar fetch.

More importantly, this makes to_decimal_string newly reachable from the numeric targets, and that function has an unguarded shift:

// mssql-tds/src/datatypes/decoder.rs:2011
.fold(0u128, |acc, (i, &part)| acc | ((part as u32 as u128) << (i * 32)))

read_decimal_data derives the limb count from a wire length byte and caps it at 64, not 4 (decoder.rs:670), so int_parts.len() > 4 is reachable from a malformed server payload. I ran that path both ways: debug builds panic with "attempt to shift left with overflow", which aborts across the extern "C" boundary, and release builds silently return garbage (a [1,0,0,0,1] payload yields mantissa 2, because a shift of 128 wraps to 0). P1's SQL_C_CHAR path already reached this, so it is pre-existing rather than introduced here, but P1a adds a second call site.

The limbs are a plain base-2^32 little-endian magnitude, so reassembling them directly is exact, allocation-free, and bounds-checked:

Suggested change

	// `DecimalParts` renders itself exactly; parse that back rather than
	// reassembling its base-2^32 limbs. The format-and-reparse allocates per
	// value; accepted for now in exchange for exactness.
	ColumnValues::Decimal(d) | ColumnValues::Numeric(d) => {
	parse_decimal_literal(&d.to_string())
	}
	// `DecimalParts` stores a base-2^32 little-endian magnitude; reassemble
	// it directly. 38 digits fit in 4 limbs, and the wire decoder admits up
	// to 64, so reject longer payloads rather than shifting past 128 bits.
	ColumnValues::Decimal(d) | ColumnValues::Numeric(d) => {
	if d.int_parts.len() > 4 {
	return None;
	}
	let mag = d.int_parts.iter().enumerate().fold(0u128, |acc, (i, &p)| {
	acc | (u128::from(p as u32) << (i * 32))
	});
	let m = i128::try_from(mag).ok()?;
	Some(NumericSource::Scaled {
	mantissa: if d.is_positive { m } else { -m },
	scale: u32::from(d.scale),
	})
	}

I diffed this against the current behaviour across zero, negative zero, 123.45, -0.01, the 0.005 padding branch, a 2-limb value, and the 4-limb near-10^38 maximum at both scale 0 and scale 38 -- identical results, zero allocations. With it applied, cargo fmt, clippy -D warnings, and all 537 mssql-odbc tests pass.

One thing this deliberately does not change: a genuinely out-of-range magnitude still returns None and surfaces as 07006 rather than 22003, because numeric_source returns Option. Fixing that means changing the signature to Result<NumericSource, ConvError> -- worth a follow-up, but bigger than this change.

[Saurabh Singh (saurabh500)]

str::parse accepts a leading +, and the y.len() != 4 check above does not exclude it -- so '+123-01-01' is accepted as year 123. Month and day have no length check at all, so '2023-+5-01' gives month 5.

I confirmed the parsing behaviour rather than assuming it: "+123".parse::<i16>() is Ok(123) and "+5".parse::<u16>() is Ok(5). (-0 and leading whitespace are correctly rejected, so this is only about +.)

This is not in the divergence table, and I do not think it fits the "permissive about formatting" row either -- accepting 2023-6-5 is a padding relaxation, whereas accepting a sign on a year or month admits a value with no meaning.

Suggested change

	let year: i16 = y.parse().ok()?;
	let month: u16 = m.parse().ok()?;
	let day: u16 = d.parse().ok()?;
	// `str::parse` accepts a leading `+`, which would make `+123-01-01` a valid
	// date; require plain digits.
	if !y
	.bytes()
	.chain(m.bytes())
	.chain(d.bytes())
	.all(|b| b.is_ascii_digit())
	{
	return None;
	}
	let year: i16 = y.parse().ok()?;
	let month: u16 = m.parse().ok()?;
	let day: u16 = d.parse().ok()?;

If you would rather stay permissive here, that is defensible -- but then it should be an explicit row in the divergence table rather than an accident of str::parse.

[Saurabh Singh (saurabh500)]

Same leading-+ issue as in parse_date_literal, and here it affects all three components -- '+1:00:00', '01:+5:00' and '01:00:+5' all parse. The fractional digits are already checked at line 636, so it is just hour/minute/second that are open.

Restructuring slightly so the three component strings are available before parsing keeps it to a single check:

Suggested change

	let hour: u16 = it.next()?.parse().ok()?;
	let minute: u16 = it.next()?.parse().ok()?;
	let sec_part = it.next().unwrap_or("0");
	if it.next().is_some() {
	return None;
	}
	let (sec_digits, frac_digits) = match sec_part.split_once('.') {
	Some((a, b)) => (a, b),
	None => (sec_part, ""),
	};
	let second: u16 = sec_digits.parse().ok()?;
	let hour_s = it.next()?;
	let minute_s = it.next()?;
	let sec_part = it.next().unwrap_or("0");
	if it.next().is_some() {
	return None;
	}
	let (sec_digits, frac_digits) = match sec_part.split_once('.') {
	Some((a, b)) => (a, b),
	None => (sec_part, ""),
	};
	// `str::parse` accepts a leading `+`, which would make `+1:00:00` a valid
	// time; require plain digits.
	if !hour_s
	.bytes()
	.chain(minute_s.bytes())
	.chain(sec_digits.bytes())
	.all(|b| b.is_ascii_digit())
	{
	return None;
	}
	let hour: u16 = hour_s.parse().ok()?;
	let minute: u16 = minute_s.parse().ok()?;
	let second: u16 = sec_digits.parse().ok()?;

Empty components still fail at parse, so '.5' and '01:' are rejected exactly as before. I verified fmt, clippy and all 537 tests pass with this and the date change applied together.

[Saurabh Singh (saurabh500)]

This comment is currently the only place the permanent non-goal is recorded, and it is load-bearing -- it is the whole justification for why this test's anchor will not need to move again later.

That is worth promoting out of a test comment. Appendix D lists SQL_C_NUMERIC as a valid target for every numeric and character source, so a reader coming from the spec will read HYC00 here as "not implemented yet", which is the opposite of what you mean. Every other deliberate departure in this PR is recorded in the "Known divergences from msodbcsql" table in docs/typed-columnar-fetch-plan.md; this is the one that is permanent, so it has the strongest claim to a row there.

Suggested change

	// An unsupported C target type is rejected with HYC00 and does not consume the
	// column. SQL_C_NUMERIC is the durable anchor for this: emitting the
	// SQL_NUMERIC_STRUCT is an explicit non-goal (decimal is delivered as character
	// data, which is what mssql-python requests), so unlike the other C targets it
	// is not scheduled to become supported.
	// An unsupported C target type is rejected with HYC00 and does not consume the
	// column. SQL_C_NUMERIC is the durable anchor for this: emitting the
	// SQL_NUMERIC_STRUCT is a permanent non-goal, recorded in the "Known divergences
	// from msodbcsql" table in docs/typed-columnar-fetch-plan.md, so unlike the
	// other C targets it is not scheduled to become supported.

(Paired with a suggestion adding that row to the plan doc.)

[Saurabh Singh (saurabh500)]

Two divergences I hit while reviewing are not in this table, and both are the kind that only ever gets found again by someone reading the source.

The SQL_C_NUMERIC one matters most: it is currently documented only as a C++ comment in get_data_test.cpp, it contradicts Appendix D, and unlike every other gap here it is described as permanent. This table is where a user or a future maintainer would look for it.

Suggested change

	| `T` separator, `HH:MM` without seconds, unpadded fields such as `2023-6-5` | rejected (fixed-length token grammar) | accepted | Permissive. Low risk, same task. |
	| A time-only value into `SQL_C_TYPE_TIMESTAMP` | fills in the current date and succeeds, per Appendix D | `22018` from a character source, `07006` from a `time` column | Gap — Task [47247](https://sqlclientdrivers.visualstudio.com/mssql-rs/_workitems/edit/47247). Needs a platform-specific local-date helper, so it is not a one-line fix. |
	| `T` separator, `HH:MM` without seconds, unpadded fields such as `2023-6-5` | rejected (fixed-length token grammar) | accepted | Permissive. Low risk, same task. |
	| A time-only value into `SQL_C_TYPE_TIMESTAMP` | fills in the current date and succeeds, per Appendix D | `22018` from a character source, `07006` from a `time` column | Gap — Task [47247](https://sqlclientdrivers.visualstudio.com/mssql-rs/_workitems/edit/47247). Needs a platform-specific local-date helper, so it is not a one-line fix. |
	| Any source into `SQL_C_NUMERIC` | converts, per Appendix D | `HYC00` | **Deliberate, and permanent.** Decimal is delivered as character data, which is what mssql-python requests, so `SQL_NUMERIC_STRUCT` is not scheduled to become supported. Anchored by `UnsupportedCTypeReturnsHyc00ThenValueReadable`. |
	| A leading `+` on a date or time component (`+123-01-01`, `+1:00:00`) | rejected | accepted — `str::parse` allows the sign, so the component parses | Permissive, same Task [47246](https://sqlclientdrivers.visualstudio.com/mssql-rs/_workitems/edit/47246) as the grammar rows above. Drop this row if the parser is tightened instead. |

[Saurabh Singh (saurabh500)]

This skip is temporary -- the comment says msodbcsql "very likely agrees" and the only thing keeping the skip is that IDS_22_005 has not been confirmed against a live run. If it does agree, this test should stop being skipped and start comparing on both legs. But nothing in the file records that intent in a form anyone will find later.

There is already a convention for exactly this in this file -- PlpZeroCapacityBufferDoesNotSpin (line 819):

// TODO(convergence): mssql-odbc will eventually adopt the msodbcsql
// truncate-and-continue contract for sub-minimal buffers [...] at which point
// this HY090 rejection goes away and the assertion can run on both legs.

That is greppable and states the exit condition. Worth applying the same treatment here:

Suggested change

	// Still skipped on the msodbcsql leg: msodbcsql implements this conversion and
	// its CVT_CAST_ERROR carries the "Invalid character value for cast
	// specification" message, so it very likely agrees, but the constant is spelled
	// IDS_22_005 in its source and that has not been confirmed against a live run.
	// TODO(convergence): this skip is temporary. msodbcsql implements this
	// conversion and its CVT_CAST_ERROR carries the "Invalid character value for
	// cast specification" message, so it very likely agrees, but the constant is
	// spelled IDS_22_005 in its source and that has not been confirmed against a
	// live run. Confirm against a live msodbcsql run, then drop the skip so this
	// compares on both legs; if the two do not agree, record the difference in the
	// "Known divergences from msodbcsql" table in docs/typed-columnar-fetch-plan.md.

Marking the temporary ones also makes the permanent ones unambiguous by absence, which matters for the SQL_C_NUMERIC test right below this one -- that skip is never going away, and right now the two read identically.

For context, of the eight SKIP_IF_COMPARING_MSODBCSQL() sites in this file, five are temporary and only one carries a marker:

Line	Test		Marked
174	BackwardColumnRejectedRereadIsNoData	permanent (msodbcsql non-conformant)	n/a
557	InvalidCharacterForNumericTargetIs22018ThenValueReadable	temporary	no
583	UnsupportedCTypeReturnsHyc00ThenValueReadable	permanent (explicit non-goal)	n/a
608	VarbinaryMaxToCharReturnsHyc00	temporary (binary→char unimplemented)	no
656	PlpColumnUnsupportedCTypeReturnsHyc00	temporary (Task 47238)	no
794	NvarcharMaxToCharChunkedAstralRoundTrip	permanent (UTF-8 vs ANSI, Windows-only)	n/a
825	PlpZeroCapacityBufferDoesNotSpin	temporary	yes
911	UnsupportedColumnTypeHyc00PreservesValue	temporary (VARBINARY anchor)	no

Only line 557 is yours; 608, 656 and 911 are pre-existing, so no objection if you would rather sweep those in a follow-up. Task 47238 and the binary work already have homes to link to.