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

ranger: split detach process from BuildRange. #4741

Merged
merged 4 commits into from Oct 12, 2017
Merged

ranger: split detach process from BuildRange. #4741

merged 4 commits into from Oct 12, 2017

Conversation

winoros
Copy link
Member

@winoros winoros commented Oct 10, 2017

BuildRange is used when calculate range for scan plan and for selectivity. The detach process of this two situation won't be the same in the future, so split the detach process out of the BuildRange method.
PTAL @coocood @hanfei1991 @lamxTyler

@@ -206,35 +211,42 @@ func buildColumnRange(conds []expression.Expression, sc *variable.StatementConte
return ranges, nil
}

// DetachCondsForSelectivity distinguishes between conditions dor selectivity calculation and other conditions
Copy link
Contributor

Choose a reason for hiding this comment

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

dor? Also, Add a dot at the end.

Copy link
Contributor

@alivxxx alivxxx left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -206,35 +211,42 @@ func buildColumnRange(conds []expression.Expression, sc *variable.StatementConte
return ranges, nil
}

// DetachCondsForSelectivity distinguishes between conditions or selectivity calculation and other conditions.
Copy link
Member

Choose a reason for hiding this comment

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

The comment is not clear.

eqCount int
)
for eqCount = 0; eqCount < len(accessCondition) && eqCount < len(cols); eqCount++ {
if sf, ok := accessCondition[eqCount].(*expression.ScalarFunction); !ok || sf.FuncName.L != ast.EQ {
Copy link
Member

Choose a reason for hiding this comment

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

The original is inAndEqCount, now only Eq is counted.

Copy link
Member Author

Choose a reason for hiding this comment

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

in has be rewritten to or. Currently there won't be any in function any more.

@winoros winoros added the status/LGT1 Indicates that a PR has LGTM 1. label Oct 12, 2017
@winoros
Copy link
Member Author

winoros commented Oct 12, 2017

PTAL @coocood

@coocood
Copy link
Member

coocood commented Oct 12, 2017

LGTM

@coocood
Copy link
Member

coocood commented Oct 12, 2017

/run-all-test

@shenli shenli merged commit 95a6e1e into master Oct 12, 2017
@shenli shenli deleted the yiding/split branch October 12, 2017 16:22
@hanfei1991 hanfei1991 added this to Others in Planner Oct 17, 2017
@hanfei1991 hanfei1991 moved this from Others to ranger in Planner Oct 19, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status/LGT1 Indicates that a PR has LGTM 1.
Projects
No open projects
Planner
  
ranger
Development

Successfully merging this pull request may close these issues.

None yet

4 participants