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

*: Support limit with DAG #3004

Merged
merged 4 commits into from
Apr 7, 2017
Merged

*: Support limit with DAG #3004

merged 4 commits into from
Apr 7, 2017

Conversation

zimulala
Copy link
Contributor

@zimulala zimulala commented Apr 6, 2017

No description provided.

Copy link
Member

@hanfei1991 hanfei1991 left a comment

Choose a reason for hiding this comment

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

LGTM

// And it's used for testing.
desc := false
var desc bool
if sel.OrderBy != nil {
Copy link
Member

Choose a reason for hiding this comment

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

Your check is wrong....
it means a desc scan when sel.OrderBy[0].Expr is nil.

}

// extractExecutors is used for testing.
func extractExecutors(sel *tipb.SelectRequest) []*tipb.Executor {
Copy link
Member

Choose a reason for hiding this comment

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

Seems it is not used in unit test.

@shenli
Copy link
Member

shenli commented Apr 6, 2017

/cc @AndreMouche @hhkbp2

@zimulala zimulala added the WIP label Apr 7, 2017
@hanfei1991
Copy link
Member

LGTM

@hanfei1991 hanfei1991 added the status/LGT1 Indicates that a PR has LGTM 1. label Apr 7, 2017
@zimulala
Copy link
Contributor Author

zimulala commented Apr 7, 2017

PTAL @shenli

@shenli
Copy link
Member

shenli commented Apr 7, 2017

LGTM

@shenli shenli merged commit 5eb8a81 into master Apr 7, 2017
@shenli shenli deleted the zimuxia/dag-limit branch April 7, 2017 09:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/coprocessor status/LGT1 Indicates that a PR has LGTM 1.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants