microsoft / microsoft/mssql-rs

Verify decimal_from_numeric's precision_scale_explicit persistence across a same-C-type rebind against retail msodbcsql

Open
#536 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
Rust
Stars
53
Forks
14
Avg merge
1d 15h
Merged PRs (30d)
137

Description

Follow-up from PR #521 review (https://github.com/microsoft/mssql-rs/pull/521#discussion_r3971705057, David-Engel).

## The question

`BoundParam::write_to_records` (`mssql-odbc/src/params/bound_param.rs`) only clears `precision_scale_explicit` when the C type is actually changing:

```rust
let type_changing = apd_record.concise_type != self.c_type;
...
if type_changing {
apd_record.precision_scale_explicit = false;
}
// values always reset to SQL_C_NUMERIC's type default (38, 0), even on a same-type rebind
if self.c_type == SQL_C_NUMERIC {
apd_record.precision = SQL_PREC_NUMERIC;
apd_record.scale = 0;
}
```

`decimal_from_numeric` (`mssql-odbc/src/conversion/param_convert.rs:955-993`) uses that flag to decide whether to trust the APD's `(app_precision, app_scale)` (fast path — forward the embedded `SQL_NUMERIC_STRUCT` metadata unchanged) or fall back to the struct's own embedded scale (slow path — rescale to the IPD's declared scale).

Because the flag only clears on a **type change**, not on every bind, the following sequence takes the fast path even though the app never called `SQLSetDescField` for the *second* bind:

1. `SQLBindParameter(ordinal 1, SQL_C_NUMERIC, SQL_DECIMAL, 5, 3, ...)`, then `SQLSetDescFieldW(SQL_DESC_PRECISION, 5)` / `SQLSetDescFieldW(SQL_DESC_SCALE, 3)` — a deliberate, explicit APD write matching the column exactly. `precision_scale_explicit` becomes `true`.
2. A **bare** rebind at the same ordinal/statement (no `SQL_RESET_PARAMS` in between): `SQLBindParameter(ordinal 1, SQL_C_NUMERIC, SQL_DECIMAL, 38, 0, ...)`. Since the C type (`SQL_C_NUMERIC`) hasn't changed, `precision_scale_explicit` stays `true`, and the value reset sets `(app_precision, app_scale) = (38, 0)` — which now coincidentally equals the fresh IPD's `(38, 0)`, so `decimal_from_numeric`'s fast-path gate passes and the embedded struct's metadata is forwarded as-is, unrescaled.

This differs from the **same bare `(38, 0)` bind on a fresh ordinal that was never explicitly bound**, which is deliberately tested by `NumericStructWithoutDescriptorFieldWritesUsesDefaultApdScale` and takes the slow path (rescales using the struct's own embedded scale, truncating `12.345` to `"12"`).

Retail's `SetADRecBP` re-applies `SetTypeDefaults` (the same `(38, 0)` value reset) on *every* `SQLBindParameter` call regardless of C-type change, per the existing doc comment in `write_to_records`. What's unconfirmed is whether retail's *fast/slow path decision itself* (not just the value reset) is keyed off some analogous "was this ordinal's precision/scale ever explicitly authored" bit that would similarly persist across a same-type rebind, or whether retail's decision is purely a function of the APD's *current* values with no such memory — in which case this driver's `precision_scale_explicit` retention would be a genuine divergence for this specific sequence.

## What's needed

A dedicated e2e test (`mssql-odbc/tests/e2e/tests/param_conversions_test.cpp`), run with `--compare-with-msodbcsql` (see `tests/e2e/README.md`), that:
1. Binds ordinal 1 with an explicit `SQLSetDescField` precision/scale matching the column exactly (forcing the fast path).
2. Rebinds the same ordinal, same statement, no reset, with a bare `(38, 0)` `SQLBindParameter` call and a struct embedding a different, non-`(38,0)` precision/scale.
3. Compares the result against retail msodbcsql.

If retail rescales in this case (i.e. does **not** retain the fast path across the bare rebind), `precision_scale_explicit` needs to reset on every bind, not just on a C-type change — but note `NumericRebindDoesNotInheritAPreviousBindsStaleApdScale` (`bound_param.rs`) already depends on the flag surviving a same-type rebind in a *different* sub-case (a same-type rebind where the freshly-reset `(38, 0)` values must still be treated as deliberate rather than default), so the fallback rule in `decimal_from_numeric` (`param_convert.rs:1000`) would need to move with it rather than a straight revert.

A quick differential probe against the locally-installed `ODBC Driver 18 for SQL Server` reference driver during this review surfaced additional complexity in the plain (non-explicit) bare-bind case itself that wasn't fully reconciled with the existing pinned test expectations, so this needs a carefully written e2e test using the existing `BindNumericRaw`/`SQLSetDescFieldW` helper patterns already in the suite, not an ad hoc script, to reach a confident answer.

Tracked separately from PR #521 so it doesn't block merge; the PR's existing pinned tests (`NumericRebindDoesNotInheritAPreviousBindsStaleApdScale`, `NumericStructWithoutDescriptorFieldWritesUsesDefaultApdScale`) remain valid for the cases they cover.

Contributor guide

Open the contributing guide

Research direction

Start with mssql-odbc/tests/e2e/tests/param_conversions_test.cpp, the tests/e2e/README.md comparison instructions, and the existing BindNumericRaw and SQLSetDescFieldW helper patterns. Reproduce the explicit-bind then same-type bare rebind against retail msodbcsql. Done means the test compares the result and establishes whether precision_scale_explicit and the decimal_from_numeric fallback need different handling.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust, sql
Domain
databases, testing-qa
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
48/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.