mirror of
https://github.com/windmill-labs/windmill.git
synced 2026-10-18 08:03:11 +00:00
fix: address review findings on the duckdb bump
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) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 5
parent
f7b634b2ee
commit
5cad843e4e
@@ -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
|
||||
|
||||
@@ -1 +1 @@
|
||||
65949a067fb43a556dc93bd648e808cc5cbfa933
|
||||
ec55b3c383631f7a0eefa7266a5c474b518b6aea
|
||||
@@ -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<rust_decimal::Decimal>` 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"
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user