-
Notifications
You must be signed in to change notification settings - Fork 5.8k
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: variable tidb_opt_enable_hash_join
to skip hash join
#46575
Changes from 14 commits
f2a5193
a240907
e48b565
ac6748e
b0a6417
8b3abc5
061c178
cdf1010
7543cc8
eb9850a
cae12dc
cef90b0
3bbd2df
cdb93a7
c6dc9b3
e4dd5d7
a6893ef
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -162,6 +162,7 @@ type logicalOptRule interface { | |
func BuildLogicalPlanForTest(ctx context.Context, sctx sessionctx.Context, node ast.Node, infoSchema infoschema.InfoSchema) (Plan, types.NameSlice, error) { | ||
sctx.GetSessionVars().PlanID.Store(0) | ||
sctx.GetSessionVars().PlanColumnID.Store(0) | ||
sctx.GetSessionVars().EnableHashJoin = true | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Too many CI failures due to the default value of this variable: Can we change the name of the variable from
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. After discussing with PM, we decided to keep it unchanged as There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Too many CI failures... For safety, how about using another solution, which is using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
@coderplay I've pushed some code to your PR directly (tomorrow is the code freeze deadline of v7.4 ...) by using this approach to solve CI failures. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for fixing those @qw4990 ! In the future, how can I detect those issues locally before submitting this PR? |
||
builder, _ := NewPlanBuilder().Init(sctx, infoSchema, &utilhint.BlockHintProcessor{}) | ||
p, err := builder.Build(ctx, node) | ||
if err != nil { | ||
|
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.
Another problem that needs to solve is to avoid
no-plan
error.In the case below, after disabling
Hash Join
, the optimizer can't find a plan for this query.Can we implement this in
LogicalJoin.exhaustPhysicalPlans
where we can know whether there is any other possible join plans?Then to implement it in this way:
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.
This appears to be the behavior we actually want, doesn't it? If a user explicitly disables hash join and also disable merge join, they should expect such consequences. Why should
tidb_opt_enable_hash_join
compromise here instead ofno_merge_join
? Letting it fail seems to be the more predictable outcome; otherwise, users might not be aware of this implicit behavior, right?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.
OK, then we'd better add this into the user doc later on just like what we've done for hints (https://docs.pingcap.com/tidb/dev/optimizer-hints#using-hints-causes-the-cant-find-a-proper-physical-plan-for-this-query-error).
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.
I am creating another PR for the doc.
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.
see pingcap/docs#14905