-
Notifications
You must be signed in to change notification settings - Fork 411
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
Add decrease interface for aggregation #9737
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 |
{ | ||
this->data(place).sum -= static_cast<const ColumnVector<T> &>(*columns[0]).getData()[row_num]; | ||
} | ||
--this->data(place).count; |
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.
add assert(this->data(place).count >= 0)
?
if constexpr (is_add) | ||
nested_func->add(nested_state, nested, i, arena); | ||
else | ||
nested_func->decrease(nested_state, nested, i, 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.
why not just call nested_func->addOrDecrease<is_add>()
?
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 not just call
nested_func->addOrDecrease<is_add>()
?
Because addOrDecrease<>()
is not an interface in IAggregateFunction
.
if constexpr (is_add) | ||
this->nested_function->add(this->nestedPlace(place), &nested_column, row_num, arena); | ||
else | ||
this->nested_function->decrease(this->nestedPlace(place), &nested_column, row_num, 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.
Ditto
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.
Ditto
Because addOrDecrease<>()
is not an interface in IAggregateFunction
.
@@ -64,6 +64,16 @@ class AggregateFunctionMerge final : public IAggregateFunctionHelper<AggregateFu | |||
nested_func->merge(place, static_cast<const ColumnAggregateFunction &>(*columns[0]).getData()[row_num], arena); | |||
} | |||
|
|||
void decrease(AggregateDataPtr __restrict place, const IColumn ** columns, size_t row_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.
decrease
is the same as add
?
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.
decrease
is the same asadd
?
AggregateFunctionMerge
is impossible to be used in window agg. It's just a placeholder to pass the compilation. I will delete it.
if constexpr (is_add) | ||
this->nested_function->add(this->nestedPlace(place), &nested_column, row_num, arena); | ||
else | ||
this->nested_function->decrease(this->nestedPlace(place), &nested_column, row_num, arena); | ||
} | ||
} | ||
} | ||
else | ||
{ | ||
this->setFlag(place, 1); |
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.
The flag actually has its meanings, you should maintain the flag for decrease
* It can only be allocated and destroyed. | ||
* MemoryTracker is not used. AlignedBuffer is intended for small pieces of memory. | ||
*/ | ||
class AlignedBuffer : private boost::noncopyable |
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 class is not used?
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 class is not used?
It's used in test in this pr. And will also be used in further pr.
@@ -43,9 +45,18 @@ struct SingleValueDataFixed | |||
= false; /// We need to remember if at least one value has been passed. This is necessary for AggregateFunctionIf. | |||
T value; | |||
|
|||
// It's only used in window aggregation | |||
mutable std::deque<T> * saved_values; |
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.
I suggest to implement window_min/window_max function which support decrease
.
if constexpr (IsDecimal<T>) | ||
this->data(place).sum -= static_cast<const ColumnDecimal<T> &>(*columns[0]).getData()[row_num]; | ||
else | ||
{ |
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.
remove { }
?
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.
remove
{ }
?
okk
|
||
void AlignedBuffer::alloc(size_t size, size_t alignment) | ||
{ | ||
void * new_buf; |
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 alloc()
is called two times without calling daealloc()
or reset()
, there will be memory leak.
Maybe we can add some check to avoid it?
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
alloc()
is called two times without callingdaealloc()
orreset()
, there will be memory leak. Maybe we can add some check to avoid it?
It's impossible to call alloc()
two times. Because it's a private function and only be used in constructor and reset()
.
What problem does this PR solve?
Issue Number: ref #7376
Problem Summary:
What is changed and how it works?
Check List
Tests
Side effects
Documentation
Release note