ClickHouse / ClickHouse/pg_clickhouse

Convince Aggregates to return NULL instead of NaN

Open
#42 0 comments 0 reactions 0 assignees View on GitHub
aggregates enhancement help wanted
Dominant language
C
Stars
283
Forks
21
Avg merge
2d 6h
Merged PRs (30d)
11

Description

ClickHouse aggregates don't return `NULL` when they process no rows. Instead they return values like 0 for integers, `NaN` for floats and `[]` for arrays:

```
:) select sumIf(method_byte, false) FROM system.codecs;

┌─sumIf(method_byte, false)─┐
1. │ 0 │
└───────────────────────────┘

:) select avgIf(method_byte, false) FROM system.codecs;

┌─avgIf(method_byte, false)─┐
1. │ nan │
└───────────────────────────┘

:) select groupArrayIf(name, false), from system.codecs;

┌─groupArrayIf(name, false)─┐
1. │ [] │
└───────────────────────────┘
```

This behavior conflicts with the SQL standard and Postgres, which do return `NULL` when no rows are processed.

```
try=# select sum(relpages) filter (where false) from pg_catalog.pg_class;
sum
--------
[null]

Time: 4.614 ms
try=# select avg(relpages) filter (where false) from pg_catalog.pg_class;
avg
--------
[null]

Time: 2.605 ms
try=# select array_agg(relpages) filter (where false) from pg_catalog.pg_class;
array_agg
-----------
[null]
```

