Pre-PR Validation Gate

a) Deterministic repro? Yes — the UB site is at a fixed source line, triggered whenever countDigits is called on a Decimal{32,64,128,256} column whose underlying native integer holds the signed minimum. The regression test plants INT{32,64,128,256}_MIN into the underlying representation via reinterpret and exercises both constant-folded and materialize-d paths. Under the un-fixed binary on an UBSAN build (AMD/ARM), each reinterpret(...Decimal*(0)) call hits countDigits.cpp:138 and UBSAN flags the signed negation.

b) Root cause explained? Unary minus on the signed minimum of any signed integer type is undefined behavior: the magnitude -INT_MIN cannot be represented in the signed type's range. The decimal branch applied unary minus to the signed native integer before casting to unsigned. The unsigned cast then saw the already-overflowed operand, which is why UBSAN flagged the -value expression and not the subsequent static_cast<NativeT>. Casting the value to the unsigned type first converts the bit pattern (bit-preserving for INT_MIN), and unary minus on unsigned is well-defined modular arithmetic — producing exactly the correct magnitude.

c) Fix matches root cause? Yes — the fix is exactly the standard idiom for taking the absolute value of a signed integer without UB on the signed minimum: cast to unsigned, then negate. The integer branch a few lines below already used this pattern; the fix makes the decimal branch symmetric and clarifies the symmetry with a comment. The outer static_cast<NativeT> suppresses -Wimplicit-int-conversion-on-negation, which fires on UInt8/UInt16 due to integer promotion to int before the unary minus.

d) Test intent preserved / new tests added? Existing tests 01458_count_digits, 02560_count_digits, and 03275_count_digits_argument_evaluation continue to pass unchanged — verified locally. The new 04107_count_digits_int_min_ubsan.sql test covers every integer and decimal width at the signed minimum, including both constant and column (materialized) paths.

e) Demonstrated in both directions? On the local debug build (no UBSAN instrumentation, x86-64, two's complement), both the un-fixed and fixed binaries produce the correct numeric output for the new test — signed overflow happens to wrap to the correct bit pattern on two's-complement hardware. The actual behavioral difference is only visible under UBSAN instrumentation, where the un-fixed code triggers the diagnostic and the fixed code does not. The CI AST fuzzer (arm_asan_ubsan) and stress UBSAN jobs will validate this directly.

f) Fix is general, not a narrow patch?

• Same pattern in other code paths: Grepped src/Functions/ for -value-style signed negations near decimal handling: lcm.cpp:18 correctly branches on std::numeric_limits<T>::min() and returns unchanged (no UB); formatReadableTimeDelta.cpp:169 operates on Float64 (FP negation never overflows). No other sites exercise the same anti-pattern.
• Root cause vs. symptom: The fix addresses the source — the unary minus on signed INT_MIN — rather than guarding only against the one column width the fuzzer happened to hit. All four decimal widths (Decimal32/64/128/256) now use the same pattern, covering Int32, Int64, Int128, Int256 native minima uniformly.

Session: cron:clickhouse-ci-task-worker:20260421-034500
Tracking task: STID 1751-507c (3 hits across 3 unrelated runs in the last 30 days)

cc @rschu1ze @alexey-milovidov — could you review this? It's a small UBSAN fix for countDigits on Decimal* values: the decimal branch negated the signed native integer before casting to unsigned, which is UB on the signed minimum. The fix mirrors the existing integer-branch pattern (cast to unsigned first, then negate). Caught on master as STID 1751-507c in AST fuzzer (arm_asan_ubsan).

Workflow [PR], commit [c25d1cc]

Summary: ❌

AI Review

Summary

This PR fixes undefined behavior in countDigits for negative decimal values at signed minimum by switching to the safe cast-to-unsigned-then-negate pattern, and adds focused regression coverage for integer and decimal minimum-value cases (including non-constant column execution). The implementation matches the reported root cause, and the current diff does not show unresolved contract violations.

Final Verdict

Status: ✅ Approve

@groeneai, we fixed an intermittent failure of performance tests, please update the branch.

@alexey-milovidov Rebased onto current master 28be9d79738 — clean rebase, no conflicts. New HEAD: bb11cb5ec6b. CI starting fresh, should pick up the performance test fix you mentioned. APPROVED review preserved across force-push.

This was fixed by #105146. Let's update the branch.

LLVM Coverage Report

Changed lines: 100.00% (8/8) · Uncovered code

Full report · Diff report