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

*: implement analyze columns push down #4522

Merged
merged 7 commits into from Sep 18, 2017

Conversation

Projects
None yet
3 participants
@lamxTyler
Member

lamxTyler commented Sep 14, 2017

@coocood

This comment has been minimized.

Show comment
Hide comment
@coocood

coocood Sep 14, 2017

Member

executor/new_analyze.go:150: declaration of "err" shadows declaration at executor/new_analyze.go:149
make: *** [check] Error 1

Member

coocood commented Sep 14, 2017

executor/new_analyze.go:150: declaration of "err" shadows declaration at executor/new_analyze.go:149
make: *** [check] Error 1

@coocood

This comment has been minimized.

Show comment
Hide comment
@coocood

coocood Sep 14, 2017

Member

@lamxTyler
We should use a constant probability to collect samples in TiKV.
If the rows count in regions are not equal, use the default sample collector will collect more samples in small regions, less samples in large regions.

Member

coocood commented Sep 14, 2017

@lamxTyler
We should use a constant probability to collect samples in TiKV.
If the rows count in regions are not equal, use the default sample collector will collect more samples in small regions, less samples in large regions.

@lamxTyler

This comment has been minimized.

Show comment
Hide comment
@lamxTyler

lamxTyler Sep 14, 2017

Member

However, we may not collect enough stats if we use constant probability because we do not know the total row count. @coocood

Member

lamxTyler commented Sep 14, 2017

However, we may not collect enough stats if we use constant probability because we do not know the total row count. @coocood

@coocood

This comment has been minimized.

Show comment
Hide comment
@coocood

coocood Sep 14, 2017

Member

@lamxTyler
If the table is very small, we can analyze again with a larger probability.
This doesn't add much cost.

Member

coocood commented Sep 14, 2017

@lamxTyler
If the table is very small, we can analyze again with a larger probability.
This doesn't add much cost.

@lamxTyler

This comment has been minimized.

Show comment
Hide comment
@lamxTyler

lamxTyler Sep 14, 2017

Member

The table may change during the analyze. You cannot guarantee a larger probability gives larger samples. @coocood

Member

lamxTyler commented Sep 14, 2017

The table may change during the analyze. You cannot guarantee a larger probability gives larger samples. @coocood

@coocood

This comment has been minimized.

Show comment
Hide comment
@coocood

coocood Sep 14, 2017

Member

LGTM

Member

coocood commented Sep 14, 2017

LGTM

return nil, errors.Trace(err)
}
defer func() {
if err1 := e.result.Close(); err1 != nil {

This comment has been minimized.

@hanfei1991

hanfei1991 Sep 18, 2017

Member

It seems we needn't check the error, because it has been returned.

@hanfei1991

hanfei1991 Sep 18, 2017

Member

It seems we needn't check the error, because it has been returned.

This comment has been minimized.

@lamxTyler

lamxTyler Sep 18, 2017

Member

Nope, this function will be called just before return, so if there is an error, the return value will be modified.

@lamxTyler

lamxTyler Sep 18, 2017

Member

Nope, this function will be called just before return, so if there is an error, the return value will be modified.

Show outdated Hide outdated executor/new_analyze.go
@hanfei1991

LGTM

@lamxTyler

This comment has been minimized.

Show comment
Hide comment
@lamxTyler

lamxTyler Sep 18, 2017

Member

/run-all-test

Member

lamxTyler commented Sep 18, 2017

/run-all-test

@lamxTyler lamxTyler merged commit 31f7b9c into master Sep 18, 2017

3 of 4 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
ci/circleci Your tests passed on CircleCI!
Details
jenkins-ci-tidb/build Jenkins job succeeded.
Details
license/cla Contributor License Agreement is signed.
Details

@lamxTyler lamxTyler deleted the xhb/col branch Sep 18, 2017

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