-
Notifications
You must be signed in to change notification settings - Fork 5.7k
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
planner: consider disk cost in hashJoin #13246
Conversation
ff45c26
to
e8ae3ac
Compare
9711361
to
3a86f16
Compare
Codecov Report
@@ Coverage Diff @@
## master #13246 +/- ##
===========================================
Coverage 80.3773% 80.3773%
===========================================
Files 474 474
Lines 118353 118353
===========================================
Hits 95129 95129
Misses 15804 15804
Partials 7420 7420 |
81ecec9
to
57c889d
Compare
a3bb299
to
987d86f
Compare
/run-all-tests |
@eurekaka, @qw4990, @XuHuaiyu, @lzmhhh123, PTAL. |
planner/core/task.go
Outdated
if (p.InnerChildIdx == 1 && !p.UseOuterToBuild) || (p.InnerChildIdx == 0 && p.UseOuterToBuild) { | ||
innerCnt, outerCnt = rCnt, lCnt | ||
inner = p.children[1] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The name inner
might be confusing here. When p.InnerChildIdx == 0 && p.UseOuterToBuild
, we would set the outer table as inner
. Can we use something like build
or stream
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renamed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
return cpuCost + memoryCost | ||
|
||
if spill { | ||
memoryCost *= float64(memQuota) / (rowSize * buildCnt) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this mean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does these code can describe the meaning?
memUsed := float64(memQuota)
originMemUsed := rowSize * buildCnt
factor := memUsed / originMemUsed
memoryCost *= factor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got you, I misunderstood it before.
I think memoryCost *= float64(memQuota) / (rowSize * buildCnt)
is ok.
LGTM |
PTAL @eurekaka |
/merge |
/run-all-tests |
@SunRunAway merge failed. |
/merge |
/run-all-tests |
@SunRunAway merge failed. |
/build |
/run-all-tests |
/merge |
Your auto merge job has been accepted, waiting for 13863, 13859 |
/run-all-tests |
/run-all-tests |
/run-unit-test |
What problem does this PR solve?
part of #11607
What is changed and how it works?
Check List
Tests