-
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, executor: push limit down into IndexLookUpReader executor #12262
Conversation
Codecov Report
@@ Coverage Diff @@
## master #12262 +/- ##
===========================================
Coverage 81.1502% 81.1502%
===========================================
Files 454 454
Lines 99662 99662
===========================================
Hits 80876 80876
Misses 12972 12972
Partials 5814 5814 |
/run-all-tests |
This pr seems to do some query-rewrite in cbo pahse? |
@jingyugao Yes, it is query-rewrite to some extent, like the operator pushdown (to storage layer) here. |
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
@@ -484,9 +487,9 @@ func (e *IndexLookUpExecutor) startIndexWorker(ctx context.Context, kvRanges []k | |||
} | |||
if e.runtimeStats != nil { | |||
copStats := e.ctx.GetSessionVars().StmtCtx.RuntimeStatsColl.GetRootStats(e.idxPlans[len(e.idxPlans)-1].ExplainID().String()) | |||
copStats.SetRowNum(count) | |||
copStats.SetRowNum(int64(count)) |
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.
Not related to this PR, but it seems really strange to use int64
instead of uint64
for a count
.
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.
@qw4990 Is there any specific reason why we are using int
/int64
for SetRequiredRows
/SetRowNum
respectively?
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.
For RowNum
, we use methods like atomic.XXXInt64
to maintain it so its type is int64
.
And for RequiredRows
we maintain it using Golang statement and for convenience, we set its type to int
.
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
Your auto merge job has been accepted, waiting for 12331, 12332, 12325 |
/run-all-tests |
/run-all-tests |
cherry pick to release-3.1 failed |
cherry pick to release-3.0 failed |
cherry pick to release-2.1 failed |
…ingcap#12262) Conflicts: cmd/explaintest/r/explain_easy.result cmd/explaintest/r/explain_easy_stats.result planner/core/integration_test.go planner/core/physical_plans.go planner/core/testdata/plan_suite_out.json
…ingcap#12262) Conflicts: cmd/explaintest/r/explain_easy.result cmd/explaintest/r/explain_easy_stats.result planner/core/integration_test.go planner/core/physical_plans.go planner/core/testdata/plan_suite_out.json
…ingcap#12262) Conflicts: cmd/explaintest/r/explain_easy.result cmd/explaintest/r/explain_easy_stats.result planner/core/integration_test.go planner/core/physical_plans.go planner/core/testdata/plan_suite_out.json executor/distsql.go executor/distsql_test.go
What problem does this PR solve?
Fix #10668
What is changed and how it works?
Sink
PhysicalLimit
on top ofIndexLookUpReader
intoIndexLookUpReader
, and in executor, skip the preceding and tailing handles returned byIndexPlan
. This optimization can avoid unnecessaryTableScan
for handles not satisfyingLimit / Offset
constraint, and improve execution performance especially for queries with largeOffset
.Check List
Tests
Code changes
Side effects
Related changes
IndexLookUpReader
'sTablePlan / IndexPlan
since some unnecessary I/O and network cost is avoided. This can be done in the next PR.