Fuse multi quantile funcs with same arguments to one function. by hexiaoting · Pull Request #26657 · ClickHouse/ClickHouse

hexiaoting · 2021-07-21T06:49:27Z

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

Changelog category (leave one):

Performance Improvement

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

If query has multiple quantile aggregate functions with the same arguments but different level parameter:

SELECT quantile(0.5)(x), quantile(0.9)(x), quantile(0.95)(x) FROM ...
Rewrite it to

SELECT quantiles(0.5, 0.9, 0.95)(x)[1], quantiles(0.5, 0.9, 0.95)(x)[2], quantiles(0.5, 0.9, 0.95)(x)[3] FROM ...
So the aggregation will share the state.

The functions that can be fused in this implementation:
quantile, quantileDeterministic, quantileTiming, quantileTDigest, quantileTDigestWeighted, quantileExact, quantileExactWeighted, quantileBFloat16

close: #25961

vdimir

Thank you for your changes!

Some comments, main points are:

it's good to add some tests with aliased columns
fuzzer repport

src/Interpreters/TreeOptimizer.cpp

src/Interpreters/GatherFunctionQuantileVisitor.h

vdimir · 2021-07-23T09:22:33Z

tests/queries/0_stateless/01956_fuse_quantile_optimization.sql

+set optimize_fuse_quantile=true;
+SELECT 'quantile:';
+EXPLAIN SYNTAX SELECT quantile(0.2)(d), quantile(0.3)(d) FROM datetime;
+SELECT quantile(0.2)(d), quantile(0.3)(d) FROM datetime;


I suppose it's good to add tests were aliases are used like SELECT b, quantile(0.5)(x) as a, quantile(0.9)(x) as y, quantile(0.95)(x) FROM (select number as x, number % 2 as b from numbers(10)) group by b;
and also where gathered function are not only in select list, but in order by, join on or other section

vdimir · 2021-07-23T09:32:23Z

tests/queries/0_stateless/01956_fuse_quantile_optimization.sql

+SELECT '---------After fuse result-----------';
+set optimize_fuse_quantile=true;
+SELECT 'quantile:';
+EXPLAIN SYNTAX SELECT quantile(0.2)(d), quantile(0.3)(d) FROM datetime;


Why do we got two AS y in the result in (quantiles(0.5, 0.9, 0.95)(x) AS y)[1] AS y?

EXPLAIN SYNTAX SELECT b, quantile(0.5)(x) AS y, quantile(0.9)(x), quantile(0.95)(x) FROM ( SELECT number AS x, number % 2 AS b FROM numbers(10) ) GROUP BY b; ┌─explain──────────────────────────────────────────┐ │ SELECT │ │ b, │ │ (quantiles(0.5, 0.9, 0.95)(x) AS y)[1] AS y, │ │ y[2], │ │ y[3] │ │ FROM │ │ ( │ │ SELECT │ │ number AS x, │ │ number % 2 AS b │ │ FROM numbers(10) │ │ ) │ │ GROUP BY b │ └──────────────────────────────────────────────────┘

vdimir · 2021-07-23T09:35:56Z

src/Interpreters/GatherFunctionQuantileVisitor.h

+    {NameQuantileBFloat16::name, NameQuantilesBFloat16::name}
+};
+
+/// Gather all the quantilexxx functions


Suggested change

/// Gather all the quantilexxx functions

/// Gather all the `quantile*` functions

vdimir · 2021-07-23T09:36:49Z

src/Interpreters/TreeOptimizer.cpp

+                for (auto * func : functions)
+                {
+                    func->name = "arrayElement";


Please look at the fuzzer failure, it finds error somewhere here

vdimir · 2021-07-23T09:40:29Z

tests/queries/0_stateless/01956_fuse_quantile_optimization.sql

+
+SELECT '---------After fuse result-----------';
+set optimize_fuse_quantile=true;
+SELECT 'quantile:';


Also it may be a bit consing that we have column name quantile(0.9)(x) in query SELECT ..., quantile(0.9)(x), ... but in result it prints arrayElement(quantiles(0.5, 0.9, 0.95)(x), 1). We can try to add alias ... AS "quantile(0.9)(x)". But I'm not sure that it's a big issue, maybe we can leave it as is, though.

src/Interpreters/TreeOptimizer.cpp

src/Interpreters/GatherFunctionQuantileVisitor.h

vdimir · 2021-07-23T09:54:12Z

src/Interpreters/GatherFunctionQuantileVisitor.h

+        auto it = fuse_quantile.find(function.name);
+        if (it != fuse_quantile.end())
+        {
+            it->second.addFuncNode(&function);
+        }
+        else
+        {
+            FuseQuantileAggregatesData funcs{};
+            funcs.addFuncNode(&function);
+            fuse_quantile[function.name] = funcs;
+        }


You can simply write fuse_quantile[function.name].addFuncNode, map entry will be initialized with default FuseQuantileAggregatesData constructor

https://godbolt.org/z/7KGdGadz8

Suggested change

auto it = fuse_quantile.find(function.name);

if (it != fuse_quantile.end())

{

it->second.addFuncNode(&function);

}

else

{

FuseQuantileAggregatesData funcs{};

funcs.addFuncNode(&function);

fuse_quantile[function.name] = funcs;

}

fuse_quantile[function.name].addFuncNode(&function);

vdimir · 2021-08-02T10:55:50Z

@hexiaoting Do you wanna continue working on this PR? Or I may try to finish it

hexiaoting · 2021-08-06T07:28:51Z

@hexiaoting Do you wanna continue working on this PR? Or I may try to finish it

sorry for the delay, I will continue

vdimir · 2021-09-13T12:02:25Z

Sorry, I missed last commit, going to review it.

Following query is not working with option enabled:

SELECT `quantile(0.95)(x)`
FROM ( SELECT quantile(0.5)(x) as a, quantile(0.9)(x) as y, quantile(0.95)(x) FROM (select number as x, number % 2 as b from numbers(10)) )

But maybe it's ok, because query looks weired and we have option to disable it.

Also I'm thinking of creating more general option, like that enables all fuse syntax optimization (we already have one) not to create lots of for each function.

hexiaoting · 2021-09-14T07:58:45Z

Sorry, I missed last commit, going to review it.

Following query is not working with option enabled:
SELECT `quantile(0.95)(x)`
FROM ( SELECT quantile(0.5)(x) as a, quantile(0.9)(x) as y, quantile(0.95)(x) FROM (select number as x, number % 2 as b from numbers(10)) )
But maybe it's ok, because query looks weired and we have option to disable it.

Also I'm thinking of creating more general option, like that enables all fuse syntax optimization (we already have one) not to create lots of for each function.

I will use only one fuse option setting.

This reverts commit bbd7799.

…unt_avg` and `optimize_fuse_quantile`

vdimir

Thank you for your changes

vdimir · 2021-09-16T13:07:16Z

Some Fuzzer with UBSan failures not related to changes, reproduces on master #29075

alexey-milovidov · 2021-10-10T22:00:48Z

@vdimir Unfortunately, a bug has been found: #29965

Fuse multi quantile funcs with same arguments to one function.

de30962

robot-clickhouse added the pr-performance Pull request with some performance improvements label Jul 21, 2021

hexiaoting added 2 commits July 23, 2021 10:57

Fix style check errors

6b73fa5

Fix style error

b060c0a

vdimir self-assigned this Jul 23, 2021

vdimir reviewed Jul 23, 2021

View reviewed changes

src/Interpreters/GatherFunctionQuantileVisitor.h Show resolved Hide resolved

vdimir reviewed Jul 23, 2021

View reviewed changes

hexiaoting added 3 commits August 25, 2021 16:24

Support Alias and rewrite some code

1fe012b

Fix bug

76a4de2

Fix compiler error

8456fdd

hexiaoting and others added 4 commits September 14, 2021 18:51

fuse multi setting options into one when Optimize

bbd7799

Revert "fuse multi setting options into one when Optimize"

a3db56f

This reverts commit bbd7799.

Use optimize_syntax_fuse_aggregate instead of `optimize_fuse_sum_co…

5a1aeeb

…unt_avg` and `optimize_fuse_quantile`

Merge remote-tracking branch 'origin/master' into fuse_quantile

a1463d5

vdimir approved these changes Sep 14, 2021

View reviewed changes

vdimir added 3 commits September 14, 2021 15:27

Rename optimize_syntax_fuse_aggregate -> optimize_syntax_fuse_functions

a3304a8

Split GatherFunctionQuantileVisitor to h/cpp, fix fuzzed bugs

cd7fb81

Do not throw LOGICAL_ERROR from fuse quantile for illegal params

661a548

vdimir force-pushed the fuse_quantile branch from 16c69ac to 661a548 Compare September 16, 2021 13:06

Avoid cycles in optimizeFuseQuantileFunctions

091ce15

vdimir force-pushed the fuse_quantile branch from a40d3e7 to 091ce15 Compare September 20, 2021 08:40

vdimir merged commit bc7c41e into ClickHouse:master Sep 21, 2021

hexiaoting deleted the fuse_quantile branch September 22, 2021 02:15

UnamedRus mentioned this pull request Jul 30, 2023

Is there any way to expose some intermediate data inside a scalar function and make it shared? #52388

Open

	/// Gather all the quantilexxx functions
	/// Gather all the `quantile*` functions

Conversation

hexiaoting commented Jul 21, 2021

Uh oh!

vdimir left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

vdimir Jul 23, 2021

Choose a reason for hiding this comment

Uh oh!

vdimir Jul 23, 2021

Choose a reason for hiding this comment

Uh oh!

hexiaoting Aug 25, 2021

Choose a reason for hiding this comment

Uh oh!

vdimir Jul 23, 2021

Choose a reason for hiding this comment

Uh oh!

vdimir Jul 23, 2021

Choose a reason for hiding this comment

Uh oh!

vdimir Jul 23, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

vdimir Jul 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vdimir commented Aug 2, 2021

Uh oh!

hexiaoting commented Aug 6, 2021

Uh oh!

vdimir commented Sep 13, 2021

Uh oh!

hexiaoting commented Sep 14, 2021

Uh oh!

vdimir left a comment

Choose a reason for hiding this comment

Uh oh!

vdimir commented Sep 16, 2021

Uh oh!

alexey-milovidov commented Oct 10, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

vdimir Jul 23, 2021 •

edited

Loading