ClickHouse / ClickHouse/ClickHouse

`arrayReduce` with a seeded aggregate (`groupArraySample`) reports deterministic: the `HAVING` filter is pushed below aggregation and constant-folded, silently changing results

Open
#117,759 3 comments 0 reactions 1 assignee Claimed by @antaljanosbenjamin View on GitHub
comp-query-optimizer comp-regular-function minor potential bug
Dominant language
C++
Stars
49.9k
Forks
9k
Avg merge
21h 32m
Merged PRs (30d)
515

Description

### Company or project name

_No response_

### Describe what's wrong

`arrayReduce` and `arrayReduceInRanges` execute an aggregate function named by a string argument, but `FunctionArrayReduce` (`src/Functions/array/arrayReduce.cpp`) never overrides `isDeterministic` / `isDeterministicInScopeOfQuery`, so they report deterministic regardless of which aggregate they run. `groupArraySample` without an explicit seed draws from `thread_local_rng` on every evaluation (`src/AggregateFunctions/AggregateFunctionGroupArray.cpp`), so `arrayReduce('groupArraySample(1)', ...)` is a non-deterministic expression that the optimizer is told is deterministic. Two silently-wrong-result consequences at pure default settings:

1. A `HAVING` predicate built from it is pushed below the aggregation (legal only for deterministic predicates) and evaluated once per input row instead of once per group; a group survives if any of its rows draws favorably, so with enough rows per group every group survives.

2. With constant arguments the expression is constant-folded at analysis time, collapsing what should be an independent draw per row into a single draw reused for the whole query.

This is a different blind spot from the lambda-body one being fixed in #117747: here the nondeterminism is not hidden in a lambda — nothing consults the properties of the aggregate that the string names when the flags of `arrayReduce` itself are read.

### Does it reproduce on the most recent release?

Yes, reproduced on 26.9.1.1 (recent master build), new analyzer, default settings.

### How to reproduce

Push-down face — the predicate depends only on the grouping key, so push-down of a deterministic predicate would be legal, and the determinism lie is what makes it wrong:

```sql
SELECT count() FROM (SELECT number % 10 AS g FROM numbers(1000) GROUP BY g HAVING arrayReduce('groupArraySample(2)', [g, g + 1, 100])[1] < 50);
```

Ten runs at defaults return `10` every time (each group of 100 rows has at least one favorable draw). Control with `query_plan_filter_push_down = 0`: ten runs returned `5, 6, 7, 7, 7, 7, 7, 8, 8, 8` — the expected per-group binomial spread.

Constant-folding face:

```sql
SELECT arrayReduce('groupArraySample(1)', [1, 2, 3])[1] FROM numbers(5);
```

returns five identical values (one draw per query; the value varies across queries), while the unfoldable form varies per row as it should:

```sql
SELECT arrayReduce('groupArraySample(1)', materialize([1, 2, 3]))[1] FROM numbers(5);
-- e.g. 3 2 2 3 3
```

`arrayReduceInRanges` shows the same pair of faces:

```sql
SELECT arrayReduceInRanges('groupArraySample(1)', [(1, 3)], [10, 20, 30])[1][1]; -- one draw per query
SELECT arrayReduceInRanges('groupArraySample(1)', [(1, 3)], materialize([10, 20, 30]))[1][1] FROM numbers(5); -- varies per row
```

### Expected behavior

`arrayReduce` over a non-deterministic aggregate behaves like any other non-deterministic expression: the `HAVING` predicate is evaluated once per group after aggregation (the count varies around the binomial mean across runs), and constant arguments do not collapse the per-row draws into a single per-query value. When the named aggregate is deterministic — the overwhelmingly common case — folding and push-down stay as they are; `groupArraySample` with an explicit seed argument also stays deterministic.

### Error message and/or stacktrace

No error; silently wrong results.

### Additional context

The fix likely belongs in the overload resolver: it already creates the aggregate function object, so it can propagate the aggregate's determinism into the created `IFunctionBase` (a conservative alternative is a blocklist of seeded aggregates). Other consumers of the same flags read the same lie (for example the mutation determinism guard and query-result-cache eligibility); those consequences were not tested and are listed only as places to check when fixing.

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.