ClickHouse / ClickHouse/ClickHouse

intDivOrNull/moduloOrNull null map only checks b==0, misses INT_MIN/-1 FPE case

Open
#101,822 4 comments 0 reactions 1 assignee Claimed by @KevinyhZou View on GitHub
bug comp-regular-function
Dominant language
C++
Stars
49.9k
Forks
9k
Avg merge
21h 32m
Merged PRs (30d)
515

Description

_Found via ClickGap automated review. Please close or comment if this is incorrect or needs adjustment._

_Retrospective finding from a historical scan of [PR #78276](https://github.com/ClickHouse/ClickHouse/pull/78276) (merged 2025-04-22). Confirmed on current codebase — close with a note if already fixed._

### Describe what's wrong

intDivOrNull(toInt64(-9223372036854775808), toInt64(-1)) returns 0 instead of NULL. Same for moduloOrNull and positiveModuloOrNull with signed INT_MIN/-1.

**Root cause:** FunctionBinaryArithmetic.h:2637: null_map_data[i] = left_argument.column->isNullAt(i) || !right_argument.column->getBool(i) only checks divisor==0, but divisionLeadsToFPE also catches signed_min/-1. The impl returns 0 for that case but null map doesn't mark it as NULL.

**Why we believe this is a bug:** FunctionBinaryArithmetic.h:2637 builds the null map using !right_argument.column->getBool(i), which only detects b==0. But DivideIntegralOrNullImpl::apply (DivisionUtils.h:129) also returns 0 when divisionLeadsToFPE(a,b) is true for the INT_MIN/-1 case. The null map misses this, so the result is 0 (not NULL).

**Affected locations:**
- `src/Functions/FunctionBinaryArithmetic.h:2637` — null map construction for is_division_or_null only checks b==0 via getBool
- `src/Functions/DivisionUtils.h:129` — DivideIntegralOrNullImpl::apply returns 0 for divisionLeadsToFPE (includes INT_MIN/-1)
- `src/Functions/DivisionUtils.h:207` — ModuloOrNullImpl::apply returns 0 for divisionLeadsToFPE
- `src/Functions/DivisionUtils.h:268` — PositiveModuloOrNullImpl::apply returns 0 for divisionLeadsToFPE

**Impact:** Wrong results: intDivOrNull/moduloOrNull/positiveModuloOrNull return 0 instead of NULL for signed integer minimum divided by -1. The intDivOrNull documentation explicitly states it should return NULL for this case.

### Does it reproduce on most recent release?

Yes — confirmed on current `master` (commit `f86671aa80af`).

### How to reproduce

```sql
-- Test: intDivOrNull/moduloOrNull/positiveModuloOrNull should return NULL for INT_MIN/-1 (FPE case)
-- Covers: FunctionBinaryArithmetic.h:2637 null map only checks b==0, misses INT_MIN/-1

-- intDivOrNull with INT_MIN/-1 should return NULL (documented behavior)
SELECT 'intDivOrNull_Int64', intDivOrNull(toInt64(-9223372036854775808), toInt64(-1));
SELECT 'intDivOrNull_Int32', intDivOrNull(toInt32(-2147483648), toInt32(-1));
SELECT 'intDivOrNull_Int16', intDivOrNull(toInt16(-32768), toInt16(-1));
SELECT 'intDivOrNull_Int8', intDivOrNull(toInt8(-128), toInt8(-1));

-- moduloOrNull with INT_MIN/-1 should return NULL
SELECT 'moduloOrNull_Int64', moduloOrNull(toInt64(-9223372036854775808), toInt64(-1));

-- positiveModuloOrNull with INT_MIN/-1 should return NULL
SELECT 'positiveModuloOrNull_Int64', positiveModuloOrNull(toInt64(-9223372036854775808), toInt64(-1));

-- materialized (non-const) paths
SELECT 'intDivOrNull_mat', intDivOrNull(materialize(toInt64(-9223372036854775808)), materialize(toInt64(-1)));
SELECT 'moduloOrNull_mat', moduloOrNull(materialize(toInt64(-9223372036854775808)), materialize(toInt64(-1)));

-- Verify normal operations still work
SELECT 'normal_intDivOrNull', intDivOrNull(10, 3);
SELECT 'normal_moduloOrNull', moduloOrNull(10, 3);
SELECT 'normal_divideOrNull', divideOrNull(10, 2);
```

[Try it on ClickHouse Fiddle](https://fiddle.clickhouse.com/2ab36016-8def-4bbe-8471-44bdbd5a7272)

### Expected behavior

```
intDivOrNull_Int64 \N
intDivOrNull_Int32 \N
intDivOrNull_Int16 \N
intDivOrNull_Int8 \N
moduloOrNull_Int64 \N
positiveModuloOrNull_Int64 \N
intDivOrNull_mat \N
moduloOrNull_mat \N
normal_intDivOrNull 3
normal_moduloOrNull 1
normal_divideOrNull 5
```

### Error message and/or stacktrace

```
intDivOrNull_Int64 0
intDivOrNull_Int32 0
intDivOrNull_Int16 0
intDivOrNull_Int8 0
moduloOrNull_Int64 0
positiveModuloOrNull_Int64 0
intDivOrNull_mat 0
moduloOrNull_mat 0
normal_intDivOrNull 3
normal_moduloOrNull 1
normal_divideOrNull 5
```

### Additional context

**Open risks:**
- The const-const path at line 2496-2502 also doesn't set NULL for this case since it returns the raw 0 from OpImpl::process wrapped in a ColumnConst.

**Suggested fix:** The null map check should also account for the INT_MIN/-1 case. Either: (1) check both conditions in the null map (b==0 OR (a==INT_MIN AND b==-1) for signed types), or (2) compute the result first and compare with the sentinel to determine nullability, or (3) use a shared divisionLeadsToFPE check in the null map construction.

**Analysis details:** Confidence HIGH | Severity P1 | Testability: `STATELESS_SQL`

Found during automated review of [PR #78276](https://github.com/ClickHouse/ClickHouse/pull/78276).

---
_ClickGapAI · Confidence: HIGH · Severity: P1 · Finding: `h_pr78276_001`_

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.