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)]

Re-reviewed at 69487db. All six comments from the last round are addressed, and I checked each against the code rather than the replies.

The limb reassembly is correct at every boundary I could think to try: negative i32 limbs reinterpret correctly, 38 nines round-trips in both signs, and a 4-limb magnitude at or above 2^127 returns None through i128::try_from rather than wrapping to a negative. | is safe because the limbs occupy disjoint 32-bit windows. The as u32 cast matches what the decoder already does, so the two crates agree on the encoding.

The leading-+ tightening does not cost anything legitimate. 2023-6-5 is still accepted, so the documented unpadded-field divergence survives; a whole-literal - still takes its separate path; empty components still fail at parse as before; and the fractional component was already gated, so the new check leaves no gap.

Both new tests fail if their guard is reverted, which is what I care about. cargo fmt, cargo clippy -D warnings and 539/539 nextest are all clean locally.

Not adding the leading-+ divergence row was the right call -- tightening the parser removes the divergence rather than documenting it, so there is nothing left to record. Marking only this PR's own skip and sweeping the rest separately is also fine by me, and your count of three remaining temporary skips matches my survey; 174 and 797 are permanent and should stay unmarked.

One finding left, on the new test. It is coverage rather than correctness, so it is not merge-blocking from my side. Everything shipping here looks right to me.

Leaving this as a comment rather than an approval only because the build legs are still running on this commit.

[Saurabh Singh (saurabh500)]

I have one last comment after which this code can be approved.