-
Notifications
You must be signed in to change notification settings - Fork 5.9k
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: fix bug when pruning columns for TableDual #11054
Conversation
/run-all-tests |
/run-common-test |
Codecov Report
@@ Coverage Diff @@
## master #11054 +/- ##
===========================================
Coverage 81.2821% 81.2821%
===========================================
Files 423 423
Lines 90133 90133
===========================================
Hits 73262 73262
Misses 11571 11571
Partials 5300 5300 |
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.
LGTM
/run-all-tests tidb-test=pr/847 |
@@ -262,8 +262,15 @@ func (p *LogicalTableDual) PruneColumns(parentUsedCols []*expression.Column) err | |||
} | |||
} | |||
for k, cols := range p.schema.TblID2Handle { | |||
if p.schema.ColumnIndex(cols[0]) == -1 { | |||
for i := len(cols) - 1; i >= 0; i-- { |
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.
The content of TblID2Handle
looks weird for the above test case, i.e, t1.a
and t2.a
are all stored in single bucket of TblID2Handle[70] in my instance for example. Though t1
and t2
are two aliases for same table t
, actually they should be 2 separate tables in the query. Would it have risk in executor when using TblID2Handle
like this?
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.
Is any situation that the handle has not only one column? I think TblID2Handle
should be the map<id, *Column>
.
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.
Anyway. This is one of the chaos of Schema
. Maybe temporarily fix bugs is OK.
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.
@lzmhhh123 @eurekaka
The only case that one table id maps to multiple Column is multi-table delete.
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.
LGTM. btw, should other logical plans in pruning columns need to be changed?
@lzmhhh123 Currently, no. We can move this thing out of schema ASAP. |
What problem does this PR solve?
Dual may not be from only one data source.
So check
cols[0]
is not enough.What is changed and how it works?
Check the full slice.
Check List
Tests
Related changes