-
Notifications
You must be signed in to change notification settings - Fork 6.1k
planner: maintain a map of column indices by name in DataSource #64204
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
base: master
Are you sure you want to change the base?
Changes from all commits
5a57bc0
1078e5e
21d6381
eb145bd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,6 +17,7 @@ package logicalop | |
| import ( | ||
| "bytes" | ||
| "fmt" | ||
| "maps" | ||
|
|
||
| "github.com/pingcap/tidb/pkg/expression" | ||
| "github.com/pingcap/tidb/pkg/infoschema" | ||
|
|
@@ -52,6 +53,8 @@ type DataSource struct { | |
| Columns []*model.ColumnInfo | ||
| DBName ast.CIStr | ||
|
|
||
| ColIdxsByName map[string]int | ||
|
|
||
| TableAsName *ast.CIStr `hash64-equals:"true"` | ||
| // IndexMergeHints are the hint for indexmerge. | ||
| IndexMergeHints []h.HintedIndex | ||
|
|
@@ -195,6 +198,10 @@ func (ds *DataSource) PruneColumns(parentUsedCols []*expression.Column) (base.Lo | |
| expression.GcColumnExprIsTidbShard(ds.Schema().Columns[i].VirtualExpr) { | ||
| continue | ||
| } | ||
| delete(ds.ColIdxsByName, ds.Columns[i].Name.L) | ||
| for j := i + 1; j < len(ds.Columns); j++ { | ||
| ds.ColIdxsByName[ds.Columns[j].Name.L] = j - 1 | ||
| } | ||
|
Comment on lines
+201
to
+204
|
||
| // TODO: investigate why we cannot use slices.Delete for these two: | ||
| ds.Schema().Columns = append(ds.Schema().Columns[:i], ds.Schema().Columns[i+1:]...) | ||
| ds.Columns = append(ds.Columns[:i], ds.Columns[i+1:]...) | ||
|
|
@@ -207,8 +214,7 @@ func (ds *DataSource) PruneColumns(parentUsedCols []*expression.Column) (base.Lo | |
| var handleCol *expression.Column | ||
| var handleColInfo *model.ColumnInfo | ||
| handleCol, handleColInfo = preferKeyColumnFromTable(ds, originSchemaColumns, originColumns) | ||
| ds.Columns = append(ds.Columns, handleColInfo) | ||
| ds.Schema().Append(handleCol) | ||
| ds.AppendColumn(handleCol, handleColInfo) | ||
| addOneHandle = true | ||
| } | ||
| // ref: https://github.com/pingcap/tidb/issues/44579 | ||
|
|
@@ -498,6 +504,8 @@ func (ds *DataSource) buildIndexGather(path *util.AccessPath) base.LogicalPlan { | |
|
|
||
| is.Columns = make([]*model.ColumnInfo, len(ds.Columns)) | ||
| copy(is.Columns, ds.Columns) | ||
| is.ColIdxsByName = make(map[string]int, len(ds.Columns)) | ||
| maps.Copy(is.ColIdxsByName, ds.ColIdxsByName) | ||
| is.SetSchema(ds.Schema()) | ||
| is.IdxCols, is.IdxColLens = expression.IndexInfo2PrefixCols(is.Columns, is.Schema().Columns, is.Index) | ||
|
|
||
|
|
@@ -695,6 +703,15 @@ func isIndexColsCoveringCol(sctx expression.EvalContext, col *expression.Column, | |
| return false | ||
| } | ||
|
|
||
| // AppendColumn appends the given column and info to the columns and schema's | ||
| // columns of this data source. | ||
| func (ds *DataSource) AppendColumn(col *expression.Column, colInfo *model.ColumnInfo) { | ||
| colIdx := len(ds.Columns) | ||
| ds.ColIdxsByName[colInfo.Name.L] = colIdx | ||
| ds.Columns = append(ds.Columns, colInfo) | ||
| ds.Schema().Append(col) | ||
| } | ||
|
|
||
| // AppendTableCol appends a column to the original columns of the table before pruning, | ||
| // accessed through ds.TblCols and ds.TblColsByID. | ||
| func (ds *DataSource) AppendTableCol(col *expression.Column) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -42,6 +42,7 @@ type LogicalIndexScan struct { | |
|
|
||
| Index *model.IndexInfo | ||
| Columns []*model.ColumnInfo | ||
| ColIdxsByName map[string]int | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i saw the adding of ColIdxsByName into the PhysicalTableScan and LogicalIndexScan, but any usage of them?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This would be used when calling |
||
| FullIdxCols []*expression.Column | ||
| FullIdxColLens []int | ||
| IdxCols []*expression.Column | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,6 +17,7 @@ package physicalop | |
| import ( | ||
| "bytes" | ||
| "fmt" | ||
| "maps" | ||
| "slices" | ||
| "strconv" | ||
| "strings" | ||
|
|
@@ -97,6 +98,8 @@ type PhysicalTableScan struct { | |
| DBName ast.CIStr `plan-cache-clone:"shallow"` | ||
| Ranges []*ranger.Range `plan-cache-clone:"shallow"` | ||
|
|
||
| ColIdxsByName map[string]int `plan-cache-clone:"shallow"` | ||
|
|
||
| TableAsName *ast.CIStr `plan-cache-clone:"shallow"` | ||
|
|
||
| PhysicalTableID int64 | ||
|
|
@@ -161,6 +164,7 @@ func GetPhysicalScan4LogicalTableScan(s *logicalop.LogicalTableScan, schema *exp | |
| ts := PhysicalTableScan{ | ||
| Table: ds.TableInfo, | ||
| Columns: ds.Columns, | ||
| ColIdxsByName: ds.ColIdxsByName, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why this place didn't use the clone?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I only cloned in places where |
||
| TableAsName: ds.TableAsName, | ||
| DBName: ds.DBName, | ||
| isPartition: ds.PartitionDefIdx != nil, | ||
|
|
@@ -180,6 +184,7 @@ func GetOriginalPhysicalTableScan(ds *logicalop.DataSource, prop *property.Physi | |
| ts := PhysicalTableScan{ | ||
| Table: ds.TableInfo, | ||
| Columns: slices.Clone(ds.Columns), | ||
| ColIdxsByName: maps.Clone(ds.ColIdxsByName), | ||
| TableAsName: ds.TableAsName, | ||
| DBName: ds.DBName, | ||
| isPartition: ds.PartitionDefIdx != nil, | ||
|
|
@@ -767,6 +772,7 @@ func BuildIndexMergeTableScan(ds *logicalop.DataSource, tableFilters []expressio | |
| ts := PhysicalTableScan{ | ||
| Table: ds.TableInfo, | ||
| Columns: slices.Clone(ds.Columns), | ||
| ColIdxsByName: maps.Clone(ds.ColIdxsByName), | ||
| TableAsName: ds.TableAsName, | ||
| DBName: ds.DBName, | ||
| PhysicalTableID: ds.PhysicalTableID, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use a map
map[int]intto store the mapping: id -> offset.And then use the
tableInfo.Columns[IndexColumn.Offset].IDto get the needed information.I think the
map[string]intwill be hard to extend for other usage.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This map is used to cooperate with
expression.Schemaand other related stuff, but it's defined outside of it.It's easy to forget to update it when pruning columns and adding columns.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As an example, I don't see that we update it during column pruning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, doing this would require us to also pass the table info as an argument to
IndexInfo2PrefixCols()/IndexInfo2Cols()...I'm open to the idea, though. It might mean that we wouldn't need to maintain this mapping after all, which would make things a lot simpler.
I'll try your approach and see how it goes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should be also updating it during column pruning (see lines 201-204 added to
logical_datasource.go), unless I missed a spot.That said, I think you make a good point here: