Implementation of window function nth_value by ryzuo · Pull Request #26334 · ClickHouse/ClickHouse

ryzuo · 2021-07-15T06:10:38Z

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

Changelog category (leave one):

New Feature

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Implement window function nth_value(expr, N) that returns the value of the Nth row of the window frame.

Detailed description / Documentation draft:
nth_value(expr, N) returns the value of expr from the N-th row of the window frame. If there is no such row, the return value is NULL.

Information about CI checks: https://clickhouse.tech/docs/en/development/continuous-integration/

ryzuo · 2021-07-15T06:25:23Z

Related issue
#26112

akuzm · 2021-07-15T10:53:56Z

src/Processors/Transforms/WindowTransform.cpp

Are you also fixing the lagInFrame nullability problems in this PR? Better make two separate ones.

This change looks incorrect, because the return type of lagInFrame doesn't have to be nullable, e.g. when a not-null default value is specified. The problems in #26115 are partly caused by that we apply nullability handling for aggregate functions to window functions as well, when we shouldn't. The fix needs some refactoring that would introduce deeper separation between aggregate and window functions. I started working on it but was distracted by other tasks. Probably will continue next week.

So in the meantime, could you please remove the fix for lagInFrame from this PR, and keep only the new function nth_value? That function looks good.

Thanks for your comments, you are right, the force of nullability here is not appropriate and incomplete I accidently pushed it, I will clean it up, and I'm still looking into this problem. Speaking of the non-null default value, I found the result is not correct if the given default value is an arithmetic expression with type cast required, e.g. if the default value for the 3rd arg is like value*1.5, it looks like over flows. I tracked all the way up to interpreter, and also with null value, my guess there might be something in parser when it deduce the type of the 3rd arg? But I haven't looked into that part yet. I'm new in CH, still trying to understand lots of things.

ryzuo · 2021-07-21T09:53:06Z

@akuzm Hi Alexander, there are still some check failures do not seem to relate to the code, Could you please help to take a look? And is there any more comments and suggestions?

akuzm · 2021-07-21T10:12:37Z

@akuzm Hi Alexander, there are still some check failures do not seem to relate to the code, Could you please help to take a look? And is there any more comments and suggestions?

clang-tidy complains about your changes:

2021-07-21 02:40:24 /build/obj-x86_64-linux-gnu/../src/Processors/Transforms/WindowTransform.cpp:1624:18: error: invalid case style for local variable 'srcColumnPtr' [readability-identifier-naming,-warnings-as-errors]
2021-07-21 02:40:24             auto srcColumnPtr = transform->blockAt(target_row).input_columns[workspace.argument_column_indices[0]];
2021-07-21 02:40:24                  ^~~~~~~~~~~~
2021-07-21 02:40:24                  src_column_ptr

Other things are probably not relevant, I'll check them before merging.

ryzuo · 2021-07-21T10:21:46Z

I see, I will update that, thanks for your comments. And I saw your PR for 26115, should I change the nth_value returning in this PR like your changes? Or I should wait you merge both and open another one?

akuzm · 2021-07-21T11:12:52Z

I see, I will update that, thanks for your comments. And I saw your PR for 26115, should I change the nth_value returning in this PR like your changes? Or I should wait you merge both and open another one?

I just merged it. It would be good if you merge with master and change the nth_value implementation in this PR as well. By the way, I'm open for suggestions/test cases regarding the lagInFrame fix -- not sure I have it all totally figured out yet...

1. Implemented the window function nth_value 2. Make the return type of lag/lead, and nth_value functions to be ColumnNullable, thus the returned value can be null if it is out of frame.

…leadInFrame

Make nth_value for nullable values for out of frame rows, as the same fashion as lagInFrame just does.

akuzm · 2021-07-23T12:42:17Z

src/Processors/Transforms/WindowTransform.cpp

+        if (argument_types.empty())
+        {
+            throw Exception(ErrorCodes::BAD_ARGUMENTS,
+                "Function {} takes at least one argument", name_);
+        }
+


If it always needs two arguments, we probably don't need this check, the next one is enough.

akuzm

LGTM other than that one comment.

akuzm · 2021-07-23T12:45:17Z

src/Processors/Transforms/WindowTransform.cpp

 #include <Interpreters/ExpressionActions.h>
 #include <Interpreters/convertFieldToType.h>

-


We normally have two empty lines here, let's keep it.

…suggested

akuzm · 2021-07-27T12:01:53Z

The tests are flaky/broken in master.

alexey-milovidov · 2021-09-25T01:28:18Z

@akuzm Unfortunately, this function contains a major security issue: #29347

alexey-milovidov · 2021-09-25T01:29:05Z

tests/queries/0_stateless/01591_window_functions.sql

 order by number
 ;

+-- nth_value without specific frame range given


New test should be created instead of adding new cases to old test.

robot-clickhouse added doc-alert pr-feature Pull request with new product feature labels Jul 15, 2021

alexey-milovidov added the can be tested label Jul 15, 2021

akuzm reviewed Jul 15, 2021

View reviewed changes

akuzm self-assigned this Jul 15, 2021

ryzuo and others added 5 commits July 22, 2021 10:19

Implementaion of window function nth_value.

4eb053a

1. Implemented the window function nth_value 2. Make the return type of lag/lead, and nth_value functions to be ColumnNullable, thus the returned value can be null if it is out of frame.

Make tests of nth_value pass

fd0c016

Add missing test back in 01591_window_function

df9ec56

Cleanup the incomplete fix of nullability return value of lagInFrame/…

94d4fb9

…leadInFrame

Update the implementaion of nth_value

4d36d54

Make nth_value for nullable values for out of frame rows, as the same fashion as lagInFrame just does.

ryzuo requested a review from akuzm July 23, 2021 09:12

akuzm reviewed Jul 23, 2021

View reviewed changes

akuzm approved these changes Jul 23, 2021

View reviewed changes

akuzm reviewed Jul 23, 2021

View reviewed changes

Remove unnecessary emtpy argument check for nth_value as code review …

0e74248

…suggested

akuzm merged commit 7e737d9 into ClickHouse:master Jul 27, 2021

akuzm mentioned this pull request Jul 27, 2021

Is there any plan to implement window function nth_value in the near future? We'd like to use it. #26112

Closed

ryzuo deleted the ryzuo branch August 17, 2021 07:45

alexey-milovidov reviewed Sep 25, 2021

View reviewed changes

		#include <Interpreters/ExpressionActions.h>
		#include <Interpreters/convertFieldToType.h>

Conversation

ryzuo commented Jul 15, 2021 • edited by akuzm Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryzuo commented Jul 15, 2021

Uh oh!

akuzm Jul 15, 2021

Choose a reason for hiding this comment

Uh oh!

ryzuo Jul 16, 2021

Choose a reason for hiding this comment

Uh oh!

ryzuo commented Jul 21, 2021

Uh oh!

akuzm commented Jul 21, 2021

Uh oh!

ryzuo commented Jul 21, 2021

Uh oh!

akuzm commented Jul 21, 2021

Uh oh!

akuzm Jul 23, 2021

Choose a reason for hiding this comment

Uh oh!

akuzm left a comment

Choose a reason for hiding this comment

Uh oh!

akuzm Jul 23, 2021

Choose a reason for hiding this comment

Uh oh!

akuzm commented Jul 27, 2021

Uh oh!

alexey-milovidov commented Sep 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexey-milovidov Sep 25, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ryzuo commented Jul 15, 2021 •

edited by akuzm

Loading

alexey-milovidov commented Sep 25, 2021 •

edited

Loading