Skip to content

planner, executor: support broadcast join for tiflash engine.#17232

Merged
ti-srebot merged 95 commits intopingcap:masterfrom
hanfei1991:hanfei/join-merge
Jul 27, 2020
Merged

planner, executor: support broadcast join for tiflash engine.#17232
ti-srebot merged 95 commits intopingcap:masterfrom
hanfei1991:hanfei/join-merge

Conversation

@hanfei1991
Copy link
Member

What problem does this PR solve?

Support Broadcast Join for TiFlash Engine.

What is changed and how it works?

Change Details and Description can be referred from : https://docs.google.com/document/d/1RXW4kEKhxVS1hRsKA9Vlr-Mn94X8dMNexbuiJqfrt4A/edit?usp=sharing

Related changes

Tipb and parser is also changed:

Check List

Side effects

  • Breaking backward compatibility

TiFlash must also be updated before update for TiDB

Release note

  • Support broadcast join for tiflash.

@pingcap pingcap deleted a comment from hanfei1991 Jul 20, 2020
"StreamAgg_32 1.00 root funcs:count(Column#14)->Column#11",
"└─TableReader_33 1.00 root data:StreamAgg_13",
" └─StreamAgg_13 1.00 cop[tiflash] funcs:count(1)->Column#14",
" └─TypeBroadcastJoin_31 8.00 cop[tiflash] ",
Copy link
Contributor

Choose a reason for hiding this comment

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

TypeBroadCastJoin -> BroadCastJoin ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

IsDNFCond bool

// IsGlobalRead indicates whether this path is a remote read path for tiflash
IsGlobalRead bool
Copy link
Contributor

Choose a reason for hiding this comment

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

IsGlobalRead is not obvious, it is not related to tiflash.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done, changed to IsTiFlashGlobalRead

return value, ErrWrongValueForVar.GenWithStackByArgs(name, value)
case TiDBOptBCJ:
if (strings.EqualFold(value, "ON") || value == "1") && vars.AllowBatchCop == 0 {
return value, ErrWrongValueForVar.GenWithStackByArgs("Can't set BCJ to 1 but tidb_allow_batch_cop is 0, please active batch cop at first.")
Copy link
Contributor

Choose a reason for hiding this comment

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

BCJ -> BroadCastJoin is more direct.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

return value, ErrWrongTypeForVar.GenWithStackByArgs(name)
}
if v == 0 && vars.AllowBCJ {
return value, ErrWrongValueForVar.GenWithStackByArgs("Can't set batch cop 0 but tidb_opt_broadcast_join is 1, please set bcj 0 at first")
Copy link
Contributor

Choose a reason for hiding this comment

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

bcj ->

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

// TypeHashJoin is the type of hash join.
TypeHashJoin = "HashJoin"
// TypeBroadcastJoin is the type of broad cast join.
TypeBroadcastJoin = "TypeBroadcastJoin"
Copy link
Contributor

Choose a reason for hiding this comment

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

TypeBroadcastJoin = "BroadcastJoin"

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

Comment on lines +1561 to +1565
if prop.TaskTp == property.CopTiFlashGlobalReadTaskType {
childrenReqProps[1-preferredGlobalIndex] = &property.PhysicalProperty{TaskTp: property.CopTiFlashGlobalReadTaskType, ExpectedCnt: math.MaxFloat64}
} else {
childrenReqProps[1-preferredGlobalIndex] = &property.PhysicalProperty{TaskTp: property.CopTiFlashLocalReadTaskType, ExpectedCnt: math.MaxFloat64}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This if is not necessary depending on the prop.TaskTp would be either CopTiFlashGlobalReadTaskType or CopTiFlashLocalReadTaskType?
&property.PhysicalProperty{TaskTp: prop.TaskTP, ExpectedCnt: math.MaxFloat64} is ok?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not ok when prop.TaskTp is rootTask. When it requires rootTask, its child can return a TiFlashTask , close it and get a root task.

Comment on lines +1566 to +1569
if prop.ExpectedCnt < p.stats.RowCount {
expCntScale := prop.ExpectedCnt / p.stats.RowCount
childrenReqProps[1-baseJoin.InnerChildIdx].ExpectedCnt = p.children[1-baseJoin.InnerChildIdx].statsInfo().RowCount * expCntScale
}
Copy link
Contributor

Choose a reason for hiding this comment

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

what is the meaning of these code? can you add some comments?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's referred from

if prop.ExpectedCnt < p.stats.RowCount {

ExpectedCnt means expected returned rows before sql stops, sometimes it's different from stats.rowCount if and only if there is a limit in parent operators which makes it stop early.

Copy link
Member Author

Choose a reason for hiding this comment

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

The comments have existed in ExpectedCnt field, so it's no need to add it here

numPairs := helper.estimate()
probeCost := numPairs * sessVars.CopCPUFactor
// should divided by the concurrency in tiflash, which should be the number of core in tiflash nodes.
probeCost /= float64(sessVars.CopTiFlashConcurrencyFactor)
Copy link
Contributor

Choose a reason for hiding this comment

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

does CopTiFlashConcurrencyFactor guarentee > 0?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it's check in varsutil.go

Copy link
Contributor

@fzhedu fzhedu left a comment

Choose a reason for hiding this comment

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

LGTM after solve comments

@ti-srebot ti-srebot removed the status/LGT1 Indicates that a PR has LGTM 1. label Jul 23, 2020
Copy link
Contributor

@fzhedu fzhedu left a comment

Choose a reason for hiding this comment

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

LGTM

@ti-srebot ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Jul 25, 2020
@ti-srebot ti-srebot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Jul 27, 2020
@lzmhhh123
Copy link
Contributor

/merge

@ti-srebot ti-srebot added the status/can-merge Indicates a PR has been approved by a committer. label Jul 27, 2020
@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot ti-srebot merged commit 29178df into pingcap:master Jul 27, 2020
ti-srebot pushed a commit to ti-srebot/tidb that referenced this pull request Jul 27, 2020
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
@ti-srebot
Copy link
Contributor

cherry pick to release-4.0 in PR #18801

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

sig/execution SIG execution sig/planner SIG: Planner status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2. type/new-feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants