Skip to content
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

[FIX](complex-type) fix agg table with complex type with replace state #24873

Merged

Conversation

amorynan
Copy link
Contributor

Proposed changes

Issue Number: close #xxx
when we use replace agg state which we only supported in complex type, fix be not implement replace_column_data with agg value to iterator . so we will got a be core below this

F20230924 10:40:22.801700 1080720 column_array.h:234] replace_column_data_default not implemented
*** Check failure stack trace: ***
    @     0x564542ab01e6  google::LogMessage::SendToLog()
    @     0x564542aac7b0  google::LogMessage::Flush()
    @     0x564542ab0a29  google::LogMessageFatal::~LogMessageFatal()
    @     0x56452f2d911a  doris::vectorized::ColumnArray::replace_column_data_default()
    @     0x56452f41fc76  doris::vectorized::ColumnNullable::replace_column_data()
    @     0x5645416c26b3  doris::vectorized::VerticalBlockReader::_copy_agg_data()
    @     0x5645416c1725  doris::vectorized::VerticalBlockReader::_update_agg_data()
    @     0x5645416c15f6  doris::vectorized::VerticalBlockReader::_append_agg_data()
    @     0x5645416c0b27  doris::vectorized::VerticalBlockReader::_agg_key_next_block()
    @     0x564520b3dce7  doris::vectorized::VerticalBlockReader::next_block_with_aggregation()
    @     0x564520b2d28b  doris::Merger::vertical_compact_one_group()
    @     0x564520b3054a  doris::Merger::vertical_merge_rowsets()
    @     0x564520b4e5e9  doris::Compaction::do_compaction_impl()
    @     0x564520b4c03f  doris::Compaction::do_compaction()
    @     0x564520ce7947  doris::CumulativeCompaction::execute_compact_impl()
    @     0x564520b4b77b  doris::Compaction::execute_compact()
    @     0x564520de8bd5  doris::Tablet::execute_compaction()
    @     0x564520851d76  doris::StorageEngine::_submit_compaction_task()::$_0::operator()()
    @     0x564520851bf5  std::__invoke_impl<>()
    @     0x564520851b95  _ZSt10__invoke_rIvRZN5doris13StorageEngine23_submit_compaction_taskESt10shared_ptrINS0_6TabletEENS0_14CompactionTypeEbE3$_0JEENSt9enable_ifIX16is_invocable_r_vIT_T0_DpT1_EES9_E4typeEOSA_DpOSB_
    @     0x5645208519ad  std::_Function_handler<>::_M_invoke()
    @     0x56451ea937a3  std::function<>::operator()()
    @     0x564521e63279  doris::FunctionRunnable::run()
    @     0x564521e4f01e  doris::ThreadPool::dispatch_thread()
    @     0x564521e759f4  std::__invoke_impl<>()
    @     0x564521e758cd  std::__invoke<>()
    @     0x564521e75855  _ZNSt5_BindIFMN5doris10ThreadPoolEFvvEPS1_EE6__callIvJEJLm0EEEET_OSt5tupleIJDpT0_EESt12_Index_tupleIJXspT1_EEE
    @     0x564521e756fe  std::_Bind<>::operator()<>()
    @     0x564521e75615  std::__invoke_impl<>()
    @     0x564521e755b5  _ZSt10__invoke_rIvRSt5_BindIFMN5doris10ThreadPoolEFvvEPS2_EEJEENSt9enable_ifIX16is_invocable_r_vIT_T0_DpT1_EESA_E4typeEOSB_DpOSC_
    @     0x564521e752dd  std::_Function_handler<>::_M_invoke()
    @     0x56451ea937a3  std::function<>::operator()()

Further comments

If this is a relatively large or complex change, kick off the discussion at [email protected] by explaining why you chose the solution you did and what alternatives you considered, etc...

Copy link
Contributor

@github-actions github-actions bot left a comment

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

@@ -227,12 +227,32 @@ class ColumnArray final : public COWHelper<IColumn, ColumnArray> {
void insert_indices_from(const IColumn& src, const int* indices_begin,
const int* indices_end) override;

void replace_column_data(const IColumn&, size_t row, size_t self_row = 0) override {
LOG(FATAL) << "replace_column_data not implemented";
void replace_column_data(const IColumn& rhs, size_t row, size_t self_row = 0) override {

This comment was marked as outdated.

@@ -140,11 +140,34 @@ class ColumnMap final : public COWHelper<IColumn, ColumnMap> {
return append_data_by_selector_impl<ColumnMap>(res, selector);
}

void replace_column_data(const IColumn&, size_t row, size_t self_row = 0) override {
LOG(FATAL) << "replace_column_data not implemented";
void replace_column_data(const IColumn& rhs, size_t row, size_t self_row = 0) override {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Suggested change
void replace_column_data(const IColumn& rhs, size_t row, size_t self_row = 0) override {
void replace_column_data(const IColumn& rhs, size_t row, size_t self_row = 0) const override {

@@ -357,6 +357,25 @@ class ColumnNullable final : public COWHelper<IColumn, ColumnNullable> {
}
}

void replace_batch_column_data(const IColumn& rhs, size_t num_rows, size_t row,
Copy link
Contributor

Choose a reason for hiding this comment

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

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

be/src/vec/columns/column_nullable.h:360:

-                                    size_t self_row = 0) override {
+                                    size_t self_row = 0) const override {

@@ -130,11 +131,31 @@ class ColumnStruct final : public COWHelper<IColumn, ColumnStruct> {
void append_data_by_selector(MutableColumnPtr& res, const Selector& selector) const override {
return append_data_by_selector_impl<ColumnStruct>(res, selector);
}
void replace_column_data(const IColumn&, size_t row, size_t self_row = 0) override {
LOG(FATAL) << "replace_column_data not implemented";
void replace_column_data(const IColumn& rhs, size_t row, size_t self_row = 0) override {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Suggested change
void replace_column_data(const IColumn& rhs, size_t row, size_t self_row = 0) override {
void replace_column_data(const IColumn& rhs, size_t row, size_t self_row = 0) const override {

}

void replace_batch_column_data(const IColumn& rhs, size_t num_rows, size_t row,
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

-                                    size_t self_row = 0) override {
+                                    size_t self_row = 0) const override {

BiteTheDDDDt
BiteTheDDDDt previously approved these changes Sep 25, 2023
@github-actions github-actions bot added the approved Indicates a PR has been approved by one committer. label Sep 25, 2023
@github-actions
Copy link
Contributor

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

@github-actions
Copy link
Contributor

PR approved by anyone and no changes requested.

@amorynan
Copy link
Contributor Author

run buildall

@doris-robot
Copy link

TeamCity be ut coverage result:
Function Coverage: 36.29% (8111/22349)
Line Coverage: 28.42% (64870/228255)
Region Coverage: 27.31% (33641/123182)
Branch Coverage: 23.97% (17172/71648)
Coverage Report: http://coverage.selectdb-in.cc/coverage/a71b0bb270d0764b27954817c14cc21917a3932a_a71b0bb270d0764b27954817c14cc21917a3932a/report/index.html

@doris-robot
Copy link

(From new machine)TeamCity pipeline, clickbench performance test result:
the sum of best hot time: 45.3 seconds
stream load tsv: 554 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: 31 seconds loaded 861443392 Bytes, about 26 MB/s
insert into select: 28.7 seconds inserted 10000000 Rows, about 348K ops/s
storage size: 17162332101 Bytes

@github-actions github-actions bot removed the approved Indicates a PR has been approved by one committer. label Sep 28, 2023
@amorynan
Copy link
Contributor Author

run buildall

@doris-robot
Copy link

TeamCity be ut coverage result:
Function Coverage: 36.30% (8145/22436)
Line Coverage: 28.45% (65138/228986)
Region Coverage: 27.38% (33743/123243)
Branch Coverage: 24.04% (17213/71614)
Coverage Report: http://coverage.selectdb-in.cc/coverage/7a5c4452f52c7452b0f19e0b100616882ac5c233_7a5c4452f52c7452b0f19e0b100616882ac5c233/report/index.html

@doris-robot
Copy link

(From new machine)TeamCity pipeline, clickbench performance test result:
the sum of best hot time: 45.95 seconds
stream load tsv: 564 seconds loaded 74807831229 Bytes, about 126 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: 33 seconds loaded 861443392 Bytes, about 24 MB/s
insert into select: 29.0 seconds inserted 10000000 Rows, about 344K ops/s
storage size: 17162344204 Bytes

Copy link
Contributor

@HappenLee HappenLee left a comment

Choose a reason for hiding this comment

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

LGTM

@github-actions github-actions bot added the approved Indicates a PR has been approved by one committer. label Sep 28, 2023
@github-actions
Copy link
Contributor

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

@amorynan
Copy link
Contributor Author

run p1

@BiteTheDDDDt BiteTheDDDDt merged commit 10f0c63 into apache:master Oct 3, 2023
16 of 18 checks passed
vinlee19 pushed a commit to vinlee19/doris that referenced this pull request Oct 7, 2023
xiaokang pushed a commit that referenced this pull request Oct 7, 2023
#24873)

fix agg table with complex type with replace state
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by one committer. dev/2.0.3-merged reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants