From 5cad843e4e5a2b785470cd3e213653d04e49396c Mon Sep 17 00:00:00 2001 From: Ruben Fiszel Date: Fri, 7 Aug 2026 08:09:05 +0000 Subject: [PATCH] fix: address review findings on the duckdb bump MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Run the FFI crate's own tests in CI: it is excluded from the workspace, so the `cargo test --all` in backend-test never reached them and the new guard against the worker-aborting DECIMAL would not have run. build_dev.sh now honors a caller-pinned CARGO_TARGET_DIR so the test build reuses that compile instead of building the bundled engine a second time. Also pin UHUGEINT rendering, and correct the rust_decimal rationale — `Decimal::new` is public without the feature, so the reason is that the feature reproduces the exact binding the crate used to derive, not that nothing else can. Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/backend-test.yml | 5 ++++- backend/ee-repo-ref.txt | 2 +- backend/windmill-duckdb-ffi-internal/Cargo.toml | 7 +++++-- .../windmill-duckdb-ffi-internal/build_dev.sh | 7 ++++++- backend/windmill-duckdb-ffi-internal/src/lib.rs | 17 +++++++++++------ 5 files changed, 27 insertions(+), 11 deletions(-) diff --git a/.github/workflows/backend-test.yml b/.github/workflows/backend-test.yml index e6fb389e56..49bede8a4d 100644 --- a/.github/workflows/backend-test.yml +++ b/.github/workflows/backend-test.yml @@ -291,5 +291,8 @@ jobs: TEST_NPM_REGISTRY: "http://localhost:4873/:_authToken=${{ env.NPM_TOKEN }}" run: | deno --version && bun -v && node --version && go version && python3 --version && php --version && ruby --version && pwsh --version && dotnet --version - cd windmill-duckdb-ffi-internal && ./build_dev.sh && cd .. + # The FFI crate is excluded from the workspace, so the `cargo test` below + # never reaches it. Pin the target dir (matching the cache step above) so + # its own tests run off this compile rather than a second bundled build. + (cd windmill-duckdb-ffi-internal && export CARGO_TARGET_DIR="$PWD/target" && ./build_dev.sh && cargo test --release -p windmill_duckdb_ffi_internal) DENO_PATH=$(which deno) BUN_PATH=$(which bun) NODE_BIN_PATH=$(which node) GO_PATH=$(which go) UV_PATH=$(which uv) PHP_PATH=$(which php) COMPOSER_PATH=$(which composer) RUBY_PATH=$(which ruby) RUBY_BUNDLE_PATH=$(which bundle) RUBY_GEM_PATH=$(which gem) POWERSHELL_PATH=$(which pwsh) DOTNET_PATH=$(which dotnet) cargo test --features enterprise,deno_core,duckdb,license,python,rust,scoped_cache,parquet,private,private_registry_test,csharp,php,ruby,mysql,quickjs,mcp,run_inline --all -- --nocapture --test-threads=10 diff --git a/backend/ee-repo-ref.txt b/backend/ee-repo-ref.txt index 210ddc05ae..d72cf39517 100644 --- a/backend/ee-repo-ref.txt +++ b/backend/ee-repo-ref.txt @@ -1 +1 @@ -65949a067fb43a556dc93bd648e808cc5cbfa933 \ No newline at end of file +ec55b3c383631f7a0eefa7266a5c474b518b6aea \ No newline at end of file diff --git a/backend/windmill-duckdb-ffi-internal/Cargo.toml b/backend/windmill-duckdb-ffi-internal/Cargo.toml index 5f73eb8852..ba8b74fac0 100644 --- a/backend/windmill-duckdb-ffi-internal/Cargo.toml +++ b/backend/windmill-duckdb-ffi-internal/Cargo.toml @@ -5,8 +5,11 @@ edition = "2024" [dependencies] chrono = "0.4.41" -# `rust_decimal` is optional upstream, and off it there is no way to turn the -# JSON number behind a `decimal`/`numeric` job argument into a DuckDB DECIMAL. +# `rust_decimal` is optional upstream. It stays on because its +# `From` derives the same width/scale/mantissa the crate +# used to derive itself, so a `decimal`/`numeric` job argument still binds to the +# identical DECIMAL. Building one by hand would mean reimplementing that +# f64 → (mantissa, scale) inference. duckdb = { version = "1.10505.0", features = ["bundled", "rust_decimal"] } regex = "1" rust_decimal = "1.37.2" diff --git a/backend/windmill-duckdb-ffi-internal/build_dev.sh b/backend/windmill-duckdb-ffi-internal/build_dev.sh index f3ce520c68..b445cc29f3 100755 --- a/backend/windmill-duckdb-ffi-internal/build_dev.sh +++ b/backend/windmill-duckdb-ffi-internal/build_dev.sh @@ -15,7 +15,12 @@ cd "$(dirname "$0")" src_dirty="$(git status --porcelain -- src Cargo.toml Cargo.lock build.rs 2>/dev/null || true)" -if [ -n "$src_dirty" ]; then +if [ -n "${CARGO_TARGET_DIR:-}" ]; then + # A caller that pinned the output dir wins over both: CI pins it so its cache + # step finds the artifacts and a following `cargo test` reuses this compile + # instead of building the bundled DuckDB a second time. + echo "duckdb-ffi: caller-pinned target $CARGO_TARGET_DIR" +elif [ -n "$src_dirty" ]; then export CARGO_TARGET_DIR="$PWD/target" echo "duckdb-ffi: local crate changes detected -> isolated target $CARGO_TARGET_DIR" else diff --git a/backend/windmill-duckdb-ffi-internal/src/lib.rs b/backend/windmill-duckdb-ffi-internal/src/lib.rs index 49fb638d3d..2cec7a2185 100644 --- a/backend/windmill-duckdb-ffi-internal/src/lib.rs +++ b/backend/windmill-duckdb-ffi-internal/src/lib.rs @@ -825,19 +825,20 @@ mod temporal_json_tests { assert_eq!(json_of(4), serde_json::json!("10:30:00")); } - // DuckDB DECIMAL runs to 38 digits. Rendering one through a 96-bit decimal - // type aborts the whole worker rather than erroring — the panic escapes an - // `extern "C"` frame, which cannot unwind — so every width has to survive - // this conversion. + // Numbers too wide for a JSON number are rendered as strings. DECIMAL runs to + // 38 digits and UHUGEINT to 2^128-1; rendering either through a type that + // cannot hold it aborts the whole worker rather than erroring, because the + // panic escapes an `extern "C"` frame and those cannot unwind. #[test] - fn wide_decimals_render_without_losing_precision() { + fn wide_numbers_render_as_strings_without_losing_precision() { let conn = duckdb::Connection::open_in_memory().unwrap(); let mut stmt = conn .prepare( "SELECT (-1.05)::DECIMAL(4, 2) AS neg, (1.50)::DECIMAL(4, 2) AS trailing_zero, '1234567890123456789012345678.9012345678'::DECIMAL(38, 10) AS wide, - '-9999999999999999999999999999.9999999999'::DECIMAL(38, 10) AS wide_neg", + '-9999999999999999999999999999.9999999999'::DECIMAL(38, 10) AS wide_neg, + '340282366920938463463374607431768211455'::UHUGEINT AS uhuge", ) .unwrap(); let mut rows = stmt.query([]).unwrap(); @@ -856,6 +857,10 @@ mod temporal_json_tests { json_of(3), serde_json::json!("-9999999999999999999999999999.9999999999") ); + assert_eq!( + json_of(4), + serde_json::json!("340282366920938463463374607431768211455") + ); } // The data-test sample probe shape emitted by