Refine merge agg stream #6793

SeaRise · 2023-02-09T08:14:08Z

What problem does this PR solve?

Issue Number: ref #5900

Problem Summary:

What is changed and how it works?

Remove the constraint on two level merge: If the aggregation states are two-level, then it produces blocks strictly in order of 'bucket_num'., because tiflash does not need this.
Move most of one level and two level merge's logic from MergingAndConvertingBlockInputStream to MergingBuckets and don't create separate threads to execute two level merge.
Now MergingAndConvertingBlockInputStream only run in one thread, and use UnionBlockInputStream to do parallel two level merge.
Move MergingBuckets to Aggregator.h because of the c++ compile error...
Remove some useless code in Aggregator.h and Aggregator.cpp and remove useless class MergingAggregatedBlockInputStream

Check List

Tests

tsan unit tests pass.

[----------] 1 test from SpillAggregationTestRunner (301567 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test case ran. (301570 ms total)
[  PASSED  ] 1 test.

[----------] 9 tests from AggExecutorTestRunner (58719 ms total)

[----------] Global test environment tear-down
[==========] 9 tests from 1 test case ran. (58722 ms total)
[  PASSED  ] 9 tests.

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No code

Side effects

Performance regression: Consumes more CPU
Performance regression: Consumes more Memory
Breaking backward compatibility

Documentation

Release note

None

ti-chi-bot · 2023-02-09T08:14:10Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

windtalker
ywqzzy

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

SeaRise · 2023-02-09T11:08:45Z

/run-all-tests

SeaRise · 2023-02-09T11:16:21Z

/run-unit-test

SeaRise · 2023-02-09T15:43:35Z

/run-unit-test

SeaRise · 2023-02-09T16:02:34Z

tsan unit tests pass.

[----------] 1 test from SpillAggregationTestRunner (301567 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test case ran. (301570 ms total)
[  PASSED  ] 1 test.

[----------] 9 tests from AggExecutorTestRunner (58719 ms total)

[----------] Global test environment tear-down
[==========] 9 tests from 1 test case ran. (58722 ms total)
[  PASSED  ] 9 tests.

SeaRise · 2023-02-09T16:02:48Z

/run-all-tests

SeaRise · 2023-02-09T23:12:30Z

/run-all-tests

SeaRise · 2023-02-10T02:28:04Z

/cc @windtalker @ywqzzy

windtalker · 2023-02-13T06:29:25Z

dbms/src/Interpreters/Aggregator.cpp

+        throw Exception("Unknown aggregated data variant.", ErrorCodes::UNKNOWN_AGGREGATED_DATA_VARIANT);
+    }
+#undef M
+    single_level_blocks = aggregator.prepareBlocksAndFillSingleLevel(*first, final);


Why not move the merge code to L2201, together with the without_key case?

yes, you are right, updated.

ywqzzy · 2023-02-13T06:01:50Z

dbms/src/Interpreters/Aggregator.h

+    const LoggerPtr log;
+    const Aggregator & aggregator;
+    ManyAggregatedDataVariants data;
+    bool final;


Rename it since it conflicts with the “final” keyword

You are right, but I don't want to rename it in this pr..
Since final is used in many places such Aggregator, AggregatingBlockInputStream, and MergingAggregatedMemoryEfficientBlockInputStream and etc, it might be better to do it with another pr..

ywqzzy · 2023-02-13T06:59:06Z

dbms/src/DataStreams/MergingAndConvertingBlockInputStream.h

-
-    void thread(Int32 bucket_num)
-    {
-        try


How to handle the exception？

UnionBlockInputStream or other outside blockInputStreams will handle it.

ywqzzy · 2023-02-13T06:59:57Z

dbms/src/DataStreams/ParallelAggregatingBlockInputStream.cpp

+                    BlockInputStreams merging_streams;
+                    for (size_t i = 0; i < merging_buckets->getConcurrency(); ++i)
+                        merging_streams.push_back(std::make_shared<MergingAndConvertingBlockInputStream>(merging_buckets, i, log->identifier()));
+                    impl = std::make_unique<UnionBlockInputStream<>>(merging_streams, BlockInputStreams{}, max_threads, log->identifier());


How about moving the unionBlockInputStream construction outside the readImpl function？

I don't think it's possible because Impl needs to be constructed inside readImpl.

dbms/src/Interpreters/Aggregator.cpp

windtalker

Others LGTM

windtalker · 2023-02-14T05:11:23Z

dbms/src/Interpreters/Aggregator.cpp

+    {
+        APPLY_FOR_VARIANTS_TWO_LEVEL(M)
+    default:
+        break;


Why not throw error for the default branch?

ok, updated.

SeaRise · 2023-02-14T06:37:16Z

/merge

ti-chi-bot · 2023-02-14T06:37:17Z

@SeaRise: It seems you want to merge this PR, I will help you trigger all the tests:

/run-all-tests

You only need to trigger /merge once, and if the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

If you have any questions about the PR merge process, please refer to pr process.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

ti-chi-bot · 2023-02-14T06:37:19Z

This pull request has been accepted and is ready to merge.

Commit hash: 4d694bb

SeaRise added 4 commits February 9, 2023 14:18

tmp

3a310eb

fmt

971a832

wait for fix build fail

fffa69c

wait build fail

3715468

fix the ... c++ complie error

eef2c15

ti-chi-bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Feb 9, 2023

add some comment

05ff4b8

SeaRise changed the title ~~WIP: Refine merge agg stream~~ Refine merge agg stream Feb 9, 2023

ti-chi-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 9, 2023

Merge branch 'master' into refine_merge_agg_stream

0b79f6b

ti-chi-bot removed the do-not-merge/needs-linked-issue label Feb 9, 2023

SeaRise added 2 commits February 9, 2023 17:51

add empty test

0571d36

remove uesless code

b1cdc52

ti-chi-bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Feb 9, 2023

u

eff4a7e

add test case

b2b8a04

Merge branch 'master' into refine_merge_agg_stream

7191998

SeaRise added 2 commits February 10, 2023 10:11

fmt

696b045

u

5715d9c

ti-chi-bot requested review from windtalker and ywqzzy February 10, 2023 02:28

windtalker reviewed Feb 13, 2023

View reviewed changes

ywqzzy reviewed Feb 13, 2023

View reviewed changes

dbms/src/Interpreters/Aggregator.cpp Show resolved Hide resolved

address comments

a1089d2

ywqzzy self-requested a review February 13, 2023 07:12

SeaRise requested a review from windtalker February 13, 2023 07:26

Merge branch 'master' into refine_merge_agg_stream

3197a46

windtalker approved these changes Feb 14, 2023

View reviewed changes

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Feb 14, 2023

address comment

4d694bb

ywqzzy approved these changes Feb 14, 2023

View reviewed changes

ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Feb 14, 2023

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Feb 14, 2023

Merge branch 'master' into refine_merge_agg_stream

f5982eb

ti-chi-bot merged commit 34fc70f into pingcap:master Feb 14, 2023

SeaRise deleted the refine_merge_agg_stream branch February 14, 2023 07:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refine merge agg stream #6793

Refine merge agg stream #6793

SeaRise commented Feb 9, 2023 •

edited

Loading

ti-chi-bot commented Feb 9, 2023 •

edited

Loading

SeaRise commented Feb 9, 2023

SeaRise commented Feb 9, 2023

SeaRise commented Feb 9, 2023

SeaRise commented Feb 9, 2023

SeaRise commented Feb 9, 2023

SeaRise commented Feb 9, 2023

SeaRise commented Feb 10, 2023

windtalker Feb 13, 2023

SeaRise Feb 13, 2023

ywqzzy Feb 13, 2023

SeaRise Feb 13, 2023 •

edited

Loading

ywqzzy Feb 13, 2023

SeaRise Feb 13, 2023

ywqzzy Feb 13, 2023

SeaRise Feb 13, 2023

windtalker left a comment

windtalker Feb 14, 2023

SeaRise Feb 14, 2023

SeaRise commented Feb 14, 2023

ti-chi-bot commented Feb 14, 2023

ti-chi-bot commented Feb 14, 2023

Refine merge agg stream #6793

Refine merge agg stream #6793

Conversation

SeaRise commented Feb 9, 2023 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot commented Feb 9, 2023 • edited Loading

SeaRise commented Feb 9, 2023

SeaRise commented Feb 9, 2023

SeaRise commented Feb 9, 2023

SeaRise commented Feb 9, 2023

SeaRise commented Feb 9, 2023

SeaRise commented Feb 9, 2023

SeaRise commented Feb 10, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SeaRise Feb 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

windtalker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SeaRise commented Feb 14, 2023

ti-chi-bot commented Feb 14, 2023

ti-chi-bot commented Feb 14, 2023

SeaRise commented Feb 9, 2023 •

edited

Loading

ti-chi-bot commented Feb 9, 2023 •

edited

Loading

SeaRise Feb 13, 2023 •

edited

Loading