diff --git a/Cargo.lock b/Cargo.lock index a63bc7c..cbfc5b7 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2541,8 +2541,7 @@ dependencies = [ [[package]] name = "tiberius" version = "0.12.3" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "a1446cb4198848d1562301a3340424b4f425ef79f35ef9ee034769a9dd92c10d" +source = "git+https://github.com/victorchutw/tiberius?rev=65c1a32fbc8e9c16423a76a76cdb1a800b1c21fa#65c1a32fbc8e9c16423a76a76cdb1a800b1c21fa" dependencies = [ "async-trait", "asynchronous-codec", diff --git a/Cargo.toml b/Cargo.toml index 6c20f1c..760d110 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -20,10 +20,10 @@ parquet = "58.3.0" serde = { version = "1.0", features = ["derive"] } serde_json = { version = "1.0", features = ["preserve_order"] } serde_yaml = "0.9" -# SQL Server TDS client for the sqlserver connector. Crates.io release with -# the spike-proven feature set; its stale rustls 0.21 stack is accepted with -# a weekly cargo-audit sensor carrying the ignore list (ADR-0069). -tiberius = { version = "0.12.3", default-features = false, features = ["tds73", "rustls", "chrono"] } +# SQL Server TDS client for the sqlserver connector. Pinned by full commit +# SHA to the maintainer-controlled fork: the published 0.12.3 sources plus +# one Numeric patch so `decimal(38,38)` loads (ADR-0069 fork rung, #156). +tiberius = { git = "https://github.com/victorchutw/tiberius", rev = "65c1a32fbc8e9c16423a76a76cdb1a800b1c21fa", default-features = false, features = ["tds73", "rustls", "chrono"] } uuid = { version = "1.10", features = ["v4"] } [dev-dependencies] diff --git a/docs/adr/0062-map-dataset-fields-to-exact-fit-sql-server-column-types.md b/docs/adr/0062-map-dataset-fields-to-exact-fit-sql-server-column-types.md index 7970ff0..bc1637b 100644 --- a/docs/adr/0062-map-dataset-fields-to-exact-fit-sql-server-column-types.md +++ b/docs/adr/0062-map-dataset-fields-to-exact-fit-sql-server-column-types.md @@ -8,4 +8,4 @@ Every dataset field type maps to the narrowest SQL Server column type that holds Two mappings are judgment calls rather than forced moves. `utf8` becomes `NVARCHAR(MAX)` because the dataset type is unbounded text and the mapping must stay true under chunked streaming: the writer sees one chunk at a time and can never know a global maximum length, and an append or merge table created from early data must not become invalid when later loads carry longer values. The cost — LOB storage and no plain index on the column — is schema-design territory that belongs to the table's consumers, not the loader. Instant timestamps land as UTC-normalized `DATETIME2(6)` rather than `DATETIMEOFFSET(6)` because the maintainer's production environment stores datetimes exclusively in the `DATETIME` family, so `DATETIME2` is what every downstream consumer already reads; the price, stated plainly, is that wall-clock and instant columns are indistinguishable by destination type — the offset discipline of ADR-0043 lives only in the load definition and the report. Nothing of the instant is lost beyond that marker: ADR-0043 already normalizes instants to UTC at parse without retaining the original offset, so a `DATETIMEOFFSET` column would have carried a constant `+00:00`. Precision is 6, not SQL Server's default 7, because the dataset contract is microseconds (ADR-0043): the column type states the real contract instead of implying 100-nanosecond precision that never arrives. -Because every mapping is exact, a value that satisfies the dataset schema can fail destination typing only in two enumerated slivers: `NVARCHAR` values longer than 32,767 characters — the Tiberius bulk-path client guard measured by the spike (encoded UCS-2 length ≤ 65,535 bytes), a driver-path limit rather than a server one — and timestamps whose value lies outside SQL Server's year range of 0001–9999, reachable because the strict four-digit-year parse menu admits year 0000 and instant UTC normalization can carry a boundary value across the range edge (DuckDB's ±290k-year range never sees either case). These land as write failures, never Rejected Records: a Rejected Record is a record that violates the chosen schema or load rules, and these records violate neither — they exceed one destination path's representable range. Freezing a driver cap into record-level semantics would let the same record load on DuckDB, reject on SQL Server, and load again if the write path ever changes — semantics drifting with implementation. Failure codes, retry classification, and any client-side preflight are write-semantics and error-classification decisions (#109, #112); what an existing table's columns are allowed to look like on append or merge — including production tables whose datetime columns are legacy `DATETIME` — is likewise a named #109 question, not part of this mapping. Non-finite `float64` values are absent from the sliver list because ADR-0063 removes them at parse. Alternatives rejected: `NVARCHAR(4000)` (an arbitrary bound that trades the measured 32,767 cap for a lower, server-side failure surface); length sized from observed data (unknowable under streaming, invalidates evolving append targets); pinning a collation (asserts an opinion the database already answers); `DATETIMEOFFSET(6)` for instants (a self-describing marker, but it buys two bytes per row of type distinction the report already records, against the grain of the environment this tool actually loads into); `DATETIME2(7)` (implied false precision); legacy `DATETIME` as the created type (~3.33 ms rounding against the never-round ethos of ADR-0044, and a 1753 floor below the menu's range); and tightening the parse menu to 0001–9999 (welds one destination's range into the shared contract every destination pays for). +Because every mapping is exact, a value that satisfies the dataset schema can fail destination typing only in two enumerated slivers: `NVARCHAR` values longer than 32,767 characters — the Tiberius bulk-path client guard measured by the spike (encoded UCS-2 length ≤ 65,535 bytes), a driver-path limit rather than a server one — and timestamps whose value lies outside SQL Server's year range of 0001–9999, reachable because the strict four-digit-year parse menu admits year 0000 and instant UTC normalization can carry a boundary value across the range edge (DuckDB's ±290k-year range never sees either case). These land as write failures, never Rejected Records: a Rejected Record is a record that violates the chosen schema or load rules, and these records violate neither — they exceed one destination path's representable range. Freezing a driver cap into record-level semantics would let the same record load on DuckDB, reject on SQL Server, and load again if the write path ever changes — semantics drifting with implementation. Failure codes, retry classification, and any client-side preflight are write-semantics and error-classification decisions (#109, #112); what an existing table's columns are allowed to look like on append or merge — including production tables whose datetime columns are legacy `DATETIME` — is likewise a named #109 question, not part of this mapping. Non-finite `float64` values are absent from the sliver list because ADR-0063 removes them at parse. `decimal(38,38)` is likewise not a third sliver: the released tiberius 0.12.3 panicked on scale 38 and would have described a scale-38 fraction as the invalid `numeric(39,38)`, but that was a dependency defect, not a representability limit — the patched revision ADR-0069 pins (#156) accepts scale 38 and declares such a value as `numeric(38,38)`, so the exact-fit `DECIMAL(p,s)` mapping holds across the complete declaration range. Alternatives rejected: `NVARCHAR(4000)` (an arbitrary bound that trades the measured 32,767 cap for a lower, server-side failure surface); length sized from observed data (unknowable under streaming, invalidates evolving append targets); pinning a collation (asserts an opinion the database already answers); `DATETIMEOFFSET(6)` for instants (a self-describing marker, but it buys two bytes per row of type distinction the report already records, against the grain of the environment this tool actually loads into); `DATETIME2(7)` (implied false precision); legacy `DATETIME` as the created type (~3.33 ms rounding against the never-round ethos of ADR-0044, and a 1753 floor below the menu's range); and tightening the parse menu to 0001–9999 (welds one destination's range into the shared contract every destination pays for). diff --git a/docs/adr/0069-take-tiberius-from-crates-io-and-accept-its-stale-rustls-stack.md b/docs/adr/0069-take-tiberius-from-crates-io-and-accept-its-stale-rustls-stack.md index da97d36..ab08d57 100644 --- a/docs/adr/0069-take-tiberius-from-crates-io-and-accept-its-stale-rustls-stack.md +++ b/docs/adr/0069-take-tiberius-from-crates-io-and-accept-its-stale-rustls-stack.md @@ -4,10 +4,12 @@ status: accepted # Take Tiberius from crates.io and Accept Its Stale rustls Stack -The SQL Server destination depends on `tiberius = "0.12.3"` from crates.io, with `default-features = false` and exactly the spike-proven feature set `tds73, rustls, chrono` — #115's type matrix, `DECIMAL` included, round-trips on this set, and `rustls` rather than `native-tls` keeps the single-binary promise, so there is no OpenSSL to vendor. Upstream is alive but has not released since 0.12.3 (2024-07-19; tiberius #321), which made the source a real decision — settled by the spike's build-against-both measurement: git master (`a6b4fcda`, 2026-03-06) is v0.12.3 plus four commits (a vec-reallocation perf tweak, a libgssapi bump behind the unused GSSAPI feature, two chores), byte-near-identical binaries (Δ < 300 B), identical behavior on every probe, and none of the defects this cycle designs around (#322, #410, #425, #329) fixed on either side. The requirement stays caret like every other dependency: the committed `Cargo.lock` plus the `--locked` discipline CI and release already enforce is the actual pin, and any upgrade arrives as a reviewed lockfile diff. +The SQL Server destination originally took `tiberius = "0.12.3"` from crates.io — the source decision this ADR records; the fork activation in the closing paragraph has since replaced the source, and only the source — with `default-features = false` and exactly the spike-proven feature set `tds73, rustls, chrono` — #115's type matrix, `DECIMAL` included, round-trips on this set, and `rustls` rather than `native-tls` keeps the single-binary promise, so there is no OpenSSL to vendor. Upstream is alive but has not released since 0.12.3 (2024-07-19; tiberius #321), which made the source a real decision — settled by the spike's build-against-both measurement: git master (`a6b4fcda`, 2026-03-06) is v0.12.3 plus four commits (a vec-reallocation perf tweak, a libgssapi bump behind the unused GSSAPI feature, two chores), byte-near-identical binaries (Δ < 300 B), identical behavior on every probe, and none of the defects this cycle designs around (#322, #410, #425, #329) fixed on either side. The requirement was caret like every other dependency: the committed `Cargo.lock` plus the `--locked` discipline CI and release already enforce was the actual pin, and any upgrade arrives as a reviewed lockfile diff — a discipline the later git pin keeps, now with the revision also named explicitly in the manifest. The accepted cost is the stale TLS stack tiberius pins — rustls 0.21.12 + rustls-webpki 0.101.7 + tokio-rustls 0.24.1 (update request tiberius #329 still open) — which `cargo audit` flags with three advisories (tiberius #417). Their reachability in this connector is narrow. RUSTSEC-2026-0104 (reachable panic in CRL parsing) is unreachable: tiberius configures no certificate revocation lists. RUSTSEC-2026-0098 and RUSTSEC-2026-0099 (name-constraint weakening for URI and wildcard names) run only under `encryption: required` with `trust_server_certificate: false` (ADR-0060) — the sole posture that performs webpki path validation — and then only against a CA chain asserting name constraints that a crafted server certificate abuses. The production posture (`encryption: optional`) and the CI posture (`trust_server_certificate: true`) never enter that code. Two scope notes keep the exposure honest: this feature set leaves tiberius's `reqwest`/`oauth2`/`azure_identity` optional tree out entirely, so the flagged crates arrive by exactly one path (`tiberius → tokio-rustls → rustls → rustls-webpki`); and the shipped binary carries exactly one rustls, because the modern rustls 0.23 already in the lockfile belongs to `libduckdb-sys`'s build-dependency graph and never reaches the artifact. The acceptance stays honest through a sensor and recorded exits. A scheduled weekly, non-blocking `cargo audit` scan of `Cargo.lock` — the three accepted advisories on its ignore list — opens an issue on new findings and never touches the merge gate; it lands through the cycle's slicing ticket (#113) together with the manifest change itself. The source decision reopens on any of: a new advisory reachable in the default TLS path with material severity; an upstream release (taken deliberately, re-running the spike probes); a fix this connector needs merged upstream but unreleased; upstream archival. The response ladder is crates.io first, then a pinned revision on a fork this repository's maintainer controls carrying the minimal patch, then vendoring as the last resort — a bare upstream git pin is never the answer, because it only matters once upstream has merged something, which is exactly when a fork-owned rev is the safer pin. Alternatives rejected: a pinned git revision today (the spike measured zero benefit, and it would trade away crates.io's checksum and yank protections); an `=0.12.3` requirement (restates what `--locked` already guarantees and breaks the manifest's uniform caret style); forking now to carry the rustls upgrade upstream declines to release (a standing maintenance branch for a risk the deployment postures mostly cannot reach); a blocking advisory gate in CI (an external advisory publication would redden `main` with no code change — the scheduled scan gives the same signal without breaking the always-green gate). + +Fork path activated (2026-09-02, #156). The first fix this connector needs arrived before any upstream release: tiberius 0.12.3 asserts `scale < 38` in `Numeric::new_with_scale`, so a `decimal(38,38)` value — valid under ADR-0044's `0 <= s <= p <= 38` and mapped verbatim by ADR-0062 — panics before a bulk row can be emitted, and its `precision()` counts a phantom integral digit for fraction-only values (`1 + scale`), describing a scale-38 fraction as the invalid `numeric(39,38)` in the parameter metadata tiberius emits without destination type context. Upstream `main` (`a6b4fcda`) still carries the assertion; the open hardening PR tiberius-rs/tiberius#436 lifts it but keeps the precision defect, and the optional `rust_decimal` and `bigdecimal` conversions call the same constructor, so no feature choice avoids it. Per the ladder above, the manifest now takes tiberius from the maintainer-controlled fork `victorchutw/tiberius`, pinned by the full commit SHA `65c1a32fbc8e9c16423a76a76cdb1a800b1c21fa` (it sits on branch `data-spark/numeric-scale-38`; the manifest references the revision, never the branch), with `Cargo.lock` recording the same revision; the pin trades crates.io's checksum and yank protection for git's content-addressed revision plus the `--locked` discipline, and CI fetches the public fork anonymously. The base is upstream `c34fab2` ("Bump v0.12.3"): the `v0.12.3` tag plus its own manifest version bump, whose tree is byte-identical to the published 0.12.3 sources (the tag commit itself still declares 0.12.2). The fork carries exactly one patch, confined to `src/tds/numeric.rs` plus test modules (the `column_data.rs` codec tests, `tests/query.rs`, `tests/bulk.rs`): accept `scale <= 38` while still rejecting 39, and compute the precision of a value without integral digits as `max(scale, 1)` — `0.05` is `numeric(2,2)`, zero at scale 0 stays `numeric(1,0)`, a scale-38 fraction is `numeric(38,38)` — while `integral_digits + scale` is preserved when integral digits exist. No dependency, feature, or driver change rides along, so the feature set, the rustls acceptance, and the advisory ignore list above are unchanged. Verification posture: the fork carries its own tests — unit coverage of scale-38 construction, the corrected precision and wire-length buckets, encode/decode round trips, scale-39 rejection, and the `NUMERICNTYPE` metadata bytes, plus live `SELECT @P1` and bulk-insert round trips of `DECIMAL(38,38)` boundary values and fraction-only length-bucket edges against SQL Server 2022 — run locally under the Data Spark feature set before adoption; Data Spark keeps a direct offline smoke test of the pinned revision (`tests/tiberius_dependency.rs`) and operates no separate fork CI, so every future fork revision must pass the same local and live verification before its SHA is adopted, and the fork stays a patch carrier rather than tracking upstream `main`. Upstream trace: the reproduction and the complete invariant were posted on tiberius-rs/tiberius#436 (https://github.com/tiberius-rs/tiberius/pull/436#issuecomment-5505639544); a merge or release upstream is not a condition of this activation. Exit condition: return to crates.io — the first rung — as soon as an upstream release contains both halves of the fix and passes the same Data Spark probes, retiring the fork branch. diff --git a/tests/tiberius_dependency.rs b/tests/tiberius_dependency.rs new file mode 100644 index 0000000..2f73bea --- /dev/null +++ b/tests/tiberius_dependency.rs @@ -0,0 +1,55 @@ +//! Direct smoke test of the pinned Tiberius dependency (#156, ADR-0069). +//! +//! Data Spark's declared Decimal contract admits `decimal(38,38)` +//! (ADR-0044), and the SQL Server mapping carries that declaration +//! verbatim (ADR-0062). Tiberius 0.12.3 from crates.io panics on scale 38 +//! and describes a fraction-only scale-38 value as `numeric(39,38)`, so +//! the manifest pins a maintainer-controlled fork carrying the fix. This +//! test proves the pinned revision honors the complete scale-38 invariant +//! without opening a connection; the live round trip lives in the fork's +//! own test suite. + +use tiberius::numeric::Numeric; +use tiberius::{ColumnData, ToSql}; + +/// The #134 regression seed: Arrow `Decimal128(38,38)` scaled value `-1`. +const SCALED_MINUS_ONE: i128 = -1; + +#[test] +fn pinned_tiberius_builds_the_scale_38_value_deterministically_with_precision_38() { + let first = Numeric::new_with_scale(SCALED_MINUS_ONE, 38); + let second = Numeric::new_with_scale(SCALED_MINUS_ONE, 38); + + for value in [first, second] { + assert_eq!(value.value(), SCALED_MINUS_ONE); + assert_eq!(value.scale(), 38); + assert_eq!(value.int_part(), 0); + assert_eq!(value.dec_part(), SCALED_MINUS_ONE); + assert_eq!( + value.precision(), + 38, + "a fraction-only scale-38 value is numeric(38,38), never numeric(39,38)" + ); + } + + assert_eq!(first.to_sql(), ColumnData::Numeric(Some(second))); +} + +#[test] +fn pinned_tiberius_reports_precision_38_across_the_scale_38_range() { + let max_magnitude = 10i128.pow(38) - 1; + + for value in [0, 1, max_magnitude, -max_magnitude] { + let numeric = Numeric::new_with_scale(value, 38); + assert_eq!(numeric.value(), value); + assert_eq!(numeric.scale(), 38); + assert_eq!(numeric.precision(), 38, "precision of {value} at scale 38"); + } +} + +#[test] +#[should_panic(expected = "scale <= 38")] +fn pinned_tiberius_still_rejects_scale_39() { + // Scale 39 exceeds SQL Server's cap and must keep panicking. + Numeric::new_with_scale(1, 39); +}