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.

Sign up for GitHub

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

Jump to bottom

Aggregator support prefetch #9679

Open
wants to merge 61 commits into
base: master
Choose a base branch
from
Open

Aggregator support prefetch #9679

wants to merge 61 commits into from

Conversation

guo-shaoge
Copy link
Contributor

@guo-shaoge guo-shaoge commented Nov 28, 2024 •
edited
Loading

What problem does this PR solve?

Issue Number: close #9680

Problem Summary:

What is changed and how it works?

  1. Support prefetch for HashTable. This PR mainly works for key32/key64/key128/key256, and it doesn't affect key_string/key_serialized

Benchmark

Workloads: TPCH-50G
Queries:

 # --q0 Q1-1: key_int64; distinct rate: 10M/300M; HashMap
  "select /*+ mpp_1phase_agg() */ sum(l_quantity) as csum, l_partkey from lineitem group by l_partkey having csum < 200;"
  # --q1 Q1-2: key_int64; 75M/300M; HashMp
  "select /*+ mpp_1phase_agg() */ sum(l_quantity) as csum, l_orderkey from lineitem group by  L_orderkey having csum < 1;"
  # --q2 Q1-3: key_int64; 7/300M; HashMap
  "select /*+ mpp_1phase_agg() */ sum(l_quantity), l_linenumber from lineitem group by  l_linenumber;"

  # --q3 Q2-1: one_key_strbinpadding; 2/300M; StringHashMap
  "select /*+ mpp_1phase_agg() */ sum(l_discount), l_linestatus from lineitem group by l_linestatus;"
  # --q4 Q2-2: one_key_strbinpadding; 104M/300M; StringHashMap
  "select /*+ mpp_1phase_agg() */ sum(l_discount) as csum from lineitem group by l_comment having csum > 800;"

  # --q5 Q3-1: key_serialized as group by method;  33/300M; HashMap with StringRef key
  "select /*+ mpp_1phase_agg() */ sum(l_discount), l_returnflag from lineitem group by l_returnflag, l_discount;"
  # --q6 Q3-2: key_serialized as group by method; 77M/300M; HashMap with StringRef key
  "select /*+ mpp_1phase_agg() */ sum(l_discount) as csum, l_returnflag from lineitem group by l_returnflag, l_discount, l_extendedprice having csum > 100;"


  # --q7 Q4-1: two_keys_num64_strbinpadding: 21/300M; HashMap with StringRef key
  "select /*+ mpp_1phase_agg() */ sum(l_discount) from lineitem group by l_returnflag, L_LINENUMBER;"
  # --q8 Q4-2: two_keys_num64_strbinpadding; 29.9M/300M; HashMap with StringRef key
  "select /*+ mpp_1phase_agg() */ sum(l_discount) as csum, l_partkey from lineitem group by l_returnflag, l_partkey having csum > 100;"

  # --q9 Q5-1: keys_128; 77/300M; HashMap with UInt128 key
  "select /*+ mpp_1phase_agg() */ sum(l_linenumber), l_discount from lineitem group by l_linenumber, l_discount;"
  # --q10 Q5-2: keys_128; 5M/300M; HashMap with UInt128 key
  "select /*+ mpp_1phase_agg() */ sum(l_linenumber) as csum, l_discount from lineitem group by l_suppkey, l_discount having csum > 800;"

  # --q11 Q6-1: key_string; 4/300M; StringHashMap
   "select /*+ mpp_1phase_agg() */ sum(l_quantity), l_shipinstruct from lineitem group by l_shipinstruct;"

  # --q12 Q7-1: keys_256; 290M/300M; HashMap;
  "select /*+ mpp_1phase_agg() */ sum(l_suppkey) as csum from lineitem group by l_suppkey, l_tax, l_discount, l_partkey having csum = 10;"

  # --q13 clickbench Q4
  "select count(distinct userid) from clickbench.hits;"

ENV:

  1. Internal benchbot env
  2. 1 tidb; 2 tikv; 1 PD; 2 tiflash(16C64G, amd64)

Results (google excel):

idx Query Name nightly opt-1 opt-2 rate-opt-1 rate-opt-2
0 Q1-1 2.53 1.95 1.79 22.92% 29.25%
1 Q1-2 1.98 1.93 1.56 2.53% 21.21%
2 Q1-3 1.38 1.32 1.4 4.35% -1.45%
9 Q5-1 1.31 1.28 1.31 2.29% 0.00%
10 Q5-2 3.81 3.57 3.31 6.30% 13.12%
12 Q7-1 11.8 11 9.52 6.78% 19.32%
13 clickbench Q4 0.456 0.411 0.367 9.87% 19.52%

NOTE:

  1. infos:
    1.1 Query duration is in seconds
    1.2 baseline: nightly-b16a5f93; rate-opt1: (baseline - opt1)/baseline ; rate-opt2: (baseline - opt2)/baseline
  2. Queries like Q2-x/Q3-x are not listed because they are key_stirng/key_serialized, this PR only works for key_int64/key_int128/key_int256...). So these queries should not be affected.
  3. opt-1 means only prefetch HashTable.
  4. opt-2 means(this is the final improvement of this PR)
    1. prefetch HashTable
    2. prefetch threshold be 2MB in byte instead of 8192 buckets
    3. prefetch when insertResultInto columns
    4. prefetch aggregation data for already exists key in HashTable
    5. mini batch(step is 256 for now) when emplace HashTable
  5. ClickBench Q4 is from: SELECT COUNT(DISTINCT UserID) FROM hits; https://github.com/ClickHouse/ClickBench/blob/main/mysql/queries.sql

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

  • Affects user behaviors
  • Contains syntax changes
  • Contains variable changes
  • Contains experimental features
  • Changes MySQL compatibility

Release note

None

@guo-shaoge
new hash
b869b2f 
Signed-off-by: guo-shaoge <[email protected]>
@guo-shaoge
prefetch done
e8a2df8 
Signed-off-by: guo-shaoge <[email protected]>
@guo-shaoge
executeImplBatchStringHashMap done
b314166 
Signed-off-by: guo-shaoge <[email protected]>
@guo-shaoge
handle resize exception done
ec6e892 
Signed-off-by: guo-shaoge <[email protected]>
@guo-shaoge
tmp save
9dc702d 
Signed-off-by: guo-shaoge <[email protected]>
@guo-shaoge
revert Serialized Key changes
ce1f767 
Signed-off-by: guo-shaoge <[email protected]>
@ti-chi-bot ti-chi-bot bot added do-not-merge/needs-linked-issue release-note-none Denotes a PR that doesn't merit a release note. labels Nov 28, 2024
@ti-chi-bot ti-chi-bot
Copy link
Contributor

ti-chi-bot bot commented Nov 28, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from guo-shaoge, ensuring that each of them provides their approval before proceeding. For more information see the Code Review Process.

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

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ti-chi-bot ti-chi-bot bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed do-not-merge/needs-linked-issue labels Nov 28, 2024
@guo-shaoge
refine
8ac8beb 
Signed-off-by: guo-shaoge <[email protected]>
@guo-shaoge
refine
0053ce8 
Signed-off-by: guo-shaoge <[email protected]>
@guo-shaoge
fix unit test
fcf8ed2 
Signed-off-by: guo-shaoge <[email protected]>
@guo-shaoge
refine
ae7b969 
Signed-off-by: guo-shaoge <[email protected]>
@guo-shaoge
unit test
3a86617 
Signed-off-by: guo-shaoge <[email protected]>
@guo-shaoge guo-shaoge changed the title Aggregator support prefetch Aggregator support prefetch and new hasher Dec 2, 2024
@guo-shaoge
prefetch
623fef5 
Signed-off-by: guo-shaoge <[email protected]>
@guo-shaoge
fix
19f320d 
Signed-off-by: guo-shaoge <[email protected]>
@guo-shaoge
Copy link
Contributor Author

/retest

@guo-shaoge
refine
3a226df 
Signed-off-by: guo-shaoge <[email protected]>
@guo-shaoge
refine
c44ace7 
Signed-off-by: guo-shaoge <[email protected]>
@guo-shaoge
revert new hasher
3e30f95 
Signed-off-by: guo-shaoge <[email protected]>
@guo-shaoge
debug low distinct value
ea85d19 
Signed-off-by: guo-shaoge <[email protected]>
@guo-shaoge
Revert "revert new hasher"
16937ff 
This reverts commit 3e30f95.
@guo-shaoge
refine original code path
d2fba57 
Signed-off-by: guo-shaoge <[email protected]>
@guo-shaoge
hashvals sizeof 16
ab31beb 
Signed-off-by: guo-shaoge <[email protected]>
gengliqi
gengliqi reviewed Dec 26, 2024
View reviewed changes
dbms/src/Interpreters/Aggregator.cpp Outdated Show resolved Hide resolved
dbms/src/Interpreters/Aggregator.cpp Outdated Show resolved Hide resolved
@guo-shaoge
fix case
00cc20d 
Signed-off-by: guo-shaoge <[email protected]>
@guo-shaoge
remove macro
4aec90b 
Signed-off-by: guo-shaoge <[email protected]>
@guo-shaoge
remove macro 2
606e013 
Signed-off-by: guo-shaoge <[email protected]>
@guo-shaoge guo-shaoge requested a review from gengliqi December 27, 2024 04:11
@guo-shaoge
fix
a7361e1 
Signed-off-by: guo-shaoge <[email protected]>
@guo-shaoge
refine comment
bdb21d3 
Signed-off-by: guo-shaoge <[email protected]>
@guo-shaoge
refine
4153592 
Signed-off-by: guo-shaoge <[email protected]>
@guo-shaoge
vec 16
bbd5c45 
Signed-off-by: guo-shaoge <[email protected]>
@guo-shaoge
refine
6627558 
Signed-off-by: guo-shaoge <[email protected]>
@guo-shaoge
compute hashvals before each mini batch
530bb27 
Signed-off-by: guo-shaoge <[email protected]>
@guo-shaoge
handleMiniBatchImpl
358e851 
Signed-off-by: guo-shaoge <[email protected]>
@guo-shaoge
fmt
3d79a20 
Signed-off-by: guo-shaoge <[email protected]>
@guo-shaoge
remove useless code
51a90b5 
Signed-off-by: guo-shaoge <[email protected]>
@guo-shaoge
minor refine
e108584 
Signed-off-by: guo-shaoge <[email protected]>
@guo-shaoge
fmt
761b34d 
Signed-off-by: guo-shaoge <[email protected]>
gengliqi
gengliqi reviewed Jan 8, 2025
View reviewed changes
dbms/src/Interpreters/Aggregator.cpp Show resolved Hide resolved
dbms/src/Common/ColumnsHashingImpl.h
Comment on lines +62 to +64
Mapped * value = nullptr;
Mapped * cached_value = nullptr;
bool inserted = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

What specific issue does this change address?

Copy link
Contributor Author

@guo-shaoge guo-shaoge Jan 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

emplace_result_holder (std::optional< EmplaceResult >) needs copy assign operator

@guo-shaoge guo-shaoge requested a review from gengliqi January 9, 2025 09:46
windtalker
windtalker reviewed Jan 10, 2025
View reviewed changes
dbms/src/Interpreters/Aggregator.cpp Outdated Show resolved Hide resolved
dbms/src/Interpreters/Aggregator.cpp
}
else
{
emplace_result.setMapped(place);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it save to push the fake address?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's ok. no one use value if agg func is zero. Also check: https://github.com/ClickHouse/ClickHouse/blob/70945668f81a3621685edf4d21728efc1ae4a902/src/Interpreters/Aggregator.cpp#L1020

@guo-shaoge
fix
5343014 
Signed-off-by: guo-shaoge <[email protected]>
@guo-shaoge guo-shaoge requested a review from windtalker January 12, 2025 04:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gengliqi gengliqi Awaiting requested review from gengliqi

@windtalker windtalker Awaiting requested review from windtalker

Assignees
No one assigned
Labels
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.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Aggregator support prefetch
3 participants
@guo-shaoge @windtalker @gengliqi