Normalize AggregateFunction types and state representations #39420

amosbird · 2022-07-20T15:13:19Z

Changelog category (leave one):

Improvement

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):

Normalize AggregateFunction types and state representations because optimizations like #35788 will treat count(not null columns) as count(), which might confuses distributed interpreters with the following error : Conversion from AggregateFunction(count) to AggregateFunction(count, Int64) is not supported.

alexey-milovidov · 2022-07-21T02:38:12Z

src/DataTypes/DataTypeAggregateFunction.cpp

+    if (typeid(rhs) != typeid(*this))
+        return false;
+
+    /// All count(*) variants are of the same type.


This is not the best solution for this problem.

alexey-milovidov

I will send a proposal how to do it better.

alexey-milovidov · 2022-07-21T02:42:48Z

It should use #25015 and #24820, see IAggregateFunction::getStateType method.

Please also fix the following cases:

Materialized view when a user specified count(*) instead of count().
qualtile* functions. The data types for quantile(0.5), quantile(0.9) and quantiles(0.5, 0.9) should be the same.
-If and -Array combinators. For example, the data types for avg and avgIf should be the same.

amosbird · 2022-07-21T15:11:09Z

It should use #25015 and #24820, see IAggregateFunction::getStateType method.

Please also fix the following cases:

Materialized view when a user specified count(*) instead of count().

qualtile* functions. The data types for quantile(0.5), quantile(0.9) and quantiles(0.5, 0.9) should be the same.

-If and -Array combinators. For example, the data types for avg and avgIf should be the same.

I think all cases are resolved now.

amosbird · 2022-07-21T15:51:41Z

Some tests don't work anymore. Turn into draft until they are all fixed.

amosbird · 2022-07-23T02:45:09Z

AST fuzzer (UBSan, actions) — Logical error: 'Different order of columns in UNION subquery: countState(in(dummy, _subquery1435)) and '.

Not related to this PR, but triggers by this PR's test. A minimal reproducible case : SELECT * FROM (SELECT sqrt(0.5) AS a UNION ALL SELECT sqrt(dummy IN (SELECT 0))). Will try to fix it in another PR.

Integration tests (release, actions) [1/2] — fail: 1, passed: 1001, flaky: 0
test_merge_tree_azure_blob_storage/test.py::test_freeze_unfreeze
E helpers.client.QueryRuntimeException: Client failed! Return code: 233, stderr: Received exception from server (version 22.8.1):
E Code: 1001. DB::Exception: Received from 172.16.25.3:9000. DB::Exception: Azure::Storage::StorageException: 404 The specified blob does not exist.
E The specified blob does not exist.

Not related to this PR.

All other cases are green. It's ready for review.

amosbird · 2022-07-26T14:40:57Z

Will try to fix it in another PR

I cannot figure out a proper fix yet, only a working draft #39618

Added another case of aggregate state normalization: AggregateFunction(sum, Decimal(7, 2)) and AggregateFunction(sum, Decimal(9, 2))

This PR is complete but might be blocked by some existing issue due to new fuzz test. I'm not sure if it's proper to land now.

alexey-milovidov · 2022-07-30T15:39:54Z

tests/queries/0_stateless/01710_minmax_count_projection_distributed_query.sql

@@ -0,0 +1,7 @@
+-- Tags: no-s3-storage


But there is no projection, so "no-s3-storage" tag and "projection" from the name can be removed.

There is minmax_count_projection which is used. I'll remove "no-s3-storage" tag as this projection works with s3 storage.

alexey-milovidov · 2022-07-31T20:27:09Z

src/AggregateFunctions/IAggregateFunction.h

@@ -73,13 +73,19 @@ class IAggregateFunction : public std::enable_shared_from_this<IAggregateFunctio
    /// Get the data type of internal state. By default it is AggregateFunction(name(params), argument_types...).
    virtual DataTypePtr getStateType() const;

+    /// Same as the above but normalize state types so that variants with the same binary representation will use the same type.


Why do we need non-normalized State Type?
I thought, that if the states are the same, they should use the same type.

Because getStateType() is used to do correct finalization of aggregate states.

-- If we return original state type for quantiles* SELECT finalizeAggregation(quantilesTimingState(0.5)(number)) FROM numbers(10) ┌─finalizeAggregation(quantilesTimingState(0.5)(number))─┐ │ [5] │ └────────────────────────────────────────────────────────┘ -- If we return normalized state type for quantiles*, which param is always 1 SELECT finalizeAggregation(quantilesTimingState(0.5)(number)) FROM numbers(10) ┌─finalizeAggregation(quantilesTimingState(0.5)(number))─┐ │ [9] │ └────────────────────────────────────────────────────────┘

It's because of this

Other test failures if we only use normalized state types: https://s3.amazonaws.com/clickhouse-test-reports/39420/69347028c54edcedc9a43e6795c52c15ad6972ec/fast_test.html

Ok, understood. Let's keep it with two separate functions.

This reverts commit 6934702.

This reverts commit 5580209.

…ize_aggregateFunction_types_and_state_representations test for Decimal aggregateFunction normalization ClickHouse#39420

robot-clickhouse added the pr-improvement Pull request with some product improvements label Jul 20, 2022

KochetovNicolai self-assigned this Jul 20, 2022

KochetovNicolai approved these changes Jul 20, 2022

View reviewed changes

alexey-milovidov reviewed Jul 21, 2022

View reviewed changes

alexey-milovidov requested changes Jul 21, 2022

View reviewed changes

alexey-milovidov self-assigned this Jul 21, 2022

alexey-milovidov unassigned KochetovNicolai Jul 21, 2022

amosbird force-pushed the better-projection1-fix1 branch from 5304ba5 to b0cbb9f Compare July 21, 2022 15:10

amosbird requested a review from alexey-milovidov July 21, 2022 15:11

amosbird changed the title ~~Normalize AggregateFunctionCount type comparison~~ Normalize AggregateFunction types and state representations Jul 21, 2022

amosbird force-pushed the better-projection1-fix1 branch from b0cbb9f to 0e77179 Compare July 21, 2022 15:14

amosbird marked this pull request as draft July 21, 2022 15:51

amosbird force-pushed the better-projection1-fix1 branch from 9a807e2 to 1dee71a Compare July 22, 2022 14:34

amosbird marked this pull request as ready for review July 23, 2022 02:45

amosbird force-pushed the better-projection1-fix1 branch from 1dee71a to 5bf609e Compare July 26, 2022 14:40

amosbird force-pushed the better-projection1-fix1 branch 2 times, most recently from 94a0b66 to 99f8092 Compare July 28, 2022 03:36

alexey-milovidov approved these changes Jul 30, 2022

View reviewed changes

alexey-milovidov reviewed Jul 30, 2022

View reviewed changes

amosbird force-pushed the better-projection1-fix1 branch from 99f8092 to 26acb37 Compare July 30, 2022 18:50

alexey-milovidov reviewed Jul 31, 2022

View reviewed changes

amosbird force-pushed the better-projection1-fix1 branch from b4783a7 to 52fcf00 Compare August 1, 2022 06:05

amosbird added 3 commits August 1, 2022 21:04

Normalize AggregateFunctionCount type comparison

2a73ccb

Normalize everything else

f11d048

Add tests

f23b3d6

amosbird added 7 commits August 1, 2022 21:04

Format tests

2b2ee8a

More format refactor

0e746c1

Allow to format DataTypePtr

f84e5b6

Fix tests

09c99d8

Fix another case

8ab475c

Remove no-s3-storage tag from tests

1ac716b

test what will be wrong if state returns norm type

5580209

amosbird force-pushed the better-projection1-fix1 branch from 52fcf00 to 5580209 Compare August 1, 2022 13:04

amosbird added 3 commits August 2, 2022 03:08

Another test

6934702

Revert "Another test"

c8aaa32

This reverts commit 6934702.

Revert "test what will be wrong if state returns norm type"

b98e645

This reverts commit 5580209.

alexey-milovidov approved these changes Aug 4, 2022

View reviewed changes

alexey-milovidov merged commit 9e46abc into ClickHouse:master Aug 4, 2022

den-crane added a commit to den-crane/ClickHouse that referenced this pull request Aug 12, 2022

test for Decimal aggregateFunction normalization ClickHouse#39420

1bf98f4

den-crane mentioned this pull request Aug 12, 2022

test for Decimal aggregateFunction normalization #39420 #40178

Merged

amosbird mentioned this pull request Sep 9, 2023

Fix aggregate projections with normalized states #54480

Merged

den-crane mentioned this pull request Sep 18, 2024

With sum for example: sumMerge accepts sumState instead of sum #69509

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Normalize AggregateFunction types and state representations #39420

Normalize AggregateFunction types and state representations #39420

amosbird commented Jul 20, 2022 •

edited

Loading

alexey-milovidov Jul 21, 2022

alexey-milovidov left a comment

alexey-milovidov commented Jul 21, 2022

amosbird commented Jul 21, 2022

amosbird commented Jul 21, 2022

amosbird commented Jul 23, 2022 •

edited

Loading

amosbird commented Jul 26, 2022

alexey-milovidov Jul 30, 2022

amosbird Jul 30, 2022

alexey-milovidov Jul 31, 2022

amosbird Aug 2, 2022

alexey-milovidov Aug 3, 2022

Normalize AggregateFunction types and state representations #39420

Normalize AggregateFunction types and state representations #39420

Conversation

amosbird commented Jul 20, 2022 • edited Loading

Changelog category (leave one):

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):

alexey-milovidov Jul 21, 2022

Choose a reason for hiding this comment

alexey-milovidov left a comment

Choose a reason for hiding this comment

alexey-milovidov commented Jul 21, 2022

amosbird commented Jul 21, 2022

amosbird commented Jul 21, 2022

amosbird commented Jul 23, 2022 • edited Loading

amosbird commented Jul 26, 2022

alexey-milovidov Jul 30, 2022

Choose a reason for hiding this comment

amosbird Jul 30, 2022

Choose a reason for hiding this comment

alexey-milovidov Jul 31, 2022

Choose a reason for hiding this comment

amosbird Aug 2, 2022

Choose a reason for hiding this comment

alexey-milovidov Aug 3, 2022

Choose a reason for hiding this comment

amosbird commented Jul 20, 2022 •

edited

Loading

amosbird commented Jul 23, 2022 •

edited

Loading