Skip to content

Pin Tiberius to a fork carrying the decimal(38,38) Numeric fix - #157

Merged
victorchutw merged 3 commits into
mainfrom
issue-156-tiberius-decimal-38-38
Sep 2, 2026
Merged

Pin Tiberius to a fork carrying the decimal(38,38) Numeric fix#157
victorchutw merged 3 commits into
mainfrom
issue-156-tiberius-decimal-38-38

Conversation

@victorchutw

Copy link
Copy Markdown
Owner

Closes #156

Why

Data Spark's declared Decimal contract admits decimal(38,38) (ADR-0044) and the SQL Server mapping carries it verbatim (ADR-0062), but tiberius 0.12.3 from crates.io panics on scale 38 (assertion failed: scale < 38) and, with the assertion lifted, still describes a fraction-only scale-38 value as the invalid numeric(39,38). Upstream main carries the assertion and the open hardening PR tiberius-rs/tiberius#436 fixes only half. ADR-0069's response ladder names the answer: a pinned revision on a maintainer-controlled fork carrying the minimal patch.

What

  • Fork victorchutw/tiberius, branch data-spark/numeric-scale-38, based on upstream c34fab2 ("Bump v0.12.3" — the v0.12.3 tag plus its own manifest version bump; the tree is byte-identical to the published 0.12.3 sources, whereas the tag commit itself still declares 0.12.2). One patch, src/tds/numeric.rs plus tests: accept scale <= 38 (39 still rejected) and compute the precision of a value without integral digits as max(scale, 1), so 0.05 is numeric(2,2) and a scale-38 fraction is numeric(38,38); integral_digits + scale is preserved otherwise. Commit: victorchutw/tiberius@65c1a32
  • Pin in Cargo.toml by full commit SHA (no branch tracking); Cargo.lock records the same revision and nothing else moves (the lock diff is the one tiberius entry — a hand-limited edit, accepted by --locked, because cargo update -p tiberius also flipped unrelated windows-sys/getrandom edges of other crates).
  • Smoke test tests/tiberius_dependency.rs: scaled value -1 at scale 38 builds deterministically with precision 38, the whole scale-38 magnitude range reports precision 38, and scale 39 still panics. Red against crates.io 0.12.3 (same panic as the issue), green on the pin.
  • ADR-0069 records the activated fork path: trigger, base and pinned revision, the minimal patch, verification posture, upstream trace, and the crates.io exit condition. ADR-0062 records that the patched revision preserves the decimal(38,38) exact-fit mapping and adds no third representability sliver. ADR-0044 and CONTEXT.md are unchanged.
  • No CHANGELOG entry: SQL Server has no live load mode yet, so nothing is user-observable.

Upstream trace

Factual reproduction of the precision-39 defect on PR #436's head and the complete invariant: tiberius-rs/tiberius#436 (comment). Upstream merge or release is not a condition; the ADR names the exit.

Fork verification before adoption

Feature set tds73,rustls,chrono (Data Spark's), SQL Server 2022 CU26 container, tokio and async-std runtimes, run in a rust:1-bookworm container because the fork's v0.12.3 dev-dependencies need libssl-dev:

Test binary Result
lib unit tests 128 passed (includes the new Numeric and TYPE_INFO tests)
bulk 84 passed (includes DECIMAL(38,38) and bucket-edge bulk round trips)
query 162 passed (includes SELECT @P1 scale-38 and bucket-edge round trips with CONVERT(VARCHAR) text checks)
deadlocks / login_errors 1 / 1 passed
custom-cert 2 failed with UnsupportedCertVersion on the unpatched base as well — they need upstream's custom-certificate container image
numeric/decimal tests with rust_decimal,bigdecimal 20 passed

Clippy adds no warning inside the patched line ranges (the remaining warnings are pre-existing legacy-constant and thread_local lints on untouched lines).

Local checks

cargo fmt --check, cargo clippy --locked --all-targets -- -D warnings, cargo test --locked, cargo build --release --locked --bin data-spark, and cargo audit (unchanged four accepted advisories) — see the results comment below.

After merge

Move #134 from blocked to ready-for-agent (AC7); it already carries the production-seam Decimal128(38,38) scaled -1 regression criterion.

🤖 Generated with Claude Code

tiberius 0.12.3 from crates.io asserts `scale < 38` in
`Numeric::new_with_scale` and describes a fraction-only scale-38 value as
the invalid `numeric(39,38)`, so a valid `decimal(38,38)` declaration
(ADR-0044, mapped verbatim by ADR-0062) could never reach SQL Server.
Upstream main still carries the assertion and the open hardening PR
tiberius-rs/tiberius#436 fixes only the assertion.

Activate ADR-0069's fork rung: pin the maintainer-controlled fork
victorchutw/tiberius by full commit SHA 65c1a32f (upstream "Bump v0.12.3"
plus one Numeric patch that accepts scale <= 38 and computes the
precision of a value without integral digits as max(scale, 1)). The lock
records the same revision and nothing else moves. Add a direct offline
smoke test of the pinned revision, record the activation, verification
posture, upstream trace, and crates.io exit in ADR-0069, and note in
ADR-0062 that decimal(38,38) is not a third representability sliver.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@victorchutw

Copy link
Copy Markdown
Owner Author

Local checks on 386db0a (rustc 1.96.1):

Check Result
cargo fmt --check ok
cargo clippy --locked --all-targets -- -D warnings ok
cargo test --locked ok — every binary green, including the new tiberius_dependency smoke test (3 passed; the same tests fail against crates.io 0.12.3 with assertion failed: scale < 38)
cargo build --release --locked --bin data-spark ok
cargo audit no vulnerabilities; the four accepted advisories unchanged; 3 informational warnings (the known unmaintained pair plus a registry-side chacha20 0.10.1 yanked notice under duckdb → reqwest, untouched by this change)

The Cargo.lock diff is exactly the tiberius source line (registry → git rev) minus its checksum line; cargo tree --locked -p tiberius and every --locked command above accept it.

Name the smoke test without the glossary-avoided "numeric", prove scale-39
rejection with should_panic(expected) like the rest of the repo, keep one
determinism assertion, trim the Cargo.toml comment to the sibling style,
and make ADR-0069 name the fork's test-module edits and the branch/revision
relationship precisely.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@victorchutw

Copy link
Copy Markdown
Owner Author

Two-axis review (git diff main...HEAD @ 3b18e86), both reviewers valid:

Standards — no hard violations (CONTRIBUTING #[ignore] rule, ADR-0066, local checks, ADR prose style, CONTEXT.md vocabulary, AC8). Judgement calls applied in the follow-up commit: smoke-test name no longer uses the glossary-avoided "numeric"; scale-39 rejection uses #[should_panic(expected = "scale <= 38")] like the repo's precedent instead of catch_unwind; the determinism check keeps one assertion; the Cargo.toml comment is trimmed to the sibling style with the detail left to ADR-0069; ADR-0069 now names the fork's test-module edits (column_data.rs tests, tests/query.rs, tests/bulk.rs), states the branch/revision relationship precisely, and paragraph 1 points forward to the activation. Declined: extracting the repeated [-1, 0, 1, ±(10^38-1)] fixture in the fork's tests into a helper — a test-only refactor would mean a new fork revision, re-pin, and re-verification for no behavioural gain; noted as a candidate for the eventual upstream PR.

Spec — every acceptance criterion met; two recorded deviations in letter, not substance: the fork base is upstream c34fab2 (tag v0.12.3 plus its own version bump, tree identical to crates.io 0.12.3) rather than the bare tag whose manifest still says 0.12.2, and the Cargo.lock entry was hand-limited to exactly what cargo emits for the git source because cargo update -p tiberius also flipped unrelated windows-sys/getrandom edges. AC7's label move on #134 happens after merge. No scope creep beyond the bucket-edge tests, which the precision fix necessitates and the ADR discloses.

@victorchutw
victorchutw marked this pull request as ready for review September 2, 2026 06:59
Copilot AI lite review requested due to automatic review settings September 2, 2026 06:59

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

變更核心資料庫 driver 依賴來源(改為 fork git pin)屬供應鏈與相容性風險面較高的變更,建議由 maintainer 最終人工確認後再合併。

Pull request overview

此 PR 針對 SQL Server 連線器依賴的 Tiberius 0.12.3 在 decimal(38,38)(scale 38)會 panic、且對純小數值的 precision 描述錯誤(會變成 numeric(39,38))的問題,依 ADR-0069 的「fork rung」策略,改以 maintainer 控制的 fork 之 immutable commit SHA 釘死依賴,並加入離線 smoke test 與 ADR 更新,確保 Data Spark 的 Decimal 合約(ADR-0044 / ADR-0062)可完整覆蓋到 decimal(38,38)

Changes:

  • tiberius 依賴由 crates.io 0.12.3 改為 pin 到 victorchutw/tiberius 的指定 commit SHA,並同步更新 Cargo.lock
  • 新增 tests/tiberius_dependency.rs:離線驗證 scale 38 不再 panic、precision 一律為 38、且 scale 39 仍維持 panic。
  • 更新 ADR-0069 / ADR-0062,記錄 fork activation、修補內容與不新增 representability sliver 的結論。
File summaries
File Description
tests/tiberius_dependency.rs 新增離線 smoke test,鎖定 scale-38 invariant(precision 38、scale 39 仍 panic)。
docs/adr/0069-take-tiberius-from-crates-io-and-accept-its-stale-rustls-stack.md 記錄 fork rung activation、pin 的 commit SHA、修補範圍與驗證姿勢。
docs/adr/0062-map-dataset-fields-to-exact-fit-sql-server-column-types.md 補充說明 decimal(38,38) 不是第三 sliver:問題屬於依賴缺陷且已由 pinned revision 修正。
Cargo.toml tiberius 改為 git rev pin 到 maintainer fork。
Cargo.lock tiberius source 由 crates.io 改為 git rev,避免其他依賴邊緣變動。
Review details
  • Files reviewed: 4/5 changed files
  • Comments generated: 1
  • Review effort level: Lite

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

Comment thread docs/adr/0069-take-tiberius-from-crates-io-and-accept-its-stale-rustls-stack.md Outdated
The opening paragraph described the crates.io source and caret requirement
in the present tense, contradicting the fork pin now in Cargo.toml. State
both as the original decision, point at the fork activation that replaced
only the source, and note that the lockfile discipline carries over.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@victorchutw

Copy link
Copy Markdown
Owner Author

Merge-ready at 7644df4: rust green (4m17s, run 33601680975), copilot-reviewed success, Copilot's one thread (ADR-0069 present-tense wording) fixed and resolved, zero unresolved conversations, branch up to date with main (mergeStateStatus: CLEAN). Stopping here per the GitOps merge-authorization rule. After merge: move #134 from blocked to ready-for-agent (AC7).

@victorchutw

Copy link
Copy Markdown
Owner Author

Merge authorization: the maintainer instructed the agent to merge this PR ("merge") in the active agent session on 2026-09-02; recording it here per docs/agents/gitops.md before merging. Single-use, this PR only.

@victorchutw
victorchutw merged commit 4b30017 into main Sep 2, 2026
6 checks passed
@victorchutw
victorchutw deleted the issue-156-tiberius-decimal-38-38 branch September 2, 2026 07:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Resolve Tiberius decimal(38,38) bulk-row limitation

3 participants