Support expressions with window functions by novikd · Pull Request #37848 · ClickHouse/ClickHouse

novikd · 2022-06-04T02:04:59Z

Changelog category (leave one):

Improvement

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

Support expressions with window functions. Closes #19857

novikd · 2022-06-10T15:31:26Z

@Mergifyio update

mergify · 2022-06-10T15:31:53Z

update

✅ Branch has been successfully updated

src/Interpreters/GetAggregatesVisitor.cpp

vdimir · 2022-06-17T17:37:52Z

src/Interpreters/InDepthNodeVisitor.h

+template <typename Visitor, typename T = ASTPtr>
+struct InDepthNodeVisitorWithChildInfo : Visitor
+{
+    using ChildInfo = typename Visitor::ChildInfo;


Why do we need special visitor for this, can't we use regular one and store the info in the data?

I want explicitly show that:

Node processing result depends on the results of children nodes.

Children are processed completely independently. Data represents global visitor values.

For example, GetAggregatesVisitor could be implemented in mode efficient way using this approach.
Instead of traversing query AST for several times in getAggregates and getWindowFunctions functions, we can collect functions and check correctness on the fly if we know independent results for child nodes.

vdimir · 2022-06-17T17:39:07Z

src/Interpreters/ActionsVisitor.h

 public:
    using Visitor = ConstInDepthNodeVisitor<ActionsMatcher, true>;

+    enum class WindowDependancyState


Why not just bool depends_on_window ?

It's not affecting performance in any way, but IMHO makes code more readable. This variable describes the current state of the visitor, so enum represents it more straightforwardly than boolean.

vdimir · 2022-06-17T17:42:50Z

src/Interpreters/ActionsVisitor.cpp

+            data.window_function_in_subtree = true;
        return;
    }
+    else if (node.compute_after_window_functions)


So, this flags subtree_contains_window_call, compute_after_window_functions, window_function_in_subtree, window_dependancy_state are required just to change the order of visiting the nodes, are they? It looks like this PR's trickiest part, but it just does visit.

Yes, that's the most important part of this PR. Let's consider some general expression with a window function:

f(g(arg) OVER window_name)

Evaluation of this expression must be split to 3 separate parts:

Evaluate all arguments of window functions (arg in the example)

Evaluate window functions. It's important to notice, that any window function can't have another window function as an argument. (In the example it's g(arg) OVER window_name)

Evaluate expressions with window functions (in the example we will computef call value). We can do this way because we are sure that f is not:

Aggregate function (aggregation is executed before window functions, so I can't have a window function as an argument);

Window function.

In other words, here we have some kind of topological sort.

vdimir · 2022-06-17T17:45:04Z

src/Interpreters/ExpressionAnalyzer.cpp

+    ExpressionActionsChain::Step & step = chain.lastStep(columns_after_window);
+    for (const auto & expression : syntax->expressions_with_window_function)
+    {
+        getRootActionsForWindowFunctions(expression->clone(), true, step.actions());


So, generally, most of the work is done here. Have I got right that it's the part that drives the calculation of expressions for window functions?

Yes, here we create actions for expression evaluation.

vdimir · 2022-06-17T17:50:23Z

src/Parsers/ASTFunction.h


    bool is_window_function = false;

+    bool compute_after_window_functions = false;


AST Looks like not the best place for this flag, but we already have is_window_function here, so it seems now there's no easy way to get rid of it. And we need actually to determine which expressions are related to the window in WindowExpressionsCollectorChildInfo that also operates with AST, though

In my opinion, it's completely okay to cache some useful data in AST. Instead of that we usually recompute the same values several times.

The only part that should be considered is that we don't want to make AST nodes too big. In this case, it's not a problem, because I just added a simple boolean flag (in the opposite situation we could store a pointer to a struct with cached values).

novikd · 2022-06-19T22:14:19Z

@Mergifyio update

mergify · 2022-06-19T22:14:55Z

update

✅ Branch has been successfully updated

novikd · 2022-06-20T12:21:03Z

Stateless tests (release, s3 storage, actions) — fail: 2, passed: 4148, skipped: 53

Unrelated.

vpanfilov · 2022-06-27T11:29:01Z

@novikd, thanks for working on this functionality.
I tested this feature on unstable version 22.7.1.710. Simple expressions like:

SELECT sum(number) OVER() + 1 AS result FROM numbers(10);

SELECT arraySum(groupArray(number) OVER()) AS result FROM numbers(10);

SELECT sum(number) OVER() + avg(number) OVER() AS result FROM numbers(10);

now works.

But it is not working for array operations with lambda expressions:

SELECT arrayMap(x -> x + 1, groupArray(number) OVER()) AS result FROM numbers(10);

Code: 183. DB::Exception: Unexpected lambda expression: While processing arrayMap(x -> (x + 1), groupArray(number) OVER ()) AS result. (UNEXPECTED_EXPRESSION) (version 22.7.1.710 (official build))

And not working in case of nested window functions:

SELECT sum(avg(number) OVER()) OVER() AS result FROM numbers(10);

Code: 184. DB::Exception: Window function avg(number) OVER () is found inside another window function in query: While processing avg(number) OVER (). (ILLEGAL_AGGREGATION) (version 22.7.1.710 (official build))

UnamedRus · 2022-06-27T11:34:49Z

And not working in case of nested window functions:

It's possible to have nested WINDOW functions?

novikd · 2022-06-27T17:13:54Z

@vpanfilov Thank you for testing the feature. I'll take a look at higher-order functions.

And not working in case of nested window functions:

SQL standard defines nested window functions as a completely different entity. The query you wrote is not supposed to work. If you want, you can use GROUP BY to fix this query.

Nested window functions specified in the SQL standard require specific <row_marker> usage. As far as I know, it's not been supported in ClickHouse yet.

Support expressions with window functions

58d79f8

robot-ch-test-poll2 added the pr-improvement Pull request with some product improvements label Jun 4, 2022

mergify bot and others added 6 commits June 10, 2022 15:31

Merge branch 'master' into window-function-expression

9e0d16f

Fix window expression evaluation

0c6ed20

Update test

5d2f1b7

Allow several functions in window expression

eddd4ec

Rework expressions with window functions

0663f07

Add more tests with window expressions

4d3ff7e

novikd marked this pull request as ready for review June 16, 2022 13:42

novikd added 5 commits June 16, 2022 15:11

Cleanup ActionsVisitor

f9b7155

Small refactoring

376412e

Remove redundant code

8479d88

Add comment and update docs

644399d

Fix typo

5008953

vdimir self-assigned this Jun 17, 2022

Update test

c7c5b08

vdimir reviewed Jun 17, 2022

View reviewed changes

Small refactoring

8b917d1

vdimir approved these changes Jun 19, 2022

View reviewed changes

Merge branch 'master' into window-function-expression

670a638

novikd merged commit 7717791 into master Jun 20, 2022

novikd deleted the window-function-expression branch June 20, 2022 12:21

andrea-s mentioned this pull request Jun 20, 2022

Special release to include window function expressions? #38228

Closed

qoega mentioned this pull request Jun 20, 2022

Window functions are not explained in detail ClickHouse/clickhouse-docs#130

Closed

alexey-milovidov mentioned this pull request Jul 17, 2022

Allow to use window functions inside CASE statement. #38911

Closed

den-crane mentioned this pull request Sep 21, 2022

Сombination of window AVG and ROUND does not work #41607

Closed

den-crane mentioned this pull request Nov 14, 2022

aggregate funciton unkonwn identifier #43209

Closed


		bool is_window_function = false;

		bool compute_after_window_functions = false;

Conversation

novikd commented Jun 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changelog category (leave one):

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

Uh oh!

novikd commented Jun 10, 2022

Uh oh!

mergify bot commented Jun 10, 2022

✅ Branch has been successfully updated

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

novikd commented Jun 19, 2022

Uh oh!

mergify bot commented Jun 19, 2022

✅ Branch has been successfully updated

Uh oh!

novikd commented Jun 20, 2022

Uh oh!

vpanfilov commented Jun 27, 2022

Uh oh!

UnamedRus commented Jun 27, 2022

Uh oh!

novikd commented Jun 27, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

novikd commented Jun 4, 2022 •

edited

Loading