ClickHouse / ClickHouse/ClickHouse
isFloat() in read-in-order optimization doesn't unwrap LowCardinality, causing wrong ORDER BY for LowCardinality(Float64) with NaN
- 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
Assessment
This issue has not been assessed yet.