Pre-PR validation gate

a) Deterministic repro? ✅ — clickhouse-local -q "SELECT timeSeriesResampleToGridWithStaleness('99999999999999999999', '99999999999999999998', '1', '1')(timestamp, value) FROM ts FORMAT Null" triggers runtime error: signed integer overflow: 999999999999999999 * 10 cannot be represented in type 'long' on every run of an address,undefined-sanitized build. After applying the fix the same query exits cleanly with Code: 36. DB::Exception: Cannot parse start parameter ... (BAD_ARGUMENTS).

b) Root cause explained? ✅ — Decimal<T>'s implicit operator overloads in base/base/Decimal.cpp:22-26 and :71-75 perform unchecked arithmetic on the underlying integer type. UBSan correctly flags the wrap as undefined behaviour even though ClickHouse relies on the wrap as well-defined semantics in many code paths (digit accumulation in readDigits, running sums in aggregate functions, scale conversion). Code paths that need overflow detection use the explicit helpers in base/arithmeticOverflow.h (common::mulOverflow, common::addOverflow, …) — the operators themselves are intentionally wrap-around. The sibling helpers addOverflow (line 28) and negateOverflow (line 76) already have the NO_SANITIZE_UNDEFINED attribute for exactly this reason.

c) Fix matches root cause? ✅ — Marking every Decimal<T> arithmetic operator with NO_SANITIZE_UNDEFINED makes the wrap-around well-defined at the C++ language level. Not a defensive symptom-mask: the wrap is the intended behaviour, not a bandaid hiding a real bug; the fix is identical in spirit to the pre-existing NO_SANITIZE_UNDEFINED on addOverflow and negateOverflow in the same file.

d) Test intent preserved? / New tests added? ✅ — No existing test was modified. A new regression test tests/queries/0_stateless/04254_decimal_parsing_ubsan_stid_0937_2022.sql exercises the readDecimalText path via timeSeriesResampleToGridWithStaleness with both a 20-digit string parameter (triggers operator*=) and a 19-digit string parameter just over Int64 max (triggers operator+= — the AST fuzzer's 9223372036854775800 + 8 shape), plus a happy-path case to guard against accidental behaviour change for in-range inputs.

e) Demonstrated in both directions? ✅ — Before the fix on build-asan (clang-21, -DSANITIZE=address,undefined): UBSan aborts with the exact diagnostic from CI. After the fix: same query returns BAD_ARGUMENTS cleanly, no UBSan output. tests/clickhouse-test --test-runs 20 04254_decimal_parsing_ubsan_stid_0937_2022 → 20/20 pass. Existing decimal (00700_decimal_arithm, _aggregates, _math) and timeseries (02538_nullable_array_tuple_timeseries, 03222_create_timeseries_table, 03254_timeseries_functions, _various_arguments, _group_array, 04106_timeseries_bucket_count_overflow) tests continue to pass.

f) Fix is general, not a narrow patch? ✅ — The UBSan report only hit Decimal<long>::operator*= and operator+= (Decimal64 / Int64) via the AST fuzzer, but the same overflow class exists in the sibling cross-type Decimal<U> overloads (lines 32-36 of Decimal.cpp) and the free-function arithmetic operators (lines 71-75). All are marked NO_SANITIZE_UNDEFINED in the same way so future code paths that exercise them (Decimal128 / Decimal256 accumulators, mixed-type arithmetic, free-function Decimal<T> + Decimal<T>) cannot regress with a different fuzzer trace.

session-id: cron:clickhouse-ci-task-worker:20260517-143000

cc @alexey-milovidov @yariks5s — could you review this when you have a moment? The fix is the same shape as the existing NO_SANITIZE_UNDEFINED on addOverflow and negateOverflow in this file, extended to the implicit operator overloads. UBSan-only impact in sanitizer builds (3 master+PR hits over 4 days, AST fuzzer); no behaviour change in release.

session-id: cron:clickhouse-ci-task-worker:20260517-143000

Workflow [PR], commit [067b5de]

Summary: ✅

• Performance Comparison: Performance dashboard

AI Review

Summary

This PR suppresses UBSan signed-overflow reports for the overflow-dependent Decimal<T> arithmetic operators while leaving division and modulo instrumented, and adds a stateless regression test for the readDecimalText path through timeSeriesResampleToGridWithStaleness. The current code addresses the earlier review thread about over-broad suppression on / and %; I did not find any remaining correctness, safety, or test-coverage findings.

Final Verdict

Status: ✅ Approve

@clickhouse-gh Good catch — fixed in 355717a.

NO_SANITIZE_UNDEFINED now stays only on the overflow-dependent operators (+=, -=, *=, free + / - / *, unary -, and the existing addOverflow / negateOverflow helpers). /=, %= and the free / are no longer suppressed, so UBSan continues to report divide-by-zero and INT_MIN / -1 inside Decimal arithmetic.

The rationale comments in Decimal.h and Decimal.cpp are updated to document the narrower contract.

Verified on build-asan (clang-21, -DSANITIZE=address,undefined):

• Regression test 04254_decimal_parsing_ubsan_stid_0937_2022.sql — the trigger for STID 0937-2022 goes through value *= 10 / value += digit in readDecimalText, both still marked NO_SANITIZE_UNDEFINED, so it continues to pass (10/10 runs with randomized settings).
• SELECT toDecimal64(7,0) / toDecimal64(3,0) and friends return the expected results.

Session id: cron:clickhouse-ci-task-worker:20260517-171500

ERROR: Failed to insert data into CI DB, exception [HTTPSConnectionPool(host='play.clickhouse.com', port=443): Max retries exceeded with 
                      url: /?database=default&query=INSERT+INTO+checks+FORMAT+JSONEachRow&date_time_input_format=best_effort&send_logs_level=warning (Caused by 
                      NewConnectionError("HTTPSConnection(host='play.clickhouse.com', port=443): Failed to establish a new connection: [Errno 61] Connection 
                      refused"))] (from: Fast test (arm_darwin))

Even on retry. I guess we can override and merge this as everything else is green?

LLVM Coverage Report

Changed lines: Changed C/C++ lines covered by tests: 12/18 (66.67%) | Lost baseline coverage: none · Uncovered code

Full report · Diff report