*: fast path point select #6937

coocood · 2018-06-29T02:20:53Z

What have you changed? (mandatory)

Skip optimizer and coprocessor for point select.

What are the type of the changes (mandatory)?

The currently defined types are listed below, please pick one of the types for this PR by removing the others:

Improvement (non-breaking change which is an improvement to an existing feature)

How has this PR been tested (mandatory)?

Sysbench point select.

Refer to a related PR or issue link (optional)

Benchmark result if necessary (optional)

With a single faketikv and single tidb-server on a 24 core linux machine, improve sysbench point select by 60%, from 19000 to 32000

Add a few positive/negative examples (optional)

Skip optimizer and coprocessor for point select.

coocood · 2018-07-10T06:57:17Z

/run-all-tests

coocood · 2018-07-11T01:39:41Z

/run-all-tests

coocood · 2018-07-11T02:24:26Z

/run-all-tests

coocood · 2018-07-11T03:13:46Z

/run-mybatis-test

coocood · 2018-07-15T13:36:10Z

/rebuild

…nt-select Conflicts: plan/plan.go plan/stats.go

…nt-select Conflicts: cmd/explaintest/r/explain_easy.result statistics/selectivity_test.go

coocood · 2018-07-19T13:05:39Z

@tiancaiamao PTAL

winoros · 2018-07-19T16:27:26Z

plan/stats.go

@@ -69,10 +69,14 @@ func newSimpleStats(rowCount float64) *statsInfo {
 	return &statsInfo{count: rowCount}
 }

-func (p *basePhysicalPlan) StatsInfo() *statsInfo {
+func (p *basePhysicalPlan) statsInfo() *statsInfo {


Why do this?

The golint reports error about returning unexported type.

Since it's unexported now, is this interface still needed?

winoros

Most of changes that change handle to Handle should be reverted.

winoros · 2018-07-19T16:29:16Z

plan/rule_partition_processor.go

@@ -41,7 +41,7 @@ type partitionProcessor struct{}
 func (s *partitionProcessor) optimize(lp LogicalPlan) (LogicalPlan, error) {
 	// NOTE: partitionProcessor will assume all filter conditions are pushed down to
 	// DataSource, there will not be a Selection->DataSource case, so the rewrite just
-	// handle the DataSource node.
+	// Handle the DataSource node.


It's not the first word of the sentence.

coocood · 2018-07-23T07:07:47Z

@winoros PTAL

tiancaiamao · 2018-07-23T06:54:01Z

executor/point_select.go

+		idxInfo: p.IndexInfo,
+		idxVals: p.IndexValues,
+		handle:  p.Handle,
+		startTS: b.getStartTS(),


Why not use math.MaxUint64 ?

The statement may be UPDATE/DELETE or in an explicit transaction.

tiancaiamao · 2018-07-23T07:23:16Z

executor/point_select.go

+
+func (e *PointSelectExecutor) get(key kv.Key) (val []byte, err error) {
+	txn := e.ctx.Txn()
+	if txn != nil && txn.Valid() && !txn.IsReadOnly() {


When will this happen? PointUpdate?

tiancaiamao · 2018-07-23T07:26:27Z

expression/integration_test.go

@@ -1581,8 +1581,6 @@ func (s *testIntegrationSuite) TestTimeBuiltin(c *C) {
 	// TODO: MySQL returns "<nil> <nil>".
 	result.Check(testkit.Rows("0000-00-01 <nil>"))
 	tk.MustQuery("show warnings").Check(testutil.RowsWithSep("|", "Warning|1292|Incorrect datetime value: '0000-00-00 00:00:00'"))
-	result = tk.MustQuery("select str_to_date('2018-6-1', '%Y-%m-%d'), str_to_date('2018-6-1', '%Y-%c-%d'), str_to_date('59:20:1', '%s:%i:%k'), str_to_date('59:20:1', '%s:%i:%l')")


I wonder why it's removed?

tiancaiamao · 2018-07-23T08:18:45Z

plan/point_select_plan.go

+
+func findPKHandle(tblInfo *model.TableInfo, pairs []nameValuePair) (d types.Datum) {
+	if !tblInfo.PKIsHandle {
+		return d


d.SetNULL or return something like NULL constant is bettter
This code come with the assumption types.KindInt64 != 0

uninitialized Datum is null is a very intuitive and reasonable assumption.

tiancaiamao · 2018-07-23T08:33:13Z

types/time.go

-	"%Y": yearNumericFourDigits, // Year, numeric, four digits
+	"%b": abbreviatedMonth,           // Abbreviated month name (Jan..Dec)
+	"%c": monthNumeric,               // Month, numeric (0..12)
+	"%d": dayOfMonthNumericTwoDigits, // Day of the month, numeric (00..31)


You can probably do this kind of things in another PR

This may be caused by merge conflict.

…nt-select

jackysp · 2018-07-25T13:51:00Z

I think it's better to find a consistent name for it, maybe PointXXX or FastPathXXX?
:-)

ngaut · 2018-07-27T09:57:01Z

Yes, we should keep consistent.

ngaut · 2018-07-30T02:54:22Z

executor/point_get.go

+	"golang.org/x/net/context"
+)
+
+func (b *executorBuilder) buildPointSelect(p *plan.PointGetPlan) Executor {


s/buildPointSelect/buildPointGet/

ngaut · 2018-07-30T02:57:32Z

executor/point_get.go

+}
+
+func (e *PointGetExecutor) decodeRowValToChunk(rowVal []byte, chk *chunk.Chunk) error {
+	colIDs := make(map[int64]int)


Make with capacity.

tiancaiamao · 2018-07-30T05:28:41Z

It would be nice if maybe you could fix the CI, rest looks good to me @coocood

coocood · 2018-07-30T05:38:42Z

@tiancaiamao @winoros @zz-jason @jackysp

I remove the point-get plan for UPDATE for now, to make the test pass quickly.

Update support can be added in future PR.

coocood · 2018-07-30T05:40:23Z

/run-all-tests

coocood · 2018-07-30T07:31:27Z

/run-all-tests

coocood · 2018-07-30T09:29:19Z

/run-mybatis-test

ngaut

LGTM

tiancaiamao · 2018-07-30T10:07:26Z

LGTM

zz-jason · 2018-07-30T10:17:07Z

executor/builder.go

@@ -1483,7 +1485,7 @@ func buildNoRangeTableReader(b *executorBuilder, v *plan.PhysicalTableReader) (*
 	if containsLimit(dagReq.Executors) {
 		e.feedback = statistics.NewQueryFeedback(0, nil, 0, ts.Desc)
 	} else {
-		e.feedback = statistics.NewQueryFeedback(ts.Table.ID, ts.Hist, ts.StatsInfo().Count(), ts.Desc)
+		e.feedback = statistics.NewQueryFeedback(ts.Table.ID, ts.Hist, int64(ts.StatsCount()), ts.Desc)


why make this change?

go lint reports error because it returns an unexported type statsInfo.

shenli · 2018-07-30T10:15:44Z

plan/point_get_plan.go

+		return nil
+	}
+	handleDatum := findPKHandle(tbl, pairs)
+	if handleDatum.Kind() == types.KindInt64 {


Consider unsign flag?

handle is always int64.
unsign flag doesn't matter.

coocood added the status/WIP label Jun 29, 2018

coocood force-pushed the fast-point-select branch 5 times, most recently from fe4c4a8 to 41d35ee Compare July 3, 2018 03:46

*: fast point select path

2198b32

Skip optimizer and coprocessor for point select.

coocood force-pushed the fast-point-select branch from 41d35ee to 2198b32 Compare July 10, 2018 06:56

coocood changed the title *: fast point select path *: fast path point select Jul 10, 2018

add comments

e855838

coocood added 2 commits July 11, 2018 10:15

fix golint

699b34b

change StatsInfo.Count to StatsCount

e5adb09

fix err shadow

9c01155

Merge commit 'b729a60e019c8fec2cbae007b5b273697cac04b8' into fast-poi…

4942f47

…nt-select Conflicts: plan/plan.go plan/stats.go

coocood removed the status/WIP label Jul 19, 2018

Merge commit '90c721ad72f44ff5849e131d813509d1dedbe035' into fast-poi…

0971fff

…nt-select Conflicts: cmd/explaintest/r/explain_easy.result statistics/selectivity_test.go

fix build

171daed

winoros reviewed Jul 19, 2018

View reviewed changes

revert handle -> Handle in comments

a183771

tiancaiamao reviewed Jul 23, 2018

View reviewed changes

coocood added 2 commits July 23, 2018 19:07

revert unintended change

55757cd

Merge commit '3d04c5ef95198e9e87611939942ed9823629f094' into fast-poi…

01be667

…nt-select

address comments

4bb52a0

ngaut closed this Jul 27, 2018

ngaut reopened this Jul 27, 2018

coocood and others added 2 commits July 28, 2018 22:55

Merge branch 'master' into fast-point-select

b4843e9

Merge branch 'master' into fast-point-select

5b4f1b3

ngaut reviewed Jul 30, 2018

View reviewed changes

Merge branch 'master' into fast-point-select

fd96ed9

remove update and fix tests

aaad31e

coocood added 3 commits July 30, 2018 14:58

fix CI, add test

050235d

fix vet

74fc80d

Merge branch 'master' into fast-point-select

f0ff662

fix result field as name

c8e64d5

coocood added status/all tests passed type/performance labels Jul 30, 2018

Merge branch 'master' into fast-point-select

b8598b2

ngaut approved these changes Jul 30, 2018

View reviewed changes

tiancaiamao approved these changes Jul 30, 2018

View reviewed changes

coocood merged commit 343cb84 into pingcap:master Jul 30, 2018

coocood deleted the fast-point-select branch July 30, 2018 10:13

zz-jason reviewed Jul 30, 2018

View reviewed changes

shenli reviewed Jul 30, 2018

View reviewed changes

*: fast path point select #6937

*: fast path point select #6937

Conversation

coocood commented Jun 29, 2018

What have you changed? (mandatory)

What are the type of the changes (mandatory)?

How has this PR been tested (mandatory)?

Refer to a related PR or issue link (optional)

Benchmark result if necessary (optional)

Add a few positive/negative examples (optional)

coocood commented Jul 10, 2018

coocood commented Jul 11, 2018

coocood commented Jul 11, 2018

coocood commented Jul 11, 2018

coocood commented Jul 15, 2018

coocood commented Jul 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

winoros left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coocood commented Jul 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackysp commented Jul 25, 2018

ngaut commented Jul 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiancaiamao commented Jul 30, 2018

coocood commented Jul 30, 2018

coocood commented Jul 30, 2018

coocood commented Jul 30, 2018

coocood commented Jul 30, 2018

ngaut left a comment

Choose a reason for hiding this comment

tiancaiamao commented Jul 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment