Pipeline: support spill for fine grained aggregation #7220

SeaRise · 2023-04-05T18:18:03Z

What problem does this PR solve?

Issue Number: ref #6518

Problem Summary:

What is changed and how it works?

wait fot #7195

split Aggregator.executeOnBlock to Aggregator.executeOnBlock and Aggregator.spill in order to support cpu/io thread pool in pipeline model.
remove useless param enable_memory_tracker in ExecutorTestUtils, aggregator does not depend on memory tracker now.
add LocalAggregateRestorer to support restoring disk data for local agg.

/**
 * ┌──────────────────────────────────────────────────┐
 * │  {bucket0, bucket1, ... bucket256}spilled_file0──┼────┐
 * │  {bucket0, bucket1, ... bucket256}spilled_file1──┼────┤
 * │  {bucket0, bucket1, ... bucket256}spilled_file2──┼────┤
 * │  ...                                             │    │
 * │  {bucket0, bucket1, ... bucket256}spilled_filen──┼────┤
 * └──────────────────────────────────────────────────┘    │
 *                                                         │ loadBucketData
 *                   bucket_data◄──────────────────────────┘
 *                      │
 *                      │ tryPop
 *                      ▼
 *                restored_blocks
 */

support spill/restore in LocalAggregateTransformOp: add spill/final_spill/restore phase.
add unit test for fine grained shuffle agg in gtest_spill_aggregation.cpp.

Check List

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

Co-authored-by: yanweiqi <592838129@qq.com>

ti-chi-bot · 2023-04-05T18:18:05Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

hongyunyan
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-04-05T18:22:14Z

/run-all-tests

SeaRise · 2023-04-05T19:24:12Z

/run-all-tests

dbms/src/Operators/LocalAggregateRestorer.cpp

SeaRise · 2023-04-19T04:53:56Z

/run-unit-test

ywqzzy

Rest LGTM

dbms/src/Operators/LocalAggregateRestorer.cpp

ywqzzy · 2023-04-19T08:10:09Z

dbms/src/Operators/AggregateContext.cpp

+    assert(status.load() == AggStatus::build);
+    aggregator->finishSpill();
+    LOG_INFO(log, "Begin restore data from disk for local aggregation.");
+    auto input_streams = aggregator->restoreSpilledData();


If input_stream size == 0, we can skip creating LocalAggregateRestorer, add the operator will not switch to io pool

good point!
Since aggregator.hasSpilledData is true, input_streams must not be empty.
RUNTIME_CHECK(!input_streams.empty) added.

ywqzzy · 2023-04-19T08:11:09Z

dbms/src/Operators/LocalAggregateRestorer.cpp

+{
+    for (const auto & bucket_stream : bucket_streams)
+        bucket_inputs.emplace_back(bucket_stream);
+    if (bucket_inputs.empty())


Is is possible?

removed and added assert(!bucket_inputs.empty());

xzhangxian1008 · 2023-04-19T10:13:49Z

dbms/src/Operators/LocalAggregateTransform.cpp

-            return OperatorStatus::HAS_OUTPUT;
+            return agg_context.hasSpilledData()
+                ? fromBuildToFinalSpillOrRestore()
+                : fromBuildToConvergent(block);
        }
        agg_context.buildOnBlock(task_index, block);
        block.clear();


Why we need to clear the block?

Just to release the memory earlier, the block written to the sink will not be used anymore

xzhangxian1008 · 2023-04-19T10:44:29Z

dbms/src/Flash/Planner/Plans/PhysicalAggregationConvergent.cpp

-    if (unlikely(aggregate_context->useNullSource()))
-    {
-        group_builder.init(1);
-        group_builder.transform([&](auto & builder) {
-            builder.setSourceOp(std::make_unique<NullSourceOp>(
-                exec_status,
-                aggregate_context->getHeader(),
-                log->identifier()));
-        });
-    }


Why we need to delete these codes?

The logic of AggContext::useNullSource is wrapped into AggContext::getConvergentConcurrency and AggContext::readForConvergent, just to simplify the code.

xzhangxian1008

LGTM

ti-chi-bot · 2023-04-20T01:47:40Z

@xzhangxian1008: Thanks for your review. The bot only counts approvals from reviewers and higher roles in list, but you're still welcome to leave your comments.

In response to this:

LGTM

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.

SeaRise · 2023-04-20T03:05:36Z

/merge

ti-chi-bot · 2023-04-20T03:05:37Z

@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-04-20T03:05:39Z

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

Commit hash: 3d49d23

SeaRise · 2023-04-20T03:31:51Z

/merge

ti-chi-bot · 2023-04-20T03:31:53Z

@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-04-20T03:31:54Z

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

Commit hash: 7b0b566

SeaRise and others added 17 commits March 27, 2023 15:21

pre modify

41b44d3

update

96c4b01

fix ut

4d39f1d

update

7f73b05

fix comment

bc17c52

add more comment

0398d35

use executeIO

67a7e46

fix tidy

ea6b4b3

Merge branch 'master' into support_io_cpu_thread_pool

7ae7753

Update dbms/src/Flash/Pipeline/Exec/PipelineExec.cpp

bdc0e6f

Co-authored-by: yanweiqi <592838129@qq.com>

pre refine

00ec7a0

refine

c29f0a6

u

07f7f5d

update

62972fa

add more comments

8105719

add ut and fix

4b1aeb0

u

b676835

ti-chi-bot added do-not-merge/needs-linked-issue release-note-none Denotes a PR that doesn't merit a release note. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Apr 5, 2023

SeaRise changed the title ~~Pipeline: support spill for non fine grained aggregate~~ WIP: Pipeline: support spill for non fine grained aggregate Apr 5, 2023

ti-chi-bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. and removed do-not-merge/needs-linked-issue labels Apr 5, 2023

SeaRise mentioned this pull request Apr 5, 2023

Support pipeline model #6518

Closed

25 tasks

SeaRise added 2 commits April 6, 2023 03:21

Merge branch 'master' into pipeline_support_spill_agg

4985799

u

787467e

u

90f4842

ywqzzy reviewed Apr 18, 2023

View reviewed changes

dbms/src/Operators/LocalAggregateRestorer.cpp Outdated Show resolved Hide resolved

address comments

3753242

SeaRise requested a review from ywqzzy April 18, 2023 09:22

SeaRise mentioned this pull request Apr 19, 2023

Pipeline: support non fine grained spill agg #7318

Merged

12 tasks

add more comments

36be5a0

ywqzzy approved these changes Apr 19, 2023

View reviewed changes

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

check not empty

9f042ef

xzhangxian1008 reviewed Apr 19, 2023

View reviewed changes

SeaRise requested a review from xzhangxian1008 April 19, 2023 10:52

xzhangxian1008 approved these changes Apr 20, 2023

View reviewed changes

port from pingcap#7318

68bacca

hongyunyan approved these changes Apr 20, 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 Apr 20, 2023

Lloyd-Pottiger approved these changes Apr 20, 2023

View reviewed changes

Merge branch 'master' into pipeline_support_spill_agg

3d49d23

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

fix clang tidy

7b0b566

ti-chi-bot removed the status/can-merge Indicates a PR has been approved by a committer. label Apr 20, 2023

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

ti-chi-bot merged commit 3ef3154 into pingcap:master Apr 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pipeline: support spill for fine grained aggregation #7220

Pipeline: support spill for fine grained aggregation #7220

SeaRise commented Apr 5, 2023 •

edited

Loading

ti-chi-bot commented Apr 5, 2023 •

edited

Loading

SeaRise commented Apr 5, 2023

SeaRise commented Apr 5, 2023

SeaRise commented Apr 19, 2023

ywqzzy left a comment

ywqzzy Apr 19, 2023

SeaRise Apr 19, 2023

ywqzzy Apr 19, 2023

SeaRise Apr 19, 2023

xzhangxian1008 Apr 19, 2023

SeaRise Apr 19, 2023

xzhangxian1008 Apr 19, 2023

SeaRise Apr 19, 2023

xzhangxian1008 left a comment

ti-chi-bot commented Apr 20, 2023

SeaRise commented Apr 20, 2023

ti-chi-bot commented Apr 20, 2023

ti-chi-bot commented Apr 20, 2023

SeaRise commented Apr 20, 2023

ti-chi-bot commented Apr 20, 2023

ti-chi-bot commented Apr 20, 2023

Pipeline: support spill for fine grained aggregation #7220

Pipeline: support spill for fine grained aggregation #7220

Conversation

SeaRise commented Apr 5, 2023 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot commented Apr 5, 2023 • edited Loading

SeaRise commented Apr 5, 2023

SeaRise commented Apr 5, 2023

SeaRise commented Apr 19, 2023

ywqzzy left a 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

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

xzhangxian1008 left a comment

Choose a reason for hiding this comment

ti-chi-bot commented Apr 20, 2023

SeaRise commented Apr 20, 2023

ti-chi-bot commented Apr 20, 2023

ti-chi-bot commented Apr 20, 2023

SeaRise commented Apr 20, 2023

ti-chi-bot commented Apr 20, 2023

ti-chi-bot commented Apr 20, 2023

SeaRise commented Apr 5, 2023 •

edited

Loading

ti-chi-bot commented Apr 5, 2023 •

edited

Loading