plan, statistics: maintain `HistColl` in `DataSource`'s `StatsInfo` #7385

winoros · 2018-08-14T05:57:16Z

What problem does this PR solve?

Maintain HistColl in DataSource's StatsInfo to get more accurate statistics.

What is changed and how it works?

Map the column by its UniqueID rather than ID. So it can be maintained in the other plan's StatsInfo later.
And map the index and column by column's UniqueID rather than column's name.

Check List

Tests

Exsiting tests.

Code changes

Has exported function/method change
Has exported variable/fields change

alivxxx · 2018-08-14T07:26:26Z

plan/stats.go

@@ -27,6 +28,8 @@ type statsInfo struct {
 	count       float64
 	cardinality []float64

+	histColl    statistics.HistColl
+	idx2Columns map[int][]int


alivxxx · 2018-08-14T07:39:37Z

statistics/table.go

+			if rangePosition >= len(colIDs) {
+				colID = -1
+			} else {
+				colID = coll.Idx2ColumnIDs[idxID][rangePosition]


coll.Idx2ColumnIDs[idxID] -> colIDs.

alivxxx · 2018-08-14T08:03:56Z

statistics/table.go

+		uniqueID, ok := colInfoID2UniqueID[id]
+		// If this column is not in datasource's schema, it won't be used in this query.
+		if ok {
+			newColHistMap[uniqueID] = colHist


Do we need to adjust here, because the colID is unique id, not column id?

emm... Let me think a while

winoros · 2018-08-22T11:46:08Z

@lamxTyler I realized that statistics.Column.Info.ID is the one to load column stats so i use it in ColumnIsInvalid. But there comes a question that only DataSource can load column. But actually in other operator it may need the column that the DataSource don't need. Maybe we need add a TableID in ColumnInfo struct?
@zz-jason What do you think?

alivxxx · 2018-08-24T02:52:58Z

I prefer to put it in the statistics.Column.

winoros · 2018-08-24T12:11:44Z

@lamxTyler
Reasonable, I'll change it in a separate pr.
You can review this first.
@zz-jason PTAL

zz-jason · 2018-08-27T02:43:49Z

statistics/table.go

+	}
+	for id, colHist := range coll.Columns {
+		uniqueID, ok := colInfoID2UniqueID[id]
+		// If this column is not in datasource's schema, it won't be used in this query.


It's better to describe the beavior directlly, there is no need to explain why we don't collect the statistics of other columns:

// collect the statistics of the column in the schema of DataSource.

zz-jason · 2018-08-27T02:56:05Z

statistics/table.go

+		if len(ids) == 0 {
+			continue
+		}
+		colID2IdxID[ids[0]] = idxHist.ID


If the column is a prefix of more than one composite index, We only store the last index?

Yes, this operation is the same with the original colName2Idx map.

zz-jason · 2018-08-27T07:16:38Z

@winoros please merge master and resolve conflicts

zz-jason · 2018-08-27T12:25:21Z

statistics/feedback.go

-				return
-			}
-			rangeString = colRangeToStr(t.Columns[t.colName2ID[colName]], &rang, -1, factor)
+			return
 		}
 		log.Debugf("%s index: %s, actual: %d, equality: %s, expected equality: %d, %s", prefix, idx.Info.Name.O,


this line can be removed?

the else branch in line 852 returns the execution of this function. So this is a dead code.

Oh, its meaning changed after i changing the code. This log should be in the if branch now.

winoros · 2018-08-28T05:27:38Z

@lamxTyler PTAL

alivxxx · 2018-08-28T06:28:04Z

statistics/table.go

-func (coll *HistColl) getIndexRowCount(sc *stmtctx.StatementContext, idx *Index, indexRanges []*ranger.Range, modifyCount int64) (float64, error) {
+// GenerateHistCollFromColumnInfo generates a new HistColl whose ColID2IdxID and IdxID2ColIDs is built from the given parameter.
+func (coll *HistColl) GenerateHistCollFromColumnInfo(infos []*model.ColumnInfo, columns []*expression.Column) HistColl {
+


Remove this blank line.

@winoros PTAL this comment.

statistics/table.go

zz-jason

LGTM

alivxxx

LGTM

alivxxx · 2018-08-30T07:39:11Z

/run-all-tests

zhexuany

LGTM

winoros · 2018-08-30T11:12:39Z

I'm looking into the failure in test.

…into hist-in-datasource

winoros · 2018-08-30T12:05:58Z

/run-all-tests

zz-jason · 2018-08-30T12:06:26Z

expression/column.go

-func FindColumnsByUniqueIDs(cols []*Column, ids []int) []*Column {
+// FindColumnsByUniqueIDList will find columns by checking the unique id.
+// Then return order is the same with `ids`. If one id doesn't exists in `cols`. We'll just break.
+func FindColumnsByUniqueIDList(cols []*Column, ids []int64) []*Column {


how about:

s/FindColumnsByUniqueIDList/FindPrefixOfIndexCols/

s/ids/indexColIDs

I think OfGivenCols is better, since a more common name is better?

But it seems that only when the second parameter is column ids of an index the prefix is returned. In other situation, all the matched columns should be returned?

By the way, the caller of this function now only pass the column ids of an index, except for the test code.

zz-jason

LGTM

winoros added 2 commits August 14, 2018 13:44

plan, statistics: maintain HistColl in DataSource's StatsInfo

8962c7d

Merge branch 'master' into hist-in-datasource

2d83682

winoros added the sig/planner SIG: Planner label Aug 14, 2018

fix test.

7cb4525

alivxxx reviewed Aug 14, 2018

View reviewed changes

winoros added 4 commits August 22, 2018 15:27

Merge branch 'master' into hist-in-datasource

3a0c397

fix debug log

34133a8

use column info id.

4de6a6a

address comments

a00a1a1

fix vet

ba4b400

zz-jason reviewed Aug 27, 2018

View reviewed changes

winoros added 3 commits August 27, 2018 15:20

Merge branch 'master' into hist-in-datasource

098dbd7

address comment.

c86bba8

fix test after merging.

819822a

zz-jason reviewed Aug 27, 2018

View reviewed changes

the log should work.

8eefc9f

alivxxx reviewed Aug 28, 2018

View reviewed changes

statistics/table.go Show resolved Hide resolved

zhexuany reviewed Aug 29, 2018

View reviewed changes

statistics/table.go Show resolved Hide resolved

zhexuany reviewed Aug 29, 2018

View reviewed changes

statistics/table.go Show resolved Hide resolved

zz-jason reviewed Aug 30, 2018

View reviewed changes

alivxxx mentioned this pull request Aug 30, 2018

stats: update the way of using index feedback #7488

Merged

remove blank line.

bc42577

alivxxx previously approved these changes Aug 30, 2018

View reviewed changes

Merge branch 'master' into hist-in-datasource

a6afaae

alivxxx added the status/LGT2 Indicates that a PR has LGTM 2. label Aug 30, 2018

zhexuany previously approved these changes Aug 30, 2018

View reviewed changes

winoros added 2 commits August 30, 2018 20:00

fix FindColumnsByUniqueIDs

8ee0de6

Merge branch 'hist-in-datasource' of https://github.com/winoros/tidb …

57c331c

…into hist-in-datasource

winoros dismissed stale reviews from zhexuany and alivxxx via 57c331c August 30, 2018 12:00

zz-jason reviewed Aug 30, 2018

View reviewed changes

address comment and tiny change.

a4df382

zz-jason approved these changes Aug 30, 2018

View reviewed changes

Merge branch 'master' into hist-in-datasource

e542ea7

zz-jason added the status/all tests passed label Aug 30, 2018

Merge branch 'master' into hist-in-datasource

468b6fb

zz-jason merged commit 87c54b2 into pingcap:master Aug 30, 2018

eurekaka mentioned this pull request Aug 31, 2018

plan: change the logic of converting to index join #7553

Merged

CaitinChen mentioned this pull request Sep 5, 2018

proposal: maintaining histograms in plan. #7605

Merged

winoros deleted the hist-in-datasource branch September 5, 2018 07:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

plan, statistics: maintain `HistColl` in `DataSource`'s `StatsInfo` #7385

plan, statistics: maintain `HistColl` in `DataSource`'s `StatsInfo` #7385

winoros commented Aug 14, 2018

alivxxx Aug 14, 2018

alivxxx Aug 14, 2018

alivxxx Aug 14, 2018

winoros Aug 15, 2018

winoros commented Aug 22, 2018

alivxxx commented Aug 24, 2018

winoros commented Aug 24, 2018

zz-jason Aug 27, 2018

zz-jason Aug 27, 2018

winoros Aug 27, 2018

zz-jason commented Aug 27, 2018

zz-jason Aug 27, 2018

winoros Aug 27, 2018

zz-jason Aug 28, 2018

winoros Aug 28, 2018

winoros commented Aug 28, 2018

alivxxx Aug 28, 2018

zz-jason Aug 30, 2018

zz-jason left a comment

alivxxx left a comment

alivxxx commented Aug 30, 2018

zhexuany left a comment

winoros commented Aug 30, 2018

winoros commented Aug 30, 2018

zz-jason Aug 30, 2018

winoros Aug 30, 2018 •

edited

Loading

zz-jason Aug 30, 2018

zz-jason left a comment

plan, statistics: maintain HistColl in DataSource's StatsInfo #7385

plan, statistics: maintain HistColl in DataSource's StatsInfo #7385

Conversation

winoros commented Aug 14, 2018

What problem does this PR solve?

What is changed and how it works?

Check List

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

winoros commented Aug 22, 2018

alivxxx commented Aug 24, 2018

winoros commented Aug 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zz-jason commented Aug 27, 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

winoros commented Aug 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zz-jason left a comment

Choose a reason for hiding this comment

alivxxx left a comment

Choose a reason for hiding this comment

alivxxx commented Aug 30, 2018

zhexuany left a comment

Choose a reason for hiding this comment

winoros commented Aug 30, 2018

winoros commented Aug 30, 2018

Choose a reason for hiding this comment

winoros Aug 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zz-jason left a comment

Choose a reason for hiding this comment

plan, statistics: maintain `HistColl` in `DataSource`'s `StatsInfo` #7385

plan, statistics: maintain `HistColl` in `DataSource`'s `StatsInfo` #7385

winoros Aug 30, 2018 •

edited

Loading