github-actions[bot] commented on code in PR #16100:
URL: https://github.com/apache/doris/pull/16100#discussion_r1096932828
##########
be/src/exec/schema_scanner.cpp:
##########
@@ -145,23 +132,161 @@
}
}
-Status SchemaScanner::create_columns(const std::vector<TSchemaTableStructure>*
table_structure,
- ObjectPool* pool) {
- _column_num = table_structure->size();
- _columns = new ColumnDesc[_column_num];
- _is_create_columns = true;
- for (size_t idx = 0; idx < table_structure->size(); ++idx) {
- _columns[idx].name = table_structure->at(idx).column_name.c_str();
- _columns[idx].type = thrift_to_type(table_structure->at(idx).type);
- _columns[idx].size = table_structure->at(idx).len;
- _columns[idx].is_null = table_structure->at(idx).is_null;
+Status SchemaScanner::fill_dest_column(vectorized::Block* block, void* data,
+ const ColumnDesc& col_desc) {
+ if (!block->has(col_desc.name)) {
+ return Status::OK();
+ }
+ vectorized::MutableColumnPtr column_ptr =
+
std::move(*block->get_by_name(col_desc.name).column).assume_mutable();
Review Comment:
warning: std::move of the const expression has no effect; remove std::move()
[performance-move-const-arg]
```suggestion
*block->get_by_name(col_desc.name).column.assume_mutable();
```
##########
be/src/exec/schema_scanner.cpp:
##########
@@ -33,34 +33,24 @@
#include "exec/schema_scanner/schema_variables_scanner.h"
#include "exec/schema_scanner/schema_views_scanner.h"
#include "runtime/define_primitive_type.h"
+#include "vec/columns/column.h"
#include "vec/common/string_ref.h"
+#include "vec/core/block.h"
namespace doris {
DorisServer* SchemaScanner::_s_doris_server;
-SchemaScanner::SchemaScanner(ColumnDesc* columns, int column_num)
+SchemaScanner::SchemaScanner(const std::vector<ColumnDesc>& columns)
: _is_init(false),
_param(nullptr),
_columns(columns),
- _column_num(column_num),
- _tuple_desc(nullptr),
_schema_table_type(TSchemaTableType::SCH_INVALID) {}
-SchemaScanner::SchemaScanner(ColumnDesc* columns, int column_num,
TSchemaTableType::type type)
- : _is_init(false),
- _param(nullptr),
- _columns(columns),
- _column_num(column_num),
- _tuple_desc(nullptr),
- _schema_table_type(type) {}
+SchemaScanner::SchemaScanner(const std::vector<ColumnDesc>& columns,
TSchemaTableType::type type)
+ : _is_init(false), _param(nullptr), _columns(columns),
_schema_table_type(type) {}
-SchemaScanner::~SchemaScanner() {
- if (_is_create_columns == true && _columns != nullptr) {
- delete[] _columns;
- _columns = nullptr;
- }
-}
+SchemaScanner::~SchemaScanner() {}
Review Comment:
warning: use '= default' to define a trivial destructor
[modernize-use-equals-default]
```suggestion
SchemaScanner::~SchemaScanner() = default;
```
##########
be/src/gutil/cpu.cc:
##########
@@ -253,6 +256,7 @@ void CPU::Initialize() {
#endif
}
CPU::IntelMicroArchitecture CPU::GetIntelMicroArchitecture() const {
+ if (has_avx512()) return AVX512;
Review Comment:
warning: statement should be inside braces
[readability-braces-around-statements]
```suggestion
if (has_avx512()) { return AVX512;
}
```
##########
be/src/olap/tablet_meta.cpp:
##########
@@ -877,13 +868,7 @@ bool operator==(const TabletMeta& a, const TabletMeta& b) {
}
if (a._in_restore_mode != b._in_restore_mode) return false;
if (a._preferred_rowset_type != b._preferred_rowset_type) return false;
- if (a._storage_policy != b._storage_policy) return false;
- if (a._cooldown_replica_id != b._cooldown_replica_id) {
- return false;
- }
- if (a._cooldown_term != b._cooldown_term) {
- return false;
- }
+ if (a._storage_policy_id != b._storage_policy_id) return false;
Review Comment:
warning: statement should be inside braces
[readability-braces-around-statements]
```suggestion
if (a._storage_policy_id != b._storage_policy_id) { return false;
}
```
##########
be/src/vec/exec/vschema_scan_node.cpp:
##########
@@ -33,26 +40,12 @@ VSchemaScanNode::VSchemaScanNode(ObjectPool* pool, const
TPlanNode& tnode,
_is_init(false),
_table_name(tnode.schema_scan_node.table_name),
_tuple_id(tnode.schema_scan_node.tuple_id),
- _src_tuple_desc(nullptr),
_dest_tuple_desc(nullptr),
_tuple_idx(0),
_slot_num(0),
- _tuple_pool(nullptr),
- _schema_scanner(nullptr),
- _src_tuple(nullptr),
- _src_single_tuple(nullptr),
- _dest_single_tuple(nullptr) {}
-
-VSchemaScanNode::~VSchemaScanNode() {
- delete[] reinterpret_cast<char*>(_src_tuple);
- _src_tuple = nullptr;
+ _schema_scanner(nullptr) {}
- delete[] reinterpret_cast<char*>(_src_single_tuple);
- _src_single_tuple = nullptr;
-
- delete[] reinterpret_cast<char*>(_dest_single_tuple);
- _dest_single_tuple = nullptr;
-}
+VSchemaScanNode::~VSchemaScanNode() {}
Review Comment:
warning: use '= default' to define a trivial destructor
[modernize-use-equals-default]
```suggestion
VSchemaScanNode::~VSchemaScanNode() = default;
```
##########
be/src/runtime/result_writer.h:
##########
@@ -33,9 +33,9 @@
// abstract class of the result writer
class ResultWriter {
public:
- ResultWriter() {};
- ResultWriter(bool output_object_data) :
_output_object_data(output_object_data) {};
- ~ResultWriter() {};
+ ResultWriter() {}
+ ResultWriter(bool output_object_data) :
_output_object_data(output_object_data) {}
+ ~ResultWriter() {}
Review Comment:
warning: use '= default' to define a trivial destructor
[modernize-use-equals-default]
```suggestion
~ResultWriter() = default;
```
##########
be/test/http/stream_load_test.cpp:
##########
@@ -71,7 +70,6 @@ class StreamLoadActionTest : public testing::Test {
k_response_str = "";
config::streaming_load_max_mb = 1;
- _env._thread_mgr = new ThreadResourceMgr();
_env._master_info = new TMasterInfo();
Review Comment:
warning: '_master_info' is a private member of 'doris::ExecEnv'
[clang-diagnostic-error]
```cpp
_env._master_info = new TMasterInfo();
^
```
**be/src/runtime/exec_env.h:225:** declared private here
```cpp
TMasterInfo* _master_info = nullptr;
^
```
##########
be/test/http/stream_load_test.cpp:
##########
@@ -71,7 +70,6 @@
k_response_str = "";
config::streaming_load_max_mb = 1;
- _env._thread_mgr = new ThreadResourceMgr();
_env._master_info = new TMasterInfo();
_env._load_stream_mgr = new LoadStreamMgr();
Review Comment:
warning: '_load_stream_mgr' is a private member of 'doris::ExecEnv'
[clang-diagnostic-error]
```cpp
_env._load_stream_mgr = new LoadStreamMgr();
^
```
**be/src/runtime/exec_env.h:232:** declared private here
```cpp
LoadStreamMgr* _load_stream_mgr = nullptr;
^
```
##########
be/test/http/stream_load_test.cpp:
##########
@@ -89,8 +87,6 @@
_env._load_stream_mgr = nullptr;
delete _env._master_info;
_env._master_info = nullptr;
Review Comment:
warning: '_master_info' is a private member of 'doris::ExecEnv'
[clang-diagnostic-error]
```cpp
_env._master_info = nullptr;
^
```
**be/src/runtime/exec_env.h:225:** declared private here
```cpp
TMasterInfo* _master_info = nullptr;
^
```
##########
be/src/io/fs/s3_file_system.h:
##########
@@ -68,19 +68,17 @@ class S3FileSystem final : public RemoteFileSystem {
std::shared_ptr<Aws::S3::S3Client> get_client() const {
std::lock_guard lock(_client_mu);
return _client;
- };
+ }
// Guarded by external lock.
- void set_ak(std::string ak) { _s3_conf.ak = std::move(ak); }
-
- // Guarded by external lock.
- void set_sk(std::string sk) { _s3_conf.sk = std::move(sk); }
+ void set_conf(S3Conf s3_conf) { _s3_conf = std::move(s3_conf); }
std::string get_key(const Path& path) const;
private:
- S3FileSystem(S3Conf s3_conf, ResourceId resource_id);
+ S3FileSystem(S3Conf&& s3_conf, std::string&& id);
+private:
Review Comment:
warning: redundant access specifier has the same accessibility as the
previous access specifier [readability-redundant-access-specifiers]
```suggestion
```
**be/src/io/fs/s3_file_system.h:77:** previously declared here
```cpp
private:
^
```
##########
be/src/olap/cumulative_compaction_policy.cpp:
##########
@@ -334,13 +324,12 @@ int
SizeBasedCumulativeCompactionPolicy::pick_input_rowsets(
return transient_size;
}
-int SizeBasedCumulativeCompactionPolicy::_level_size(const int64_t size) {
- for (auto& i : _levels) {
- if (size >= i) {
- return i;
- }
- }
- return 0;
+int64_t SizeBasedCumulativeCompactionPolicy::_level_size(const int64_t size) {
+ if (size < 1024) return 0;
Review Comment:
warning: statement should be inside braces
[readability-braces-around-statements]
```suggestion
if (size < 1024) { return 0;
}
```
##########
be/src/olap/cumulative_compaction_policy.cpp:
##########
@@ -334,13 +324,12 @@
return transient_size;
}
-int SizeBasedCumulativeCompactionPolicy::_level_size(const int64_t size) {
- for (auto& i : _levels) {
- if (size >= i) {
- return i;
- }
- }
- return 0;
+int64_t SizeBasedCumulativeCompactionPolicy::_level_size(const int64_t size) {
+ if (size < 1024) return 0;
+ int64_t max_level = (int64_t)1
+ << (sizeof(_promotion_size) * 8 - 1 -
__builtin_clzl(_promotion_size / 2));
+ if (size >= max_level) return max_level;
Review Comment:
warning: statement should be inside braces
[readability-braces-around-statements]
```suggestion
if (size >= max_level) { return max_level;
}
```
##########
be/src/runtime/result_writer.h:
##########
@@ -33,9 +33,9 @@ class Block;
// abstract class of the result writer
class ResultWriter {
public:
- ResultWriter() {};
- ResultWriter(bool output_object_data) :
_output_object_data(output_object_data) {};
- ~ResultWriter() {};
+ ResultWriter() {}
Review Comment:
warning: use '= default' to define a trivial default constructor
[modernize-use-equals-default]
```suggestion
ResultWriter() = default;
```
##########
be/src/runtime/memory/mem_tracker_limiter.cpp:
##########
@@ -195,26 +196,35 @@ void MemTrackerLimiter::print_log_usage(const
std::string& msg) {
}
}
+std::string MemTrackerLimiter::log_process_usage_str(const std::string& msg,
bool with_stacktrace) {
+ std::string detail = msg;
+ detail += "\nProcess Memory Summary:\n " +
MemTrackerLimiter::process_mem_log_str();
+ if (with_stacktrace) detail += "\nAlloc Stacktrace:\n" + get_stack_trace();
Review Comment:
warning: statement should be inside braces
[readability-braces-around-statements]
```suggestion
if (with_stacktrace) { detail += "\nAlloc Stacktrace:\n" +
get_stack_trace();
}
```
##########
be/test/agent/cgroups_mgr_test.cpp:
##########
@@ -21,13 +21,8 @@
#include <filesystem>
#include <fstream>
-#include "gmock/gmock.h"
#include "gtest/gtest.h"
-#ifndef BE_TEST
-#define BE_TEST
-#endif
-
using ::testing::_;
using ::testing::Return;
Review Comment:
warning: no member named 'Return' in namespace 'testing'
[clang-diagnostic-error]
```cpp
using ::testing::Return;
^
```
##########
be/src/vec/exec/vschema_scan_node.cpp:
##########
@@ -255,254 +212,70 @@
return Status::InternalError("used before initialize.");
}
RETURN_IF_CANCELLED(state);
- std::vector<vectorized::MutableColumnPtr> columns(_slot_num);
bool schema_eos = false;
- do {
- bool mem_reuse = block->mem_reuse();
- DCHECK(block->rows() == 0);
+ const std::vector<SchemaScanner::ColumnDesc>&
columns_desc(_schema_scanner->get_column_desc());
- columns.resize(_slot_num);
+ do {
+ block->clear();
for (int i = 0; i < _slot_num; ++i) {
- if (mem_reuse) {
- columns[i] =
std::move(*block->get_by_position(i).column).mutate();
- } else {
- columns[i] =
_dest_tuple_desc->slots()[i]->get_empty_mutable_column();
- }
+ auto dest_slot_desc = _dest_tuple_desc->slots()[i];
+
block->insert(ColumnWithTypeAndName(dest_slot_desc->get_empty_mutable_column(),
+
dest_slot_desc->get_data_type_ptr(),
+ dest_slot_desc->col_name()));
+ }
+
+ vectorized::Block src_block;
+ for (int i = 0; i < columns_desc.size(); ++i) {
+ TypeDescriptor descriptor(columns_desc[i].type);
+ auto data_type =
+
vectorized::DataTypeFactory::instance().create_data_type(descriptor, true);
+ src_block.insert(ColumnWithTypeAndName(data_type->create_column(),
data_type,
+ columns_desc[i].name));
}
while (true) {
RETURN_IF_CANCELLED(state);
// get all slots from schema table.
- RETURN_IF_ERROR(_schema_scanner->get_next_row(_src_single_tuple,
_tuple_pool.get(),
- &schema_eos));
+ RETURN_IF_ERROR(_schema_scanner->get_next_block(&src_block,
&schema_eos));
+
if (schema_eos) {
*eos = true;
break;
}
- // tuple project
- project_tuple();
- for (int i = 0; i < _slot_num; ++i) {
- auto slot_desc = _dest_tuple_desc->slots()[i];
- if (!slot_desc->is_materialized()) {
- continue;
- }
-
- if
(_dest_single_tuple->is_null(slot_desc->null_indicator_offset())) {
- if (slot_desc->is_nullable()) {
- auto* nullable_column =
-
reinterpret_cast<vectorized::ColumnNullable*>(columns[i].get());
- nullable_column->insert_data(nullptr, 0);
- } else {
- return Status::InternalError(
- "nonnull column contains NULL. table={},
column={}", _table_name,
- slot_desc->col_name());
- }
- } else {
- RETURN_IF_ERROR(write_slot_to_vectorized_column(
-
_dest_single_tuple->get_slot(slot_desc->tuple_offset()), slot_desc,
- &columns[i]));
- }
- }
- if (columns[0]->size() == state->batch_size()) {
+ if (src_block.rows() >= state->batch_size()) {
break;
}
}
- if (!columns.empty() && !columns[0]->empty()) {
- auto n_columns = 0;
- if (!mem_reuse) {
- for (const auto slot_desc : _dest_tuple_desc->slots()) {
-
block->insert(ColumnWithTypeAndName(std::move(columns[n_columns++]),
-
slot_desc->get_data_type_ptr(),
-
slot_desc->col_name()));
- }
- } else {
- columns.clear();
+
+ if (src_block.rows()) {
+ // block->check_number_of_rows();
+ for (int i = 0; i < _slot_num; ++i) {
+ auto dest_slot_desc = _dest_tuple_desc->slots()[i];
+ vectorized::MutableColumnPtr column_ptr =
+ std::move(*block->get_by_position(i).column).mutate();
Review Comment:
warning: std::move of the const expression has no effect; remove std::move()
[performance-move-const-arg]
```suggestion
*block->get_by_position(i).column.mutate();
```
##########
be/src/util/mem_info.cpp:
##########
@@ -47,6 +47,7 @@ int64_t MemInfo::_s_physical_mem = -1;
int64_t MemInfo::_s_mem_limit = -1;
std::string MemInfo::_s_mem_limit_str = "";
int64_t MemInfo::_s_soft_mem_limit = -1;
+std::string MemInfo::_s_soft_mem_limit_str = "";
Review Comment:
warning: redundant string initialization [readability-redundant-string-init]
```suggestion
std::string MemInfo::_s_soft_mem_limit_str;
```
##########
be/test/agent/cgroups_mgr_test.cpp:
##########
@@ -21,13 +21,8 @@
#include <filesystem>
#include <fstream>
-#include "gmock/gmock.h"
#include "gtest/gtest.h"
-#ifndef BE_TEST
-#define BE_TEST
-#endif
-
using ::testing::_;
using ::testing::Return;
using ::testing::SetArgPointee;
Review Comment:
warning: no member named 'SetArgPointee' in namespace 'testing'
[clang-diagnostic-error]
```cpp
using ::testing::SetArgPointee;
^
```
##########
be/test/http/stream_load_test.cpp:
##########
@@ -71,7 +70,6 @@
k_response_str = "";
config::streaming_load_max_mb = 1;
- _env._thread_mgr = new ThreadResourceMgr();
_env._master_info = new TMasterInfo();
_env._load_stream_mgr = new LoadStreamMgr();
_env._internal_client_cache = new
BrpcClientCache<PBackendService_Stub>();
Review Comment:
warning: '_internal_client_cache' is a private member of 'doris::ExecEnv'
[clang-diagnostic-error]
```cpp
_env._internal_client_cache = new
BrpcClientCache<PBackendService_Stub>();
^
```
**be/src/runtime/exec_env.h:234:** declared private here
```cpp
BrpcClientCache<PBackendService_Stub>* _internal_client_cache = nullptr;
^
```
##########
be/test/agent/cgroups_mgr_test.cpp:
##########
@@ -21,13 +21,8 @@
#include <filesystem>
#include <fstream>
-#include "gmock/gmock.h"
#include "gtest/gtest.h"
-#ifndef BE_TEST
-#define BE_TEST
-#endif
-
using ::testing::_;
Review Comment:
warning: no member named '_' in namespace 'testing' [clang-diagnostic-error]
```cpp
using ::testing::_;
^
```
##########
be/test/http/stream_load_test.cpp:
##########
@@ -89,8 +87,6 @@
_env._load_stream_mgr = nullptr;
delete _env._master_info;
Review Comment:
warning: '_master_info' is a private member of 'doris::ExecEnv'
[clang-diagnostic-error]
```cpp
delete _env._master_info;
^
```
**be/src/runtime/exec_env.h:225:** declared private here
```cpp
TMasterInfo* _master_info = nullptr;
^
```
##########
be/test/http/stream_load_test.cpp:
##########
@@ -89,8 +87,6 @@
_env._load_stream_mgr = nullptr;
Review Comment:
warning: '_load_stream_mgr' is a private member of 'doris::ExecEnv'
[clang-diagnostic-error]
```cpp
_env._load_stream_mgr = nullptr;
^
```
**be/src/runtime/exec_env.h:232:** declared private here
```cpp
LoadStreamMgr* _load_stream_mgr = nullptr;
^
```
--
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]
