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

planner: support a hint to force using a IndexMerge path #12843

Merged
merged 2 commits into from Nov 27, 2019

Conversation

@hailanwhu
Copy link
Contributor

hailanwhu commented Oct 20, 2019

What problem does this PR solve?

Support a hint to access a table using IndexMerge way.

What is changed and how it works?

It is just like the other hints.

Check List

Tests

  • Unit test

Code changes

  • Has exported function/method change
    mainly change func (ds *DataSource) accessPathsForConds and func (ds *DataSource) DeriveStats

  • Has exported variable/fields change
    tableHintInfo adds the indexMergeHintList field.
    Datasource adds the indexMergeHint field.
    Side effects

  • Possible performance regression

  • Breaking backward compatibility

@codecov

This comment has been minimized.

Copy link

codecov bot commented Oct 20, 2019

Codecov Report

Merging #12843 into master will increase coverage by 0.0839%.
The diff coverage is 94.9367%.

@@               Coverage Diff                @@
##             master     #12843        +/-   ##
================================================
+ Coverage   80.0403%   80.1242%   +0.0839%     
================================================
  Files           474        474                
  Lines        116630     116695        +65     
================================================
+ Hits          93351      93501       +150     
+ Misses        15897      15805        -92     
- Partials       7382       7389         +7
@hailanwhu

This comment has been minimized.

Copy link
Contributor Author

hailanwhu commented Oct 20, 2019

@eurekaka, it failed to add the test for can't find proper plan. because if it returns a invalidTask, TiDB will generate an error for it and the explain test will fail.

@zz-jason zz-jason requested review from foreyes and eurekaka Oct 21, 2019
Copy link
Member

lamxTyler left a comment

Could you also generate hints for index merge in GenHintsFromPhysicalPlan?

Copy link
Contributor

eurekaka left a comment

Also, I think we should update genHintsFromPhysicalPlan for index merge hint as well, @lamxTyler please confirm.

--

Oh, I just noticed Haibin had already commented about it.

planner/core/find_best_task.go Outdated Show resolved Hide resolved
planner/core/stats.go Show resolved Hide resolved
planner/core/find_best_task.go Outdated Show resolved Hide resolved
@hailanwhu hailanwhu force-pushed the hailanwhu:IdxMergeHint branch from 9b3353b to 7aa3733 Oct 24, 2019
@hailanwhu

This comment has been minimized.

Copy link
Contributor Author

hailanwhu commented Oct 24, 2019

@eurekaka all the comments are fixed.

planner/core/find_best_task.go Outdated Show resolved Hide resolved
planner/core/hints.go Outdated Show resolved Hide resolved
planner/core/stats.go Show resolved Hide resolved
planner/core/stats.go Outdated Show resolved Hide resolved
@hailanwhu hailanwhu force-pushed the hailanwhu:IdxMergeHint branch from a38cf51 to 56629b7 Oct 25, 2019
@hailanwhu

This comment has been minimized.

Copy link
Contributor Author

hailanwhu commented Oct 25, 2019

@eurekaka all comments are fixed.

planner/core/logical_plan_builder.go Outdated Show resolved Hide resolved
planner/core/stats.go Outdated Show resolved Hide resolved
planner/core/stats.go Outdated Show resolved Hide resolved
planner/core/stats.go Outdated Show resolved Hide resolved
planner/core/stats.go Show resolved Hide resolved
planner/core/stats.go Outdated Show resolved Hide resolved
@hailanwhu hailanwhu force-pushed the hailanwhu:IdxMergeHint branch 4 times, most recently from 387cb47 to 4913c9c Oct 29, 2019
planner/core/stats.go Outdated Show resolved Hide resolved
planner/core/find_best_task.go Outdated Show resolved Hide resolved
planner/core/stats.go Outdated Show resolved Hide resolved
sessionctx/stmtctx/stmtctx.go Outdated Show resolved Hide resolved
@hailanwhu hailanwhu force-pushed the hailanwhu:IdxMergeHint branch 2 times, most recently from a8bcf7f to 28b1c1f Nov 12, 2019
@hailanwhu

This comment has been minimized.

Copy link
Contributor Author

hailanwhu commented Nov 12, 2019

@eurekaka all done

Copy link
Contributor

eurekaka left a comment

LGTM

@eurekaka eurekaka requested a review from lamxTyler Nov 13, 2019
planner/core/stats.go Outdated Show resolved Hide resolved
planner/core/stats.go Outdated Show resolved Hide resolved
@hailanwhu hailanwhu force-pushed the hailanwhu:IdxMergeHint branch 2 times, most recently from 1d464df to 1ab52fe Nov 15, 2019
@hailanwhu

This comment has been minimized.

Copy link
Contributor Author

hailanwhu commented Nov 26, 2019

@eurekaka @lamxTyler this has been somedays. If it is ok, I can solve the conflict and push it again.

@eurekaka eurekaka requested a review from lamxTyler Nov 26, 2019
Copy link
Member

lamxTyler left a comment

LGTM
Please resolve the conflicts.

@hailanwhu hailanwhu force-pushed the hailanwhu:IdxMergeHint branch from 1ab52fe to 6e132b2 Nov 27, 2019
@CLAassistant

This comment has been minimized.

Copy link

CLAassistant commented Nov 27, 2019

CLA assistant check
All committers have signed the CLA.

@hailanwhu hailanwhu force-pushed the hailanwhu:IdxMergeHint branch from 6e132b2 to 78ba597 Nov 27, 2019
@hailanwhu

This comment has been minimized.

Copy link
Contributor Author

hailanwhu commented Nov 27, 2019

@eurekaka @lamxTyler it is finished.

Copy link
Member

lamxTyler left a comment

LGTM

@sre-bot

This comment has been minimized.

Copy link

sre-bot commented Nov 27, 2019

/run-all-tests

@sre-bot sre-bot merged commit 596fb64 into pingcap:master Nov 27, 2019
14 checks passed
14 checks passed
idc-jenkins-ci-tidb/build Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/build_check_race Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/check_dev Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/check_dev_2 Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/common-test job succeeded
Details
idc-jenkins-ci-tidb/integration-common-test Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/integration-compatibility-test Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/integration-copr-test Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/integration-ddl-test Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/mybatis-test job succeeded
Details
idc-jenkins-ci-tidb/sqllogic-test-1 Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/sqllogic-test-2 Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/unit-test Jenkins job succeeded.
Details
license/cla Contributor License Agreement is signed.
Details
edytagarbarz added a commit to edytagarbarz/tidb that referenced this pull request Dec 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.