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/core: fix range info for the inner child of index join in explain (#8058) #9528

Closed
wants to merge 4 commits into from

Conversation

jarvys
Copy link
Contributor

@jarvys jarvys commented Mar 2, 2019

What problem does this PR solve?

fix issue #8058 , fix range info for the inner child of index join.

What is changed and how it works?

Add two more fields rangesWithFakeConds and keyOff2IdxOff to struct PhysicalIndexScan. When the ExplainInfo method called, generate range info with these two fields, not only with rangeDecidedBy.

Check List

Tests

  • Unit test: add TestIssue8058 in cbo_test.go

Code changes

  • Has exported function/method change

Side effects

  • Nothing

Related changes

  • Nothing

@CLAassistant
Copy link

CLAassistant commented Mar 2, 2019

CLA assistant check
All committers have signed the CLA.

@zz-jason zz-jason added contribution This PR is from a community contributor. sig/planner SIG: Planner labels Mar 3, 2019
@zz-jason zz-jason requested review from winoros and eurekaka March 3, 2019 05:56
@shenli
Copy link
Member

shenli commented Mar 3, 2019

@jarvys Thanks! Please fix the CI.

@winoros
Copy link
Member

winoros commented Mar 4, 2019

Hi, thanks for your contribution.

  1. please take a look at planner, executor: index join enhancement #8471. When the index join support using t.c > tt.c and t.c < tt.c + 100 to construct ranges. The way you used cannot show enough information.
  2. And you can just pass a string information to IndexScan to avoid add more fields to it. IMHO, Using three fields just to construct explain information is a little expensive.
  3. You can use make dev to make sure you pass all the unit-test and the explain test in cmd/explaintest.

@jarvys
Copy link
Contributor Author

jarvys commented Mar 4, 2019

@winoros Thanks for code review!

@jarvys
Copy link
Contributor Author

jarvys commented Mar 5, 2019

@winoros I have made all the test cases passed by modifying the expect result of some test cases. I don't understand how the PR #8471 works now. I'm learning it.

I find you use the buildRangeDecidedByInformation method to build range info :)

@jarvys
Copy link
Contributor Author

jarvys commented Mar 8, 2019

@winoros I find issue #9562. Maybe it's better to fix it first before the problems in #8471 and #9528

@codecov-io
Copy link

codecov-io commented Mar 8, 2019

Codecov Report

Merging #9528 into master will decrease coverage by 0.0241%.
The diff coverage is 17.8571%.

@@               Coverage Diff                @@
##             master      #9528        +/-   ##
================================================
- Coverage   67.3963%   67.3722%   -0.0242%     
================================================
  Files           374        374                
  Lines         78942      78994        +52     
================================================
+ Hits          53204      53220        +16     
- Misses        20986      21022        +36     
  Partials       4752       4752

@qw4990 qw4990 self-requested a review March 8, 2019 06:21
@winoros
Copy link
Member

winoros commented Jul 16, 2019

Thanks for you contribution!
But we have to close it since #8471 has done the same thing.

@winoros winoros closed this Jul 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution This PR is from a community contributor. sig/planner SIG: Planner
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants