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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
