Fix intDiv and intDivOrNull on the smallest positive float

[groeneai]

Changelog category (leave one):

• Bug Fix (user-visible misbehavior in an official stable release)

Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):

Fixed intDiv raising ILLEGAL_DIVISION ("Division of minimal signed number by minus one") and intDivOrNull returning NULL when the dividend is the smallest positive normal Float32 or Float64 value (or the corresponding BFloat16 value, 2^-125) and the divisor is -1. The correct result, 0, is now returned.

Description

std::numeric_limits<Float>::min is the smallest positive normal value, not the domain minimum, and is_signed_v is true for Float32, Float64 and BFloat16. The INT_MIN / -1 overflow guard in throwIfDivisionLeadsToFPE and its non-throwing twin divisionLeadsToFPE compared a floating dividend against min as if it were lowest, so a tiny positive float over -1 tripped a check meant for integer overflow.

Two user-visible symptoms on master:

• intDiv(toFloat64(2.2250738585072014e-308), toFloat64(-1)) throws ILLEGAL_DIVISION; the correct result is 0. Same for Float32 at 1.17549435e-38, and for BFloat16 at its numeric_limits::min of 2^-125.
• intDivOrNull on the same operands returns NULL instead of 0 - a silent wrong result, reached through the predicate twin rather than the thrower.

intDivOrZero already returned 0, but only because it short-circuits to 0, the real quotient here.

