You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@doris.apache.org by GitBox <gi...@apache.org> on 2022/07/26 12:58:27 UTC
[GitHub] [doris] BiteTheDDDDt commented on a diff in pull request #11187: [Vectorized] Support order by aggregate function
BiteTheDDDDt commented on code in PR #11187:
URL: https://github.com/apache/doris/pull/11187#discussion_r929920458
##########
be/src/vec/aggregate_functions/aggregate_function_sort.h:
##########
@@ -155,13 +143,13 @@ class AggregateFunctionSort
void insert_result_into(ConstAggregateDataPtr targetplace, IColumn& to) const override {
auto place = const_cast<AggregateDataPtr>(targetplace);
- if (!this->data(place).block.is_empty_column()) {
+ if (!this->data(place)._block.is_empty_column()) {
Review Comment:
Maybe we can just use `block` instead `_block` in struct.
##########
fe/fe-core/src/main/java/org/apache/doris/analysis/FunctionCallExpr.java:
##########
@@ -513,13 +538,14 @@ private void analyzeBuiltinAggFunction(Analyzer analyzer) throws AnalysisExcepti
"group_concat requires first parameter to be of type STRING: " + this.toSql());
}
- if (children.size() == 2) {
+ if (children.size() == 2 && orderByElements.isEmpty()) {
Review Comment:
Maybe `children.size()-orderByElements.size()==2` is better.
##########
be/src/vec/aggregate_functions/aggregate_function_sort.h:
##########
@@ -64,59 +72,34 @@ struct AggregateFunctionSortData {
PBlock pblock;
pblock.ParseFromString(data);
- new (&block) Block(pblock);
+ new (&_block) Block(pblock);
}
void add(const IColumn** columns, size_t columns_num, size_t row_num) {
- DCHECK(block.columns() == columns_num)
+ DCHECK(_block.columns() == columns_num)
<< fmt::format("block.columns()!=columns_num, block.columns()={}, columns_num={}",
- block.columns(), columns_num);
+ _block.columns(), columns_num);
for (size_t i = 0; i < columns_num; ++i) {
- auto column = block.get_by_position(i).column->assume_mutable();
+ auto column = _block.get_by_position(i).column->assume_mutable();
column->insert_from(*columns[i], row_num);
}
}
- void sort() {
- size_t sort_desc_idx = block.columns() - sort_column_size - 1;
- StringRef desc_str =
- block.get_by_position(sort_desc_idx).column->assume_mutable()->get_data_at(0);
- DCHECK(sort_column_size == desc_str.size);
-
- SortDescription sort_description(sort_column_size);
- for (size_t i = 0; i < sort_column_size; i++) {
- sort_description[i].column_number = sort_desc_idx + 1 + i;
- sort_description[i].direction = (desc_str.data[i] == '0') ? 1 : -1;
- sort_description[i].nulls_direction = sort_description[i].direction;
- }
-
- sort_block(block, sort_description, block.rows());
- }
-
- void try_init(const DataTypes& _arguments) {
- if (!block.is_empty_column()) {
- return;
- }
-
- for (auto type : _arguments) {
- block.insert({type, ""});
- }
- }
-
- Block block;
+ void sort() { sort_block(_block, _sort_desc, _block.rows()); }
};
-template <int sort_column_size, template <int> typename Data>
+template <typename Data>
class AggregateFunctionSort
- : public IAggregateFunctionDataHelper<Data<sort_column_size>,
- AggregateFunctionSort<sort_column_size, Data>> {
- using DataReal = Data<sort_column_size>;
+ : public IAggregateFunctionDataHelper<Data, AggregateFunctionSort<Data>> {
+ using DataReal = Data;
Review Comment:
We can remove `DataReal` directly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org