ClickHouse / ClickHouse/ClickHouse

isFloat() in read-in-order optimization doesn't unwrap LowCardinality, causing wrong ORDER BY for LowCardinality(Float64) with NaN

Open
#101,838 1 comment 0 reactions 1 assignee Claimed by @scanhex12 View on GitHub
bug comp-query-optimizer
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 #80515](https://github.com/ClickHouse/ClickHouse/pull/80515) (merged 2025-05-27). Confirmed on current codebase — close with a note if already fixed._

### Describe what's wrong

SELECT * FROM table ORDER BY lc_float_col ASC NULLS FIRST returns NaN last instead of first when optimize_read_in_order=1 and column is LowCardinality(Float64)

**Root cause:** optimizeReadInOrder.cpp:516: isFloat() does not unwrap LowCardinality wrapper, unlike isNullableOrLowCardinalityNullable() used for the nullable check on the same line

**Why we believe this is a bug:** optimizeReadInOrder.cpp:516 → isFloat(*sorting_key.data_types[next_sort_key]) returns false for LowCardinality(Float64) because isFloat checks TypeIndex which is LowCardinality, not Float64 → the NaN-aware break condition is skipped → optimization incorrectly applied → wrong sort order

**Affected locations:**
- `src/Processors/QueryPlan/Optimizations/optimizeReadInOrder.cpp:516` — isFloat check for NaN-aware read-in-order break

**Impact:** Wrong ORDER BY results for LowCardinality(Float64) columns containing NaN when NULLS FIRST or NULLS LAST is specified with optimize_read_in_order enabled (default). Silent wrong results.

### Does it reproduce on most recent release?

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

### How to reproduce

```sql
-- Test: LowCardinality(Float64) with NaN and optimize_read_in_order produces wrong ordering
-- Covers: optimizeReadInOrder.cpp:516 isFloat() doesn't unwrap LowCardinality

SET optimize_read_in_order = 1;
SET max_threads = 1;
SET allow_suspicious_low_cardinality_types = 1;

DROP TABLE IF EXISTS test_lc_float_nan;
CREATE TABLE test_lc_float_nan (c0 LowCardinality(Float64)) ENGINE = MergeTree() ORDER BY c0;
INSERT INTO TABLE test_lc_float_nan VALUES (0);
INSERT INTO TABLE test_lc_float_nan VALUES (nan), (1);

SELECT '--- ASC NULLS LAST';
SELECT * FROM test_lc_float_nan ORDER BY c0 ASC NULLS LAST;
SELECT '--- ASC NULLS FIRST';
SELECT * FROM test_lc_float_nan ORDER BY c0 ASC NULLS FIRST;
SELECT '--- DESC NULLS LAST';
SELECT * FROM test_lc_float_nan ORDER BY c0 DESC NULLS LAST;
SELECT '--- DESC NULLS FIRST';
SELECT * FROM test_lc_float_nan ORDER BY c0 DESC NULLS FIRST;

DROP TABLE test_lc_float_nan;
```

[Try it on ClickHouse Fiddle](https://fiddle.clickhouse.com/ea7fdc8e-255e-421a-aeb1-e75e8cfd779c)

### Expected behavior

```
--- ASC NULLS LAST
0
1
nan
--- ASC NULLS FIRST
nan
0
1
--- DESC NULLS LAST
1
0
nan
--- DESC NULLS FIRST
nan
1
0
```

### Error message and/or stacktrace

```
--- ASC NULLS LAST
0
1
nan
--- ASC NULLS FIRST
0
1
nan
--- DESC NULLS LAST
0
nan
1
--- DESC NULLS FIRST
nan
1
0
```

### Additional context

**Open risks:**
- LowCardinality(Float32) and LowCardinality(BFloat16) likely have the same issue

**Suggested fix:** Use isFloat(removeLowCardinality(*sorting_key.data_types[next_sort_key])) or create an isFloatOrLowCardinalityFloat helper similar to isNullableOrLowCardinalityNullable

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

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

---
_ClickGapAI · Confidence: HIGH · Severity: P0 · Finding: `h_pr80515_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.