The fix adds is_integer<A> to both guards, so they apply only to a signed integer dividend; dividing a float by -1 cannot overflow, since IEEE negation is exact and the quotient truncates to 0. B is deliberately not restricted: an integer dividend at its minimum with a floating -1 divisor is suppressed by intDivOrZero / intDivOrNull today, and only the is_signed_v<B> clause keeps that working. moduloLeadsToFPE documents this trap (#101976), but that change covered the modulo consumers only.

The new test fails on unpatched master and passes with the fix, in every constness shape, under Nullable / LowCardinality / arrayMap, and across float widths. Integer INT_MIN / -1, division by zero and out-of-range floats still throw; the modulo family is unchanged. A local matrix also covers compile_expressions = 1; DivideIntegralImpl is not compilable, so the guard has no JIT copy. Three mutations confirm each changed line is load-bearing; 50/50 runs are green with and without randomization.

No setting default changes, and the added clause is constexpr, so integer code generation is unchanged.

[groeneai]

Internal second-model review: 2 rounds, 2 findings, both addressed (click to expand)

Before opening this PR I ran an independent cold review of the resulting code plus two rounds of a
second-model review pass. Both findings were accepted; neither required a change to the shipped
diff, because both were inaccuracies in this description rather than in the code.

Findings

⚠️ numeric_limits<Float>::min is the smallest positive normal value, not the smallest positive
value, and for BFloat16 it is not even that type's smallest normal. Verified by recomputing the
IEEE encodings: Float64 has the strictly smaller subnormal 4.9e-324, which this test itself
exercises one block later; BFloat16 is 1+8+7 bits, so its smallest positive normal is bit pattern
0x0080 (2^-126) while std::numeric_limits<BFloat16>::min() is 0x0100 (2^-125, BFloat16.h:343).
The changelog entry and description now say "smallest positive normal" for the standard floats and
name the BFloat16 case as its numeric_limits::min value.

The same review asked to rename the test's section labels, the source comment and the filename. I did
not do that, for consistency reasons: the phrase "the smallest positive value" at
DivisionUtils.h:83 is pre-existing text from the merged #101976, whose own title is "Fix
moduloOrNull / positiveModuloOrNull marking smallest positive float as NULL" and whose shipped
test 04402_modulo_or_null_float_no_fpe.sql uses the same wording. Renaming only this half would
leave the two sibling guards describing one trap in two vocabularies. The precise statement is
already present where a reader looks for the mechanism: the test's own header comment says
"smallest positive normal value". Aligning the terminology across both guards, both tests and both
titles is a separate editorial change.

⚠️ The description credited the test with JIT coverage it does not have. It claimed the new test
passes "under ... compile_expressions = 1", but 04653 contains no such row; that arm exists only
in a local A/B matrix (measured rc=153 before the fix, 0 after). Reworded to attribute it to the
local matrix, and to note why the gap is harmless: DivideIntegralImpl has compilable = false, so
the JIT path cannot hold a second copy of the guard.

Verification performed independently of the fix author

💡 The carrier set was re-enumerated from scratch before reading the implementation notes, and
matched. 13 call sites reach the two changed functions. The least obvious is moduloOrZero.cpp:29,
the only caller of divisionLeadsToFPE outside this header: it is unaffected because that call sits
in the else of if constexpr (is_floating_point<ResultType>) and ResultOfModulo is Float64
whenever either operand is floating, so the else is reachable only for two integer operands.
ModuloImpl and GCDLCMImpl are cleared by a different mechanism, since their arguments pass through
NumberTraits::ToInteger<>, which maps a float to Int64.

💡 The behaviour delta was measured rather than argued: over the full native scalar type matrix the
old and new gates differ for exactly the pairs where the dividend is floating. No integer keeps or
loses a check. Int128 / Int256 retain the guard, and their numeric_limits::min() genuinely
equals lowest().

💡 The load-bearing premise was measured too: min / -1 is exactly -min for both float and
double, and even lowest / -1 stays finite. The genuinely out-of-range float cases still throw,
but through the separate input-bounds check that this PR does not touch, which is why those
serverError assertions still hold after narrowing the guard.

💡 Test liveness was traced per block rather than assumed. The float rows fail on unpatched master;
the seven "integer dividend at its minimum with a float divisor" rows are live specifically against
the tempting-but-wrong variant of this fix that also restricts the divisor type, which would turn
those 0 / NULL answers into exceptions; the reference file maps 1:1 to the queries (81 statements
minus 11 expected errors equals its 70 lines). The two BFloat16 literals were re-encoded to confirm
they round to the intended bit patterns, and the test asserts those patterns directly, so those rows
cannot silently go vacuous.

Round 2 of the second-model pass returned no findings against the final text.

[groeneai]

Pre-PR validation gate (click to expand)

#	Question	Answer
a	Deterministic repro?	Yes, single statement, no randomization needed: SELECT intDiv(toFloat64(2.2250738585072014e-308), toFloat64(-1)) throws Code: 153 ILLEGAL_DIVISION on unpatched master; intDivOrNull on the same operands returns NULL. Measured on the pristine build of the exact base commit.
b	Root cause explained?	std::numeric_limits<Float>::min is the smallest positive normal value, not the domain minimum, and is_signed_v is true for Float32 / Float64 / BFloat16. So the INT_MIN / -1 guard a == std::numeric_limits<A>::min() && b == -1 in throwIfDivisionLeadsToFPE (DivisionUtils.h:26) and in its non-throwing twin divisionLeadsToFPE (:44) matches an ordinary tiny positive float. The thrower reaches a floating A through checkedDivision from DivideIntegralImpl::apply's else branch; the predicate reaches it through integerDivisionLeadsToFPE from intDivOrZero, intDivOrNull and the divisionOrNullLeadsToNull null map.
c	Fix matches root cause?	Yes, the mis-typed predicate is corrected where it is written: is_integer<A> is added to both guards, so they apply only to a signed integer dividend. Not a call-site guard and not an error-message change; every consumer is repaired at once.
d	Test intent preserved / new tests added?	New test 04653_intdiv_smallest_positive_float_no_fpe, modelled on 04402_modulo_or_null_float_no_fpe from #101976. No existing test was modified or weakened; no no-random-* tag and no no-parallel tag (nothing global is touched, no table is created). Includes must-keep-throwing and must-not-regress groups so a "never throw" or "always throw" change is caught in both directions.
e	Both directions demonstrated?	Yes. Same 66-case matrix on the pristine base binary and the patched binary: 26 rows move, all intended (16 intDiv throws to 0, 6 intDivOrNull NULL to 0, 2 BFloat16 throws to 0, 2 toTypeName rows now evaluable); the 40 control rows are byte-identical. The test file itself reddens on the pristine binary and passes patched.
f	Fix is general across code paths?	All 13 call sites of the two changed functions were enumerated. Three reach the guard with a floating dividend and are fixed; the rest are unaffected with a reason: moduloLeadsToFPE selects the b == 0 arm for a floating result type, ModuloImpl and GCDLCMImpl pre-convert through NumberTraits::ToInteger (and gcd / lcm reject floats at type resolution), both *ByConstantImpl::vectorConstant have integer-only specialisations, DecimalBinaryOperation instantiates on the native integer type, and the JIT path sets compilable = false so there is no second copy of the guard. Fixing only the reported thrower would have left the silent intDivOrNull wrong result live, which mutation M2 demonstrates.
g	Fix generalizes across inputs?	Verified for Float32, Float64 and BFloat16; all four constness shapes; Nullable, LowCardinality, Nullable(LowCardinality(...)), inside arrayMap; compile_expressions = 1; mixed float widths in either role; and integer divisor spellings -1, Int8, Int64, Int128. Boundaries checked and unchanged: denorm_min, the next representable normal, negated min, 0.0, -0.0, inf, NaN, lowest, divisor 0 / 1 / -2, and Decimal32. The is_signed_v specialisation set is closed: its only non-is_integer members are the three float types, all covered.
h	Backward compatible?	Yes. An error becomes a correct value and a wrong NULL becomes a correct value; nothing that previously succeeded behaves differently. No setting default changes, so no SettingsChangesHistory.cpp entry applies, and no serialization or protocol format is touched. No gate is appropriate, since a spurious error is not a behaviour worth preserving.
i	Invariants and contracts preserved?	The invariant is now stated correctly: the INT_MIN / -1 overflow guard applies only when the dividend is a signed integer at its domain minimum. The two twins stay symmetric, which is the contract they are built on (throwIfDivisionLeadsToFPE throws exactly where divisionLeadsToFPE returns true), and both were changed together. B is deliberately not restricted: mutation M3 shows that adding is_integer<B> regresses six measured cases where a minimal signed integer dividend with a floating -1 divisor is suppressed today. The separate division-by-zero check and the float range and result-bounds checks are untouched and still fire, verified in the test.

Session id: cron:clickhouse-impl-slot-46:20260729-063900

[groeneai]

cc @Avogar @Algunenano could you review this? throwIfDivisionLeadsToFPE and its non-throwing twin divisionLeadsToFPE tested a floating dividend against std::numeric_limits<A>::min() as if it were the domain minimum, so intDiv threw ILLEGAL_DIVISION and intDivOrNull returned NULL for the smallest positive normal float over -1, where the answer is 0. The fix adds is_integer<A> to both guards; the divisor type is deliberately left alone, because an integer dividend at its minimum with a floating -1 divisor relies on the existing is_signed_v<B> clause.

[clickhouse-gh]

Workflow [PR], commit [3c17be0]

Summary: ❌

job_name	test_name	status	info	comment
Finish Workflow		FAIL
	python3 ./ci/jobs/scripts/workflow_hooks/new_tests_check.py	FAIL
Config Workflow		ERROR
Dockers Build (amd)		DROPPED
Dockers Build (arm)		DROPPED
Dockers Build (multiplatform manifest)		DROPPED
Style check		DROPPED
Code Review		DROPPED
Docs check		DROPPED
Docs check (Mintlify)		DROPPED
Fast test		DROPPED

[clickhouse-gh]

LLVM Coverage Report

Metric	Baseline	Current	Δ
Lines	86.40%	86.30%	-0.10%
Functions	91.90%	91.90%	+0.00%
Branches	78.50%	78.40%	-0.10%

Changed lines: Changed C/C++ lines covered: 7/7 (100.00%) · Uncovered code

Full report · Diff report

[groeneai]

CI finish ledger - 2e20e97

Every failure below has an owner: a fixing PR (mine or external), or a full-effort fix task whose fixing-PR link will be posted here when it opens. Only CH Inc sync is exempt.

Check / test	Reason	Owner / fixing PR
Stateless tests (amd_llvm_coverage, ParallelReplicas, s3 storage, parallel) / 01666_merge_tree_max_query_limit	trunk regression, not caused by this diff (fires on master and on many unrelated PRs in the same window); this PR changes only the intDiv FPE guard and its test	#112385 (mine, merged 2026-07-29 15:37Z); this build predates it, so a rerun after the next master merge picks it up
Sync	-	CH Inc sync (private, not actionable)

This is the only failing check at this head: 174 check-runs, 173 non-failing, 0 queued or in progress, Finish Workflow and Config Workflow both green.

Session id: cron:our-pr-ci-monitor:20260729-180000