-
Notifications
You must be signed in to change notification settings - Fork 410
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refine window when preceding and following boundary type are unbounded #9916
base: master
Are you sure you want to change the base?
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
||
has_shortcut = window_description_.frame.begin_type == WindowFrame::BoundaryType::Unbounded | ||
&& window_description_.frame.end_type == WindowFrame::BoundaryType::Unbounded | ||
&& !aggregation_workspaces.empty(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and window_workspaces
should be empty?
@@ -292,6 +295,9 @@ void WindowTransformAction::initialWorkspaces() | |||
} | |||
} | |||
|
|||
// Can not have window and agg functions together in one window | |||
assert((!aggregation_workspaces.empty() && !window_workspaces.empty()) == false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why have this restriction?
Block WindowTransformAction::tryGetOutputBlock() | ||
{ | ||
// first try calculate the result based on current data | ||
tryCalculate(); | ||
if (window_description.need_decrease) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if need_decrease is true, has_shortcut should always be false?
/hold |
@@ -119,6 +119,11 @@ class IAggregateFunction | |||
/// Inserts results into a column. | |||
virtual void insertResultInto(ConstAggregateDataPtr __restrict place, IColumn & to, Arena * arena) const = 0; | |||
|
|||
/// Inserts batch results into a column | |||
virtual void insertBatchResultInto(ConstAggregateDataPtr __restrict place, IColumn & to, size_t num, Arena * arena) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this method try to insert the same place
into to
column for num
rows.
IMO the method name insertBatchResultInto
should do things like insert many places
into a column, just like IAggregateFunction::addBatch()
.
Maybe you can change to batchInsertSameResultInto
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe later we will add a new function batchInsertResultInto()
, which will insert many places
into a column, this can avoid calling insertResultInto()
each time for each row, which can avoid virtual function call for Aggregation.
What problem does this PR solve?
Issue Number: close #9913
Problem Summary:
What is changed and how it works?
Check List
Tests
Side effects
Documentation
Release note