Planner: Run full partition pruning if convertToPointGet converted keys to SortKey #59918
Conversation
|
Skipping CI for Draft Pull Request. |
ti-chi-bot
bot
added
do-not-merge/needs-triage-completed
release-note-none
|
Hi @mjonss. Thanks for your PR. PRs from untrusted users cannot be marked as trusted with I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/test all |
|
@mjonss: Cannot trigger testing until a trusted user reviews the PR and leaves an In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
ti-chi-bot
bot
added
size/L
|
/retest |
|
@mjonss: Cannot trigger testing until a trusted user reviews the PR and leaves an In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #59918 +/- ##
================================================
+ Coverage 72.9654% 73.2498% +0.2844%
================================================
Files 1699 1703 +4
Lines 469477 475179 +5702
================================================
+ Hits 342556 348068 +5512
+ Misses 105828 105779 -49
- Partials 21093 21332 +239
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
KEY partitioning does not seem to be affected, since it is using |
ti-chi-bot
bot
added
size/XL
ti-chi-bot
bot
removed
the
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
This PR also fixes other issues. Like using diff --git a/pkg/planner/core/point_get_plan.go b/pkg/planner/core/point_get_plan.go
index 030fd88139..671522f454 100644
--- a/pkg/planner/core/point_get_plan.go
+++ b/pkg/planner/core/point_get_plan.go
@@ -348,6 +348,9 @@ func (p *PointGetPlan) LoadTableStats(ctx sessionctx.Context) {
// error
// TODO: Also supporting BatchPointGet? Problem is that partition ID must be mapped to handle/IndexValue.
func needsPartitionPruning(sctx sessionctx.Context, tblInfo *model.TableInfo, pt table.PartitionedTable, dbName string, indexInfo *model.IndexInfo, indexCols []*expression.Column, indexValues []types.Datum, conds []expression.Expression, partitionNames []ast.CIStr) ([]int, bool, error) {
+ if tblInfo != nil {
+ return nil, false, nil
+ }
for i := range indexValues {
if tblInfo.Columns[indexInfo.Columns[i].Offset].FieldType.EvalType() != types.ETString ||
indexValues[i].Collation() == tblInfo.Columns[indexInfo.Columns[i].Offset].GetCollate() {I get this stack trace: |
There was a problem hiding this comment.
PR Overview
This PR refines partition pruning for point get and batch point get plans by propagating a new case‐sensitivity flag through various test and plan functions and restructuring error returns. Key changes include:
- Propagation of an isCaseSensitive flag and corresponding adjustments in functions like getRowData and prepared/non-prepared statement functions.
- Updates to partition pruning logic in point_get_plan.go including introducing needsPartitionPruning to handle SortKey conversion.
- Enhancements in executor functions to properly handle error returns from the revised pruning functions.
Reviewed Changes
| File | Description |
|---|---|
| pkg/planner/core/plan_cache_partition_table_test.go | Updated test helper functions to support case sensitivity adjustments and apply collation settings. |
| pkg/planner/core/point_get_plan.go | Revised partition pruning logic and error handling; introduced needsPartitionPruning. |
| pkg/server/conn.go | Modified prefetchPointPlanKeys to handle new error returns from partition pruning calls. |
| pkg/executor/point_get.go | Adjusted buildPointGet to consider the new return types from pruning functions. |
| pkg/planner/core/find_best_task.go | Updated comments regarding the inability to support BatchPointGet when keys are converted. |
| pkg/executor/builder.go | Updated handling of BatchPointGet plans by checking for errors from the pruning phase. |
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
pkg/planner/core/plan_cache_partition_table_test.go:620
- Verify that changing the threshold from 't' to 'x' for filtering string keys in getRowData is intentional, as this adjustment alters which rows may be skipped.
if isCaseSensitive { if x >= "x" {
What problem does this PR solve?
Issue Number: close #59827
Problem Summary:
TODO: Also apply same change and tests for BatchPointGet
What changed and how does it work?
Check List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.