ClickHouse / ClickHouse/ClickHouse
singleValueOrNullState / singleValueOrNullMerge lose semantic state during merge and serialization
- Dominant language
- C++
- Stars
- 49.9k
- Forks
- 9k
- Avg merge
- 21h 32m
- Merged PRs (30d)
- 515
Description
**Describe the unexpected behaviour**
`singleValueOrNull` tracks two semantic flags in addition to the stored value: `first_value` and `is_null`.
In the current implementation:
- merge does not propagate `rhs.is_null`
- write/read serialize only the underlying value payload
That means aggregate states can change meaning in two ways:
1. a partial state that already means `result must be NULL` can merge into an empty destination and turn back into a concrete value
2. a valid single-value state can lose `first_value = false` after serialization/deserialization and then merge back as `NULL`
Current code on `master`:
- merge path and state fields:
- https://github.com/ClickHouse/ClickHouse/blob/fac6aa2045d1c81e28650fdd39dcbdb477f25d28/src/AggregateFunctions/AggregateFunctionSingleValueOrNull.cpp#L26-L77
- explicit TODO about serialization losing `first_value` / `is_null`:
- https://github.com/ClickHouse/ClickHouse/blob/fac6aa2045d1c81e28650fdd39dcbdb477f25d28/src/AggregateFunctions/AggregateFunctionSingleValueOrNull.cpp#L79-L83
**Which ClickHouse versions are affected?**
Confirmed in current `master` by source review at commit `fac6aa2045d1c81e28650fdd39dcbdb477f25d28`.
This appears to be longstanding. The same pattern already existed in the original implementation added on June 4, 2021:
- https://github.com/ClickHouse/ClickHouse/blob/289c5d3ad680c6e07ef189eed9ea30417acdac65/src/AggregateFunctions/AggregateFunctionMinMaxAny.h#L633-L668
- https://github.com/ClickHouse/ClickHouse/blob/289c5d3ad680c6e07ef189eed9ea30417acdac65/src/AggregateFunctions/AggregateFunctionMinMaxAny.h#L789-L801
So this likely affects all released versions since `singleValueOrNull` was introduced.
**How to reproduce**
I have not executed the SQL below locally yet; these are the minimal validation queries derived from the current implementation.
1. Merge-state loss:
```sql
SELECT singleValueOrNullMerge(s)
FROM
(
SELECT singleValueOrNullState(number) AS s
FROM numbers(2)
);
```
The inner state has already observed two distinct values, so the final result should be `NULL`.
From the current merge code, it looks like the empty destination copies only the stored value from `s` and ignores `s.is_null`, so this may return the first stored value instead.
2. Serialized-state loss:
```sql
DROP TABLE IF EXISTS t_single_value_or_null_state;
CREATE TABLE t_single_value_or_null_state
(
id UInt8,
s AggregateFunction(singleValueOrNull, UInt64)
)
ENGINE = MergeTree
ORDER BY id;
INSERT INTO t_single_value_or_null_state
SELECT 1, singleValueOrNullState(toUInt64(42));
SELECT singleValueOrNullMerge(s)
FROM t_single_value_or_null_state
GROUP BY id
ORDER BY id;
DROP TABLE t_single_value_or_null_state;
```
This should return `42`.
From the current `write/read` implementation, only `data()` is persisted; `first_value` and `is_null` are dropped. After deserialization, the state appears non-empty but still has `first_value = true`, which should make the final merge return `NULL`.
**Expected behavior**
- A partial state that has already determined `multiple distinct values => NULL` should stay NULL when merged again.
- A serialized/deserialized state should preserve `first_value` and `is_null`.
- `singleValueOrNullState` followed by `singleValueOrNullMerge` should be semantically stable.
**Additional context**
This looks separate from the recent JSON-specific crash report / fix:
- issue: https://github.com/ClickHouse/ClickHouse/issues/103630
- PR: https://github.com/ClickHouse/ClickHouse/pull/105535
That pair is about passing `result_type` instead of `value_type` during deserialization for JSON. The issue here is broader semantic state loss in merge and serialization logic itself.
The 2024 rewrite kept the same behavior:
- `49d089d` Reimplement singleValueOrNull
- `ff98e763` AggregateFunctionSingleValueOrNull: Prefer composition to inheritance
Contributor guide
Assessment
This issue has not been assessed yet.