Skip to content

[Feature](Variant) support variant load - #24554

Closed
eldenmoon wants to merge 8 commits into
apache:masterfrom
eldenmoon:var-load
Closed

[Feature](Variant) support variant load#24554
eldenmoon wants to merge 8 commits into
apache:masterfrom
eldenmoon:var-load

Conversation

@eldenmoon

@eldenmooneldenmoon commented Sep 18, 2023

Copy link
Copy Markdown
Member

Proposed changes

This PR is the load part of variant column

related #26225
Issue Number: close #xxx

Further comments

If this is a relatively large or complex change, kick off the discussion at dev@doris.apache.org by explaining why you chose the solution you did and what alternatives you considered, etc...

@github-actionsgithub-actionsBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy made some suggestions

Comment threadbe/src/udf/udf.h
Comment threadbe/src/udf/udf.h
@eldenmoon

Copy link
Copy Markdown
MemberAuthor

run buildall

@doris-robot

Copy link
Copy Markdown

TeamCity be ut coverage result:
Function Coverage: 36.57% (7990/21847)
Line Coverage: 28.64% (64068/223725)
Region Coverage: 27.52% (33283/120926)
Branch Coverage: 24.16% (17054/70590)
Coverage Report: http://coverage.selectdb-in.cc/coverage/1b2f9420f2d4ddbda3c129dc92b04b7d62b2e124_1b2f9420f2d4ddbda3c129dc92b04b7d62b2e124/report/index.html

@eldenmoon

Copy link
Copy Markdown
MemberAuthor

run buildall

@doris-robot

Copy link
Copy Markdown

TeamCity be ut coverage result:
Function Coverage: 36.58% (7990/21845)
Line Coverage: 28.64% (64079/223736)
Region Coverage: 27.53% (33286/120930)
Branch Coverage: 24.17% (17062/70594)
Coverage Report: http://coverage.selectdb-in.cc/coverage/cff9e79c025a3418f0b18d1af247051e0ac6b53b_cff9e79c025a3418f0b18d1af247051e0ac6b53b/report/index.html

@eldenmoon

Copy link
Copy Markdown
MemberAuthor

run buildall

2 similar comments
@eldenmoon

Copy link
Copy Markdown
MemberAuthor

run buildall

@eldenmoon

Copy link
Copy Markdown
MemberAuthor

run buildall

@doris-robot

Copy link
Copy Markdown

TeamCity be ut coverage result:
Function Coverage: 36.58% (7990/21845)
Line Coverage: 28.64% (64084/223738)
Region Coverage: 27.52% (33286/120931)
Branch Coverage: 24.17% (17062/70594)
Coverage Report: http://coverage.selectdb-in.cc/coverage/ddd048c7006e1e3a5305b12a1b5a251a952dca6c_ddd048c7006e1e3a5305b12a1b5a251a952dca6c/report/index.html

@doris-robot

Copy link
Copy Markdown

TeamCity be ut coverage result:
Function Coverage: 36.58% (7990/21845)
Line Coverage: 28.64% (64077/223738)
Region Coverage: 27.53% (33290/120931)
Branch Coverage: 24.17% (17061/70594)
Coverage Report: http://coverage.selectdb-in.cc/coverage/ddd048c7006e1e3a5305b12a1b5a251a952dca6c_ddd048c7006e1e3a5305b12a1b5a251a952dca6c/report/index.html

@doris-robot

Copy link
Copy Markdown

(From new machine)TeamCity pipeline, clickbench performance test result:
the sum of best hot time: 46.39 seconds
stream load tsv: 600 seconds loaded 74807831229 Bytes, about 118 MB/s
stream load json: 20 seconds loaded 2358488459 Bytes, about 112 MB/s
stream load orc: 64 seconds loaded 1101869774 Bytes, about 16 MB/s
stream load parquet: 31 seconds loaded 861443392 Bytes, about 26 MB/s
insert into select: 28.8 seconds inserted 10000000 Rows, about 347K ops/s
storage size: 17162367314 Bytes

@eldenmoon

Copy link
Copy Markdown
MemberAuthor

run buildall

@doris-robot

Copy link
Copy Markdown

TeamCity be ut coverage result:
Function Coverage: 36.43% (7990/21933)
Line Coverage: 28.52% (64080/224697)
Region Coverage: 27.42% (33296/121434)
Branch Coverage: 24.08% (17062/70850)
Coverage Report: http://coverage.selectdb-in.cc/coverage/812d71c1980ddfe12aed2b2820cea13a78f613ac_812d71c1980ddfe12aed2b2820cea13a78f613ac/report/index.html

@doris-robot

Copy link
Copy Markdown

(From new machine)TeamCity pipeline, clickbench performance test result:
the sum of best hot time: 45.26 seconds
stream load tsv: 612 seconds loaded 74807831229 Bytes, about 116 MB/s
stream load json: 20 seconds loaded 2358488459 Bytes, about 112 MB/s
stream load orc: 64 seconds loaded 1101869774 Bytes, about 16 MB/s
stream load parquet: 32 seconds loaded 861443392 Bytes, about 25 MB/s
insert into select: 28.9 seconds inserted 10000000 Rows, about 346K ops/s
storage size: 17162433259 Bytes

@xiaokangxiaokang left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

olap/rowset reviewed

Comment threadbe/src/olap/rowset/beta_rowset_writer.cpp
Comment threadbe/src/olap/rowset/beta_rowset_writer.cpp
Comment threadbe/src/vec/common/schema_util.cpp
Comment threadbe/src/olap/rowset/beta_rowset_writer_v2.h Outdated
Comment threadbe/src/olap/rowset/rowset_writer.h Outdated
Comment threadbe/src/olap/rowset/segment_v2/encoding_info.cpp
Comment threadbe/src/olap/tablet.cpp Outdated
Comment threadbe/src/olap/tablet.cpp
Comment threadbe/src/olap/tablet_meta.cpp
Comment threadbe/src/vec/olap/olap_data_convertor.cpp

@github-actionsgithub-actionsBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy made some suggestions

There were too many comments to post at once. Showing the first 25 out of 65. Check the log or trigger a new build to see more.

Comment threadbe/src/vec/columns/column.h Outdated
Comment threadbe/src/vec/columns/column.h
Comment threadbe/src/vec/columns/column.h
Comment threadbe/src/vec/columns/column.h
Comment threadbe/src/vec/columns/column_array.h
Comment threadbe/src/vec/columns/column_object.h
Comment threadbe/src/vec/columns/column_object.h
Comment threadbe/src/vec/columns/column_object.h
Comment threadbe/src/vec/columns/column_object.h
Comment threadbe/src/vec/columns/column_object.h Outdated

@github-actionsgithub-actionsBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy made some suggestions

There were too many comments to post at once. Showing the first 25 out of 44. Check the log or trigger a new build to see more.

Comment threadbe/src/olap/rowset/beta_rowset_writer.h Outdated
Comment threadbe/src/olap/rowset/beta_rowset_writer_v2.h
Comment threadbe/src/olap/rowset/rowset_writer.h
subcolumns.get_root()->data.get_least_common_type()->get_name(), path.get_path()); \
}

Status ColumnObject::extract_root(const PathInData& path) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: method 'extract_root' can be made const [readability-make-member-function-const]

be/src/vec/columns/column_object.h:457:

- Status extract_root(const PathInData& path);+ Status extract_root(const PathInData& path) const;
Suggested change
Status ColumnObject::extract_root(const PathInData& path) {
Status ColumnObject::extract_root(const PathInData& path) const{

Comment threadbe/src/vec/columns/column_object.h
return TPrimitiveType::INVALID_TYPE;
}

doris::FieldType get_type_as_field_type() const override {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: function 'get_type_as_field_type' should be marked [[nodiscard]] [modernize-use-nodiscard]

Suggested change
doris::FieldTypeget_type_as_field_type() constoverride {
[[nodiscard]] doris::FieldTypeget_type_as_field_type() constoverride {

return TPrimitiveType::HLL;
}

doris::FieldType get_type_as_field_type() const override {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: function 'get_type_as_field_type' should be marked [[nodiscard]] [modernize-use-nodiscard]

Suggested change
doris::FieldTypeget_type_as_field_type() constoverride {
[[nodiscard]] doris::FieldTypeget_type_as_field_type() constoverride {

return TPrimitiveType::JSONB;
}

doris::FieldType get_type_as_field_type() const override {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: function 'get_type_as_field_type' should be marked [[nodiscard]] [modernize-use-nodiscard]

Suggested change
doris::FieldTypeget_type_as_field_type() constoverride {
[[nodiscard]] doris::FieldTypeget_type_as_field_type() constoverride {

__builtin_unreachable();
}

doris::FieldType get_type_as_field_type() const override {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: function 'get_type_as_field_type' should be marked [[nodiscard]] [modernize-use-nodiscard]

Suggested change
doris::FieldTypeget_type_as_field_type() constoverride {
[[nodiscard]] doris::FieldTypeget_type_as_field_type() constoverride {

// and modified by Doris

#pragma once
#include <gen_cpp/Exprs_types.h>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: 'gen_cpp/Exprs_types.h' file not found [clang-diagnostic-error]

#include<gen_cpp/Exprs_types.h>
^

@eldenmoon

Copy link
Copy Markdown
MemberAuthor

run buildall

@doris-robot

Copy link
Copy Markdown

TeamCity be ut coverage result:
Function Coverage: 36.05% (8161/22641)
Line Coverage: 28.22% (65240/231182)
Region Coverage: 27.15% (33822/124562)
Branch Coverage: 23.84% (17250/72360)
Coverage Report: http://coverage.selectdb-in.cc/coverage/b8ed00c82e9f407035f0b580ae79062642a5dabb_b8ed00c82e9f407035f0b580ae79062642a5dabb/report/index.html

@doris-robot

Copy link
Copy Markdown

(From new machine)TeamCity pipeline, clickbench performance test result:
the sum of best hot time: 45.49 seconds
stream load tsv: 578 seconds loaded 74807831229 Bytes, about 123 MB/s
stream load json: 20 seconds loaded 2358488459 Bytes, about 112 MB/s
stream load orc: 64 seconds loaded 1101869774 Bytes, about 16 MB/s
stream load parquet: 32 seconds loaded 861443392 Bytes, about 25 MB/s
insert into select: 29.0 seconds inserted 10000000 Rows, about 344K ops/s
storage size: 17162544698 Bytes

@github-actionsgithub-actionsBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy made some suggestions

TPrimitiveType::type get_type_as_tprimitive_type() const override {
return TPrimitiveType::VARIANT;
}
doris::FieldType get_type_as_field_type() const override {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: function 'get_type_as_field_type' should be marked [[nodiscard]] [modernize-use-nodiscard]

Suggested change
doris::FieldTypeget_type_as_field_type() constoverride {
[[nodiscard]] doris::FieldTypeget_type_as_field_type() constoverride {

bool get_is_parametric() const override { return true; }
bool can_be_inside_nullable() const override { return true; }
bool have_subtypes() const override { return true; };
bool can_be_inside_nullable() const override { return false; }

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: function 'can_be_inside_nullable' should be marked [[nodiscard]] [modernize-use-nodiscard]

Suggested change
boolcan_be_inside_nullable() constoverride { return false; }
[[nodiscard]] boolcan_be_inside_nullable() constoverride { return false; }

const auto& column_object = assert_cast<const ColumnObject&>(column);
return "Variant: " + column_object.get_keys_str();
}
std::string to_string(const IColumn& column, size_t row_num) const override;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: function 'to_string' should be marked [[nodiscard]] [modernize-use-nodiscard]

Suggested change
std::stringto_string(constIColumn&column, size_trow_num) constoverride;
[[nodiscard]] std::stringto_string(constIColumn&column, size_trow_num) constoverride;

[[noreturn]] Field get_default() const override {
LOG(FATAL) << "Method getDefault() is not implemented for data type " << get_name();
}
Field get_default() const override { return VariantMap(); }

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: function 'get_default' should be marked [[nodiscard]] [modernize-use-nodiscard]

Suggested change
Fieldget_default() constoverride { returnVariantMap(); }
[[nodiscard]] Fieldget_default() constoverride { returnVariantMap(); }


[[noreturn]] Field get_field(const TExprNode& node) const override {
LOG(FATAL) << "Unimplemented get_field for object";
Field get_field(const TExprNode& node) const override {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: function 'get_field' should be marked [[nodiscard]] [modernize-use-nodiscard]

Suggested change
Fieldget_field(constTExprNode&node) constoverride {
[[nodiscard]] Fieldget_field(constTExprNode&node) constoverride {

};

// create cresponding variant value to wrap from_type
WrapperType create_variant_wrapper(const DataTypePtr& from_type,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: function 'create_variant_wrapper' should be marked [[nodiscard]] [modernize-use-nodiscard]

Suggested change
WrapperTypecreate_variant_wrapper(constDataTypePtr&from_type,
[[nodiscard]] WrapperTypecreate_variant_wrapper(constDataTypePtr&from_type,

}

// create cresponding type convert from variant
WrapperType create_variant_wrapper(const DataTypeObject& from_type,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: function 'create_variant_wrapper' should be marked [[nodiscard]] [modernize-use-nodiscard]

Suggested change
WrapperTypecreate_variant_wrapper(constDataTypeObject&from_type,
[[nodiscard]] WrapperTypecreate_variant_wrapper(constDataTypeObject&from_type,


class OlapColumnDataConvertorVariant : public OlapColumnDataConvertorBase {
public:
OlapColumnDataConvertorVariant()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: use '= default' to define a trivial default constructor [modernize-use-equals-default]

be/src/vec/olap/olap_data_convertor.h:488:

- : _root_data_convertor(std::make_unique<OlapColumnDataConvertorVarChar>(true)) {}+ : _root_data_convertor(std::make_unique<OlapColumnDataConvertorVarChar>(true)) = default;

size_t num_rows) override;
Status convert_to_olap() override;

const void* get_data() const override;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: function 'get_data' should be marked [[nodiscard]] [modernize-use-nodiscard]

Suggested change
constvoid*get_data() constoverride;
[[nodiscard]] constvoid*get_data() constoverride;

Status convert_to_olap() override;

const void* get_data() const override;
const void* get_data_at(size_t offset) const override;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: function 'get_data_at' should be marked [[nodiscard]] [modernize-use-nodiscard]

Suggested change
constvoid*get_data_at(size_toffset) constoverride;
[[nodiscard]] constvoid*get_data_at(size_toffset) constoverride;

@eldenmoon

Copy link
Copy Markdown
MemberAuthor

run buildall

@github-actionsgithub-actionsBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy made some suggestions

There were too many comments to post at once. Showing the first 25 out of 46. Check the log or trigger a new build to see more.

return std::string(buffer.GetString());
}

void JsonFunctions::merge_objects(rapidjson::Value& dst_object, rapidjson::Value& src_object,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: method 'merge_objects' can be made static [readability-convert-member-functions-to-static]

Suggested change
voidJsonFunctions::merge_objects(rapidjson::Value& dst_object, rapidjson::Value& src_object,
staticvoidJsonFunctions::merge_objects(rapidjson::Value& dst_object, rapidjson::Value& src_object,

}
}

void BaseTablet::update_by_least_common_schema(const TabletSchemaSPtr& update_schema) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: method 'update_by_least_common_schema' can be made static [readability-convert-member-functions-to-static]

be/src/olap/base_tablet.h:67:

- void update_by_least_common_schema(const TabletSchemaSPtr& update_schema);+ static void update_by_least_common_schema(const TabletSchemaSPtr& update_schema);

// Eg. rowset schema: A(int), B(float), C(int), D(int)
// _tabelt->tablet_schema: A(bigint), B(double)
// => update_schema: A(bigint), B(double), C(int), D(int)
void BetaRowsetWriter::update_rowset_schema(TabletSchemaSPtr flush_schema) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: method 'update_rowset_schema' can be made static [readability-convert-member-functions-to-static]

be/src/olap/rowset/beta_rowset_writer.h:174:

- void update_rowset_schema(TabletSchemaSPtr flush_schema);+ static void update_rowset_schema(TabletSchemaSPtr flush_schema);


int64_t num_rows() const override { return _total_key_group_rows; }

virtual const RowsetWriterContext& context() const override { LOG(FATAL) << "Not implemented"; }

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: 'virtual' is redundant since the function is already declared 'override' [modernize-use-override]

Suggested change
virtualconstRowsetWriterContext&context() constoverride { LOG(FATAL) << "Not implemented"; }
constRowsetWriterContext&context() constoverride { LOG(FATAL) << "Not implemented"; }

}
}

double ColumnArray::get_ratio_of_default_rows(double sample_ratio) const {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: method 'get_ratio_of_default_rows' can be made static [readability-convert-member-functions-to-static]

Suggested change
doubleColumnArray::get_ratio_of_default_rows(double sample_ratio) const{
doubleColumnArray::get_ratio_of_default_rows(double sample_ratio) {

be/src/vec/columns/column_array.h:267:

- double get_ratio_of_default_rows(double sample_ratio) const override;+ static double get_ratio_of_default_rows(double sample_ratio) override;

return get_ptr();
}

double get_ratio_of_default_rows(double sample_ratio) const override {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: method 'get_ratio_of_default_rows' can be made static [readability-convert-member-functions-to-static]

Suggested change
doubleget_ratio_of_default_rows(doublesample_ratio)constoverride {
staticdoubleget_ratio_of_default_rows(doublesample_ratio) override {

[&](const auto& subcolumn) { return subcolumn.index(indexes, limit); });
}

bool ColumnObject::Subcolumn::check_if_sparse_column(size_t num_rows) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: method 'check_if_sparse_column' can be made static [readability-convert-member-functions-to-static]

be/src/vec/columns/column_object.h:143:

- bool check_if_sparse_column(size_t num_rows);+ static bool check_if_sparse_column(size_t num_rows);

}

void ColumnObject::try_insert_range_from(const IColumn& src, size_t start, size_t length) {
FieldInfo ColumnObject::Subcolumn::get_subcolumn_field_info() const {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: method 'get_subcolumn_field_info' can be made static [readability-convert-member-functions-to-static]

be/src/vec/columns/column_object.h:148:

- FieldInfo get_subcolumn_field_info() const;+ static FieldInfo get_subcolumn_field_info() ;
Suggested change
FieldInfo ColumnObject::Subcolumn::get_subcolumn_field_info() const{
FieldInfo ColumnObject::Subcolumn::get_subcolumn_field_info() {

return std::regex_match(path.get_path(), COLUMN_NAME_REGEX);
}

void ColumnObject::Subcolumn::wrapp_array_nullable() {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: method 'wrapp_array_nullable' can be made static [readability-convert-member-functions-to-static]

be/src/vec/columns/column_object.h:190:

- void wrapp_array_nullable();+ static void wrapp_array_nullable();

finalize(true);
}

void ColumnObject::ensure_root_node_type(const DataTypePtr& expected_root_type) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: method 'ensure_root_node_type' can be made static [readability-convert-member-functions-to-static]

be/src/vec/columns/column_object.h:257:

- void ensure_root_node_type(const DataTypePtr& type);+ static void ensure_root_node_type(const DataTypePtr& type);

@doris-robot

Copy link
Copy Markdown

TeamCity be ut coverage result:
Function Coverage: 36.74% (8305/22607)
Line Coverage: 28.94% (66656/230322)
Region Coverage: 27.52% (34651/125893)
Branch Coverage: 24.20% (17601/72722)
Coverage Report: http://coverage.selectdb-in.cc/coverage/94b38ae9a01034d1c4ca6691cedac92f3472e016_94b38ae9a01034d1c4ca6691cedac92f3472e016/report/index.html

@doris-robot

Copy link
Copy Markdown

(From new machine)TeamCity pipeline, clickbench performance test result:
the sum of best hot time: 46.67 seconds
stream load tsv: 569 seconds loaded 74807831229 Bytes, about 125 MB/s
stream load json: 20 seconds loaded 2358488459 Bytes, about 112 MB/s
stream load orc: 64 seconds loaded 1101869774 Bytes, about 16 MB/s
stream load parquet: 32 seconds loaded 861443392 Bytes, about 25 MB/s
insert into select: 29.0 seconds inserted 10000000 Rows, about 344K ops/s
storage size: 17162797382 Bytes

@github-actionsgithub-actionsBot added the approved Indicates a PR has been approved by one committer. label Nov 7, 2023
@github-actions

Copy link
Copy Markdown
Contributor

PR approved by at least one committer and no changes requested.

@github-actions

Copy link
Copy Markdown
Contributor

PR approved by anyone and no changes requested.

xiaokang
xiaokang previously approved these changes Nov 7, 2023

@xiaokangxiaokang left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@eldenmoon
eldenmoon removed the request for review from yiguoleiNovember 7, 2023 03:58
qidaye
qidaye previously approved these changes Nov 7, 2023

@qidayeqidaye left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Comment threadbe/src/vec/columns/column_string.h
@eldenmoon
eldenmoon dismissed stale reviews from qidaye, xiaokang, and dataroaring via f32a5b7November 7, 2023 10:17
@eldenmoon

Copy link
Copy Markdown
MemberAuthor

run buildall

@github-actionsgithub-actionsBot removed the approved Indicates a PR has been approved by one committer. label Nov 7, 2023

@github-actionsgithub-actionsBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy made some suggestions

return true;
}

void ColumnObject::merge_sparse_to_root_column() {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: method 'merge_sparse_to_root_column' can be made const [readability-make-member-function-const]

be/src/vec/columns/column_object.h:252:

- void merge_sparse_to_root_column();+ void merge_sparse_to_root_column() const;
Suggested change
voidColumnObject::merge_sparse_to_root_column() {
voidColumnObject::merge_sparse_to_root_column() const{

subcolumns.get_mutable_root()->data.get_finalized_column_ptr() = mresult->get_ptr();
}

void ColumnObject::finalize(bool ignore_sparse) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: method 'finalize' can be made const [readability-make-member-function-const]

be/src/vec/columns/column_object.h:326:

- void finalize(bool ignore_sparse);+ void finalize(bool ignore_sparse) const;
Suggested change
voidColumnObject::finalize(bool ignore_sparse) {
voidColumnObject::finalize(bool ignore_sparse) const{

finalize(true);
}

void ColumnObject::ensure_root_node_type(const DataTypePtr& expected_root_type) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: method 'ensure_root_node_type' can be made static [readability-convert-member-functions-to-static]

be/src/vec/columns/column_object.h:255:

- void ensure_root_node_type(const DataTypePtr& type);+ static void ensure_root_node_type(const DataTypePtr& type);

add_sub_column({}, std::move(column), type);
}

bool ColumnObject::is_null_root() const {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: method 'is_null_root' can be made static [readability-convert-member-functions-to-static]

be/src/vec/columns/column_object.h:264:

- bool is_null_root() const;+ static bool is_null_root() ;
Suggested change
boolColumnObject::is_null_root() const{
boolColumnObject::is_null_root() {

@doris-robot

Copy link
Copy Markdown

TeamCity be ut coverage result:
Function Coverage: 36.68% (8379/22845)
Line Coverage: 29.17% (67927/232845)
Region Coverage: 27.85% (35161/126269)
Branch Coverage: 24.67% (17979/72892)
Coverage Report: http://coverage.selectdb-in.cc/coverage/f32a5b7164427ff4a1e1db1b54e1b1c96a817ffe_f32a5b7164427ff4a1e1db1b54e1b1c96a817ffe/report/index.html

@doris-robot

Copy link
Copy Markdown

(From new machine)TeamCity pipeline, clickbench performance test result:
the sum of best hot time: 45.73 seconds
stream load tsv: 565 seconds loaded 74807831229 Bytes, about 126 MB/s
stream load json: 20 seconds loaded 2358488459 Bytes, about 112 MB/s
stream load orc: 65 seconds loaded 1101869774 Bytes, about 16 MB/s
stream load parquet: 32 seconds loaded 861443392 Bytes, about 25 MB/s
insert into select: 28.8 seconds inserted 10000000 Rows, about 347K ops/s
storage size: 17162380854 Bytes

@yiguoleiyiguolei left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@github-actionsgithub-actionsBot added the approved Indicates a PR has been approved by one committer. label Nov 8, 2023
@github-actions

Copy link
Copy Markdown
Contributor

PR approved by at least one committer and no changes requested.

@xiaokangxiaokang left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@eldenmoon

Copy link
Copy Markdown
MemberAuthor

run buildall

@doris-robot

Copy link
Copy Markdown

TeamCity be ut coverage result:
Function Coverage: 36.70% (8380/22832)
Line Coverage: 29.19% (67921/232721)
Region Coverage: 27.86% (35151/126187)
Branch Coverage: 24.68% (17976/72844)
Coverage Report: http://coverage.selectdb-in.cc/coverage/c52d069029b2906ffbb10263e6c749692d2599ca_c52d069029b2906ffbb10263e6c749692d2599ca/report/index.html

@doris-robot

Copy link
Copy Markdown

(From new machine)TeamCity pipeline, clickbench performance test result:
the sum of best hot time: 45.4 seconds
stream load tsv: 555 seconds loaded 74807831229 Bytes, about 128 MB/s
stream load json: 20 seconds loaded 2358488459 Bytes, about 112 MB/s
stream load orc: 65 seconds loaded 1101869774 Bytes, about 16 MB/s
stream load parquet: 32 seconds loaded 861443392 Bytes, about 25 MB/s
insert into select: 28.7 seconds inserted 10000000 Rows, about 348K ops/s
storage size: 17162308898 Bytes

@eldenmoon

Copy link
Copy Markdown
MemberAuthor

For some reason this PR can not pass github action.I will push another PR

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approvedIndicates a PR has been approved by one committer.reviewedvariant

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants

@eldenmoon@doris-robot@xiaokang@yiguolei@qidaye@zhangstar333@dataroaring