Skip to content

Add aggregate function combinators: -OrNull & -OrDefault#7331

Merged
akuzm merged 6 commits intoClickHouse:masterfrom
hczhcz:patch-1015
Oct 18, 2019
Merged

Add aggregate function combinators: -OrNull & -OrDefault#7331
akuzm merged 6 commits intoClickHouse:masterfrom
hczhcz:patch-1015

Conversation

@hczhcz
Copy link
Copy Markdown
Contributor

@hczhcz hczhcz commented Oct 15, 2019

I hereby agree to the terms of the CLA available at: https://yandex.ru/legal/cla/?lang=en

For changelog. Remove if this is non-significant change.

Category (leave one):

  • New Feature

Short description (up to few sentences):

Add aggregate function combinators which fill null or default value when there is nothing to aggregate.

@hczhcz
Copy link
Copy Markdown
Contributor Author

hczhcz commented Oct 15, 2019

See also #6741

@hczhcz hczhcz changed the title [WIP] Add aggregate function combinators: -OrNull & -OrDefault Add aggregate function combinators: -OrNull & -OrDefault Oct 15, 2019
@akuzm
Copy link
Copy Markdown
Contributor

akuzm commented Oct 15, 2019

Looks very useful. Please add a mention of these combinators to the docs (it would be in docs/en/query_language/agg_functions/combinators.md).

A recent related issue: #7336

akuzm
akuzm reviewed Oct 17, 2019
View reviewed changes
dbms/src/AggregateFunctions/AggregateFunctionOrFill.h Outdated
private:
AggregateFunctionPtr nested_function;

size_t sod;
Copy link
Copy Markdown
Contributor

@akuzm akuzm Oct 17, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's use a full name like size_of_data.

akuzm
akuzm reviewed Oct 17, 2019
View reviewed changes
dbms/src/AggregateFunctions/AggregateFunctionOrFill.h
@@ -0,0 +1,173 @@
#pragma once
Copy link
Copy Markdown
Contributor

@akuzm akuzm Oct 17, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#pragma once
/**
* -OrNull and -OrDefault combinators for aggregate functions.
* If there are no input values, return NULL or a default value, accordingly.
* Use a single additional byte of data after the nested function data:
* 0 means there was no input, 1 means there was some.
*/
#pragma once

akuzm
akuzm reviewed Oct 17, 2019
View reviewed changes
dbms/src/AggregateFunctions/AggregateFunctionOrFill.h Outdated

size_t sizeOfData() const override
{
return sod + sizeof(bool);
Copy link
Copy Markdown
Contributor

@akuzm akuzm Oct 17, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sizeof(bool) is implementation-defined, may be greater that 1. Maybe we should decide that our marker is char and just use 1 here?

akuzm
akuzm reviewed Oct 17, 2019
View reviewed changes
dbms/tests/queries/0_stateless/01018_empty_aggregation_filling.sql
SELECT arrayReduce('maxOrDefault', arrayPopBack(['hello']));
SELECT arrayReduce('maxOrNull', arrayPopBack(['hello']));

SELECT arrayReduce('maxOrDefault', arrayPopBack(arrayPopBack([toDateTime('2011-04-05 14:19:19'), null])));
Copy link
Copy Markdown
Contributor

@akuzm akuzm Oct 17, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

JFYI, this can be written more explicitly with cast, like SELECT arrayReduce('maxOrDefault', cast([], 'Array(Nullable(DateTime))'));
You don't have to change it, the current way is also OK.

@akuzm
Copy link
Copy Markdown
Contributor

akuzm commented Oct 17, 2019

I'm not sure about the orDefault name -- the user might think that this means the default for column, but it's actually the default for data type. Like this:

:) create table t(a int default 7) engine Memory;
:) select avgOrDefault(a) from t;
┌─avgOrDefault(a)─┐
│               0 │
└─────────────────┘

Maybe orZero? We use this suffix for parsing functions:
https://clickhouse.yandex/docs/en/query_language/functions/type_conversion_functions/#todecimal-32-64-128-orzero

@hczhcz
Copy link
Copy Markdown
Contributor Author

hczhcz commented Oct 17, 2019

@akuzm
The consideration here is that the default values of some types (e.g. strings) are not "zero".
Probably we can add some aliases?

akuzm
akuzm approved these changes Oct 18, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@akuzm akuzm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alexey-milovidov says orDefault is OK, so let's merge.

@akuzm akuzm merged commit 502672c into ClickHouse:master Oct 18, 2019
@filimonov filimonov mentioned this pull request Oct 28, 2019
Closed
@akuzm akuzm added the pr-feature Pull request with new product feature label Oct 29, 2019
@hczhcz hczhcz deleted the patch-1015 branch October 31, 2019 06:30
@BayoNet BayoNet mentioned this pull request May 4, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@akuzm akuzm akuzm approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

pr-feature Pull request with new product feature

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@hczhcz @akuzm