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.

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

plan: new plan supports join. #3126

Merged
merged 5 commits into from
Apr 25, 2017
Merged

plan: new plan supports join. #3126

merged 5 commits into from
Apr 25, 2017

Conversation

hanfei1991
Copy link
Member

This PR let new plan support join. We only introduce hash join now, and in order to simplify problem, hash join will block any physical property.
@shenli @coocood @winoros @lamxTyler PTAL

case RightOuterJoin:
hashJoin.SmallTable = 0
case InnerJoin:
// We will use right table as small table.
Copy link
Member

Choose a reason for hiding this comment

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

What does this comment mean?

Copy link
Member Author

Choose a reason for hiding this comment

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

If right plan's count is smaller than the left one, we will treat right plan as small table.

@shenli shenli added the priority/P1 The issue has P1 priority. label Apr 24, 2017
@@ -27,6 +27,7 @@ type taskProfile interface {
cost() float64
copy() taskProfile
plan() PhysicalPlan
finishTask(ctx context.Context, allocator *idAllocator) taskProfile
Copy link
Member

Choose a reason for hiding this comment

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

The finishTask only make sense in copTaskProfile, promote it to taskProfile is confusing.

Copy link
Member

Choose a reason for hiding this comment

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

finishCopTask makes more sense.
And add comment on it.

Copy link
Member

Choose a reason for hiding this comment

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

Move it to a plain function is better, it makes the interface clean.

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed, PTAL

@coocood
Copy link
Member

coocood commented Apr 25, 2017

LGTM

@hanfei1991 hanfei1991 added the status/LGT1 Indicates that a PR has LGTM 1. label Apr 25, 2017
if cop, ok := profile.(*copTaskProfile); ok {
profile = cop.finishTask(p.ctx, p.allocator)
}
profile = finishCopTask(profile, p.ctx, p.allocator)
Copy link
Member

Choose a reason for hiding this comment

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

Why not check task type here?

Copy link
Member Author

Choose a reason for hiding this comment

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

task checking is moved to finishCopTask

@@ -378,7 +401,7 @@ func (p *PhysicalAggregation) attach2TaskProfile(profiles ...taskProfile) taskPr
cop.cnt = cop.cnt * aggFactor
}
}
profile = cop.finishTask(p.ctx, p.allocator)
profile = finishCopTask(cop, p.ctx, p.allocator)
Copy link
Member

Choose a reason for hiding this comment

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

Need cop.copy()?

Copy link
Member Author

Choose a reason for hiding this comment

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

needn't, finishCopTask will not change cop task.

@shenli
Copy link
Member

shenli commented Apr 25, 2017

LGTM

@hanfei1991 hanfei1991 merged commit 79a89fc into master Apr 25, 2017
@hanfei1991 hanfei1991 deleted the hanfei/join branch April 25, 2017 15:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority/P1 The issue has P1 priority. status/LGT1 Indicates that a PR has LGTM 1.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants