Fix JIT-compiled min and max on DateTime64 comparing timestamps as unsigned

Project: ClickHouse/ClickHouse | Language: C++ | Concepts: type-systems, testing-strategy | Difficulty: mid
Link: https://github.com/ClickHouse/ClickHouse/pull/114168

The problem

When using JIT-compiled min and max aggregate functions on DateTime64 columns in ClickHouse, the results were incorrect if the data contained timestamps before 1970 (i.e., negative values). Instead of treating these as negative numbers, the JIT-compiled code compared them as unsigned integers. This caused timestamps before 1970 to be considered extremely large, so they would incorrectly win max and lose min.

Why it's non-trivial

A naive fix might just update the trait for the base decimal types, but DateTime64 and Time64 are not type aliases—they are derived classes of Decimal64. In C++, explicit template specializations for a base class do not automatically apply to derived classes. Therefore, simply specializing is_signed for Decimal64 does not fix the issue for its derived types. The fix must account for all types that are considered "decimals," including future ones, without missing any or introducing inconsistencies.

The approach

1. Replace explicit specializations with a constrained template specialization:

Previously, the code listed each decimal type explicitly:

-template <> struct is_signed<DB::Decimal32> { static constexpr bool value = true; };
-template <> struct is_signed<DB::Decimal64> { static constexpr bool value = true; };
-template <> struct is_signed<DB::Decimal128> { static constexpr bool value = true; };
-template <> struct is_signed<DB::Decimal256> { static constexpr bool value = true; };

This approach failed for derived types like DateTime64 and Time64.

The fix replaces the list with a single specialization constrained by the DB::is_decimal concept:

+/// Constrained, not a list: `DateTime64`/`Time64` derive from `Decimal64`, so a specialisation of it would not cover them.
+template <DB::is_decimal T> struct is_signed<T> { static constexpr bool value = true; };

What this does:
This makes is_signed<T> true for any type that satisfies the is_decimal concept, including all current and future decimal types, and crucially, their derived types like DateTime64 and Time64.

2. Add a targeted stateless test covering all relevant cases:

A new SQL test ensures that the bug is caught if it regresses:

-- The JIT-compiled min/max must compare DateTime64 tick counts as signed, so timestamps before
-- 1970 (negative ticks) do not win max. Both settings are randomized in CI, so pin them here.
SET compile_aggregate_expressions = 1;
SET min_count_to_compile_aggregate_expression = 0;

-- Test cases for DateTime64 with different scales, Nullable, -If combinators, and merge path
SELECT 'scale 0', min(dt0), max(dt0) FROM t_jit_dt64 GROUP BY k ORDER BY k;
SELECT 'scale 3', min(dt3), max(dt3) FROM t_jit_dt64 GROUP BY k ORDER BY k;
SELECT 'scale 9', min(dt9), max(dt9) FROM t_jit_dt64 GROUP BY k ORDER BY k;
...
SELECT 'merge', countIf(mn > mx), min(mn) = epoch - toIntervalSecond(5000), max(mx) = epoch + toIntervalSecond(4999) FROM (...);
...
SELECT 'time64', min(tm), max(tm) FROM t_jit_time64 GROUP BY k ORDER BY k;

What this does:
This test covers various scales of DateTime64, nullable columns, conditional aggregates, and the state-merge path. It also includes a guard for Time64. The test fails if the comparison is unsigned, confirming the fix works and will catch regressions.

From the review thread

⚠️ test-liveness (major, raised by the gate): result-only assertions do not prove the compiled path ran, so a silent JIT fallback would pass. Not adopted.

The gate is right that the output is byte-identical with compile_aggregate_expressions = 0, which is exactly why the setting is not the oracle and the mutation arm is. Reverting only the trait line moved the server buildId() and turned 04851_... [ FAIL ] while the prior-art 04070_decimal_min_max_jit_signedness stayed [ OK ]; restoring it restored both. A test that reddens on that revert has necessarily executed the trait-dependent code, because the trait has no other consumer this test could observe: all 74 is_signed_v uses in base/ + src/ are structurally unreachable for DateTime64/Time64 (is_integer<DateTime64> = 0, is_arithmetic_v<DateTime64> = 0, make_unsigned_t<DateTime64> ill-formed, each measured with a compile probe), leaving SingleValueData.cpp:954/:1012 as the only observable ones. So a compilation oracle would add no discriminating power here.

Also verified independently while reviewing: the derived-decimal set is closed at exactly DateTime64 and Time64 repo-wide; the constrained specialisation cannot conflict with the Int128/Int256/BFloat16 ones (none satisfy DB::is_decimal); is_unsigned_v stays false for both types so the trait pair remains consistent; and the reference file cannot encode the bug (it was generated with JIT off on an unpatched binary and matches the signed answer derived by hand from the tick counts, while the reported wrong output equals the unsigned answer).

Takeaways

Template specializations for base classes do not apply to derived classes in C++. When using traits or type properties, always account for inheritance or use concepts/constraints that match all relevant types.
Prefer concept-constrained specializations over explicit type lists. This ensures future types or derived types are automatically included, reducing maintenance and preventing subtle bugs.
Tests for low-level trait changes should target observable behavior, not just configuration flags. A test that fails when the trait is reverted and passes otherwise proves the fix is effective and guards against silent regressions.