This can cause problems when one executes a query against a ClickHouse foreign table and expects to get `NULL`s for no input. An example is [this HouseClick query](https://github.com/ClickHouse/HouseClick/blob/9f737e4490167b627cc244609c2d7b6a704b80b9/app/lib/analytics_queries.ts#L17-L23)

``` sql
SELECT
round(avg(price) FILTER (WHERE town='ILMINSTER' AND district='SOUTH SOMERSET' AND postcode1='TA19')) AS filter_avg,
round(avg(price)) AS avg,
EXTRACT(YEAR FROM date) AS year
FROM public.uk_price_paid
GROUP BY year
ORDER BY year ASC;
```

For the native Postgres table, the last two rows are:

```
[null] | 376468 | 2024
[null] | 365872 | 2025
```

But for the foreign tables, they're:

```
NaN | 376468 | 2024
NaN | 365872 | 2025
```

ClickHouse provides a setting, [aggregate_functions_null_for_empty](https://clickhouse.com/docs/operations/settings/settings#aggregate_functions_null_for_empty), intended to make aggregates with now inputs return `NULL`; it does so by appending `OrNull` to the functions:

```
:) SET aggregate_functions_null_for_empty = 1

Ok.

:) select sumIf(method_byte, false) FROM system.codecs;

┌─sumIf(method_byte, false)─┐
1. │ ᴺᵁᴸᴸ │
└───────────────────────────┘

:) select avgIf(method_byte, false) FROM system.codecs;

┌─avgIf(method_byte, false)─┐
1. │ ᴺᵁᴸᴸ │
└───────────────────────────┘

```

Unfortunately, this setting [breaks groupArray](https://github.com/ClickHouse/ClickHouse/issues/38738), among other aggregate functions that work with nested values:

```
:) select groupArrayIf(name, false), from system.codecs;

Received exception from server (version 25.9.2):
Code: 43. DB::Exception: Received from localhost:9000. DB::Exception: Nested type Array(String) cannot be inside Nullable type. (ILLEGAL_TYPE_OF_ARGUMENT)
```

I thought this might be acceptable, so made a couple of attempts to enable this feature. Was was to set `aggregate_functions_null_for_empty` for every query:

```patch
diff --git a/src/binary.cpp b/src/binary.cpp
index 689ce57..1b585a6 100644
--- a/src/binary.cpp
+++ b/src/binary.cpp
@@ -181,9 +181,17 @@ ch_binary_response_t * ch_binary_simple_query(
{
resp = new ch_binary_response_t();
values = new std::vector>();
-
- client->SelectCancelable(
- std::string(query), [&resp, &values, &check_cancel](const Block & block) {
+ client->Select(
+ clickhouse::Query(query).SetQuerySettings(QuerySettings{
+ /*
+ * Enable SQL compatibility by having aggregate functions
+ * return NULL instead of NaN when no values are aggregated.
+ * Unfortunately this breaks array_agg()/groupArray() but
+ * makes all other aggregates behave as expected in a Postgres
+ * context.
+ */
+ {"aggregate_functions_null_for_empty", QuerySettingsField{ "1", 1 }},
+ }).OnDataCancelable([&resp, &values, &check_cancel](const Block & block) {
if (check_cancel && check_cancel())
{
set_resp_error(resp, "query was canceled");
@@ -210,7 +218,8 @@ ch_binary_response_t * ch_binary_simple_query(

values->push_back(std::move(vec));
return true;
- });
+ })
+ );

resp->values = (void *)values;
}
diff --git a/src/http.c b/src/http.c
index 44e69d0..73bdf19 100644
--- a/src/http.c
+++ b/src/http.c
@@ -147,9 +147,17 @@ ch_http_response_t *ch_http_simple_query(ch_http_connection_t *conn, const char

assert(conn && conn->curl);

- /* construct url */
- url = malloc(conn->base_url_len + 37 + 12 /* query_id + ?query_id= */);
- sprintf(url, "%s?query_id=%s", conn->base_url, resp->query_id);
+ /*
+ * Enable SQL compatibility by having aggregate functions return NULL
+ * instead of NaN when no values are aggregated. Unfortunately this breaks
+ * array_agg()/groupArray() but makes all other aggregates behave as
+ * expected in a Postgres context.
+ */
+ const char *params = "aggregate_functions_null_for_empty=1";
+
+ /* construct url: query_id + ?query_id= + params */
+ url = malloc(conn->base_url_len + 37 + 12 + strlen(params));
+ sprintf(url, "%s?query_id=%s&%s", conn->base_url, resp->query_id, params);

/* constant */
errbuffer[0] = '\0';
```

Unfortunately, in addition to breaking `array_agg()`/`groupArray()`, it also breaks AggregateFunction and SimpleAggregateFunction columns as documented in comments on ClickHouse/ClickHouse#38738.

I also tried to manually append `OrNull`:

```patch
diff --git a/src/deparse.c b/src/deparse.c
index d6ecc24..f5eeecc 100644
--- a/src/deparse.c
+++ b/src/deparse.c
@@ -3362,6 +3362,7 @@ deparseAggref(Aggref *node, deparse_expr_cxt *context)
uint8 brcount = 1;
bool use_variadic;
int first_arg = 0;
+ char *name = get_func_name(node->aggfnoid);

/* Only basic, non-split aggregation accepted. */
Assert(node->aggsplit == AGGSPLIT_SIMPLE);
@@ -3399,6 +3400,18 @@ deparseAggref(Aggref *node, deparse_expr_cxt *context)
appendStringInfoString(buf, "If");
}

+ /*
+ * ClickHouse aggregates return NaN instead of NULL when no values were
+ * input. Ideally we'd `SET aggregate_functions_null_for_empty` to make it
+ * compatible by appending `OrNull` to every aggregate function.
+ * Unfortunately that currenltly breaks array-returning aggregage functions
+ * like groupArray()/array_agg(). So manually append `OrNull` for
+ * aggregtes other than array_agg. Unfortunately, there is currently no
+ * way to return a NULL instead of an empty array.
+ */
+ if (strcmp("array_agg", name) != 0 && strcmp("count", name) != 0)
+ appendStringInfoString(buf, "OrNull");
+
appendStringInfoChar(buf, '(');

/* Explained below. */
@@ -3447,7 +3460,7 @@ deparseAggref(Aggref *node, deparse_expr_cxt *context)
* Client::GetServerInfo() to deparse_expr_cxt so we can allow * to be
* passed through for the fixed version.
*/
- omit_star = node->aggfilter && node->aggdistinct == NIL && strcmp(buf->data, "count");
+ omit_star = node->aggfilter && node->aggdistinct == NIL && !strcmp(name, "count");
if (context->func && context->func->cf_type == CF_SIGN_COUNT)
{
Assert(fpinfo && fpinfo->ch_table_engine == CH_COLLAPSING_MERGE_TREE);
```

This allows `array_agg()` to work, but still causes compatibility problems with AggregateFunction and SimpleAggregateFunction columns.

So for now I think the thing to do is to ask clients to properly handle these default values. In some cases, like `sum()` returning `0` and `array_agg()` returning an empty array they might be preferable! But `NaN`s will require special treatment.

Contributor guide

No contributing guide indexed for this repository

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.