planner, executor: support batchget for range and list partition table #24856

zhuo-zhi · 2021-05-24T05:07:03Z

What problem does this PR solve?

Issue Number: close #24476, subitem of #24150

Problem Summary:

Support batchget for range and list partition table.

What is changed and how it works?

What's Changed:

Change BatchGet logic in the func TryFastPlan. Change executor logic for BatchGet accordingly.

How it Works:

Allow partition table other than hash partition to support BatchGet. Locate partition column in the func TryFastPlan, and locate partition table ID in the func Next in the executor accordingly.

Check List

Tests

Unit test

Side effects

N/A

Release note

planner, executor: support batchget for range and list partition table

…titionBatchGet

executor/batch_point_get.go

planner/core/point_get_plan.go

executor/partition_table_test.go

qw4990 · 2021-05-26T03:34:39Z

Almost LGTM, please address my small comments at your convenience @zhuo-zhi

mjonss

Some minor questions/suggestions

mjonss · 2021-05-26T09:22:42Z

executor/batch_point_get.go

-			physID := getPhysID(e.tblInfo, idxVals[e.partPos].GetInt64())
+			physID, err := getPhysID(e.tblInfo, e.partExpr, idxVals[e.partPos].GetInt64())
+			if err != nil {
+				continue


Does it continue instead of return err only because getPhysID() only returns an error when there is no matching partition? I.e. would use TableDual?

As for BatchGet, if one data point in the where clause where col in (....) doesn't match any partition, we can just safely ignore it, and continue trying to fetch the next data point.

TableDual is not applicable here because we have already built our executor in buildBatchPointGet instead of buildTableDual.

mjonss · 2021-05-26T09:37:33Z

executor/batch_point_get.go

+		unsigned := mysql.HasUnsignedFlag(col.GetType().Flag)
+		ranges := partitionExpr.ForRangePruning
+		length := len(ranges.LessThan)
+		partIdx := sort.Search(length, func(i int) bool {


This seems to be used in several places (like getPartitionInfo in planner/core/point_get_plan.go), could it be moved to its own function? (i.e. searching and finding the range partition?)
Maybe refactor/break out the same code from locateRangePartition?

Yes, now it's more like an ad hoc solution. Maybe we can refactor our code after partition by range columns(...) and partition by list columns(...) GA.

mjonss · 2021-05-26T09:59:48Z

planner/core/point_get_plan.go

+			return i, nil
+		}
+	}
+	panic("unique index must include all partition columns")


There are no hint in the name of the function or comment that it only applies to Unique indexes, could you add that to the comment above the function?

Ok. It's updated in the new commit.

mjonss · 2021-05-27T20:14:19Z

/lgtm (but I'm not yet in the reviewer list)

rebelice

/lgtm

ti-chi-bot · 2021-05-28T09:48:52Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

qw4990
rebelice

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by writing /lgtm in a comment.
Reviewer can cancel approval by writing /lgtm cancel in a comment.

rebelice · 2021-05-28T09:49:06Z

/merge

ti-chi-bot · 2021-05-28T09:49:09Z

This pull request has been accepted and is ready to merge.

Commit hash: 0dd4e39

qw4990 · 2021-05-28T10:01:36Z

/merge

ti-chi-bot · 2021-05-28T12:53:47Z

@zhuo-zhi: Your PR was out of date, I have automatically updated it for you.

At the same time I will also trigger all tests for you:

/run-all-tests

If the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

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 ti-community-infra/tichi repository.

sre-bot · 2021-05-28T12:54:09Z

Please follow PR Title Format:

pkg [, pkg2, pkg3]: what's changed

Or if the count of mainly changed packages are more than 3, use

*: what's changed

support batchget for range and list partition table

bf185b5

zhuo-zhi requested review from a team as code owners May 24, 2021 05:07

zhuo-zhi requested review from XuHuaiyu and removed request for a team May 24, 2021 05:07

ti-chi-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 24, 2021

refine comment

8a7613a

github-actions bot added the sig/execution SIG execution label May 24, 2021

zhuo-zhi and others added 3 commits May 24, 2021 14:07

bugfix: convertToBatchPointGet add PartitionExpr

6873d7a

bugfix: getPartitionExpr add type assertion

b198762

Merge branch 'master' into partitionBatchGet

b71ce7f

qw4990 requested review from rebelice, mjonss and qw4990 May 24, 2021 07:02

zhuo-zhi added 2 commits May 24, 2021 15:16

bugfix: convertToBatchPointGet partitionExpr lost

a389bc9

Merge branch 'partitionBatchGet' of github.com:zhuo-zhi/tidb into par…

ba77294

…titionBatchGet

ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 24, 2021

Merge branch 'master' into partitionBatchGet

d738c4f

ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 25, 2021

zhuo-zhi added 2 commits May 25, 2021 15:37

Merge branch 'master' into partitionBatchGet

49ee2d7

remove unused imported package

fb6c662

qw4990 reviewed May 26, 2021

View reviewed changes

executor/batch_point_get.go Outdated Show resolved Hide resolved

qw4990 reviewed May 26, 2021

View reviewed changes

planner/core/point_get_plan.go Show resolved Hide resolved

qw4990 reviewed May 26, 2021

View reviewed changes

planner/core/point_get_plan.go Show resolved Hide resolved

qw4990 reviewed May 26, 2021

View reviewed changes

executor/partition_table_test.go Show resolved Hide resolved

add type assertion in getPhysID, add an extra test case

7874c17

qw4990 approved these changes May 26, 2021

View reviewed changes

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label May 26, 2021

mjonss reviewed May 26, 2021

View reviewed changes

refine comment

0dd4e39

qw4990 added the sig/planner SIG: Planner label May 28, 2021

rebelice approved these changes May 28, 2021

View reviewed changes

ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels May 28, 2021

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label May 28, 2021

Merge branch 'master' into partitionBatchGet

7293a16

ti-chi-bot added 2 commits May 28, 2021 20:33

Merge branch 'master' into partitionBatchGet

49b27b8

Merge branch 'master' into partitionBatchGet

d9931cf

ti-chi-bot merged commit ac04473 into pingcap:master May 28, 2021

This was referenced Jun 7, 2021

Partition table didn't support BatchPointGet #21847

Closed

sig-planner: nominate zhuo-zhi to a sig-planner reviewer pingcap/community#466

Closed

This was referenced Jun 17, 2021

the prepare statement result is wrong #25527

Closed

planner: forbid BatchPointGet on tables partitioned by compound expressions #25538

Merged

ti-srebot mentioned this pull request Jun 18, 2021

planner: forbid BatchPointGet on tables partitioned by compound expressions (#25538) #25543

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

planner, executor: support batchget for range and list partition table #24856

planner, executor: support batchget for range and list partition table #24856

zhuo-zhi commented May 24, 2021 •

edited

Loading

qw4990 commented May 26, 2021

mjonss left a comment

mjonss May 26, 2021

zhuo-zhi May 26, 2021 •

edited

Loading

mjonss May 26, 2021

zhuo-zhi May 26, 2021

mjonss May 26, 2021

zhuo-zhi May 26, 2021

mjonss commented May 27, 2021

rebelice left a comment

ti-chi-bot commented May 28, 2021

rebelice commented May 28, 2021

ti-chi-bot commented May 28, 2021

qw4990 commented May 28, 2021

ti-chi-bot commented May 28, 2021

sre-bot commented May 28, 2021

planner, executor: support batchget for range and list partition table #24856

planner, executor: support batchget for range and list partition table #24856

Conversation

zhuo-zhi commented May 24, 2021 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

qw4990 commented May 26, 2021

mjonss left a comment

Choose a reason for hiding this comment

mjonss May 26, 2021

Choose a reason for hiding this comment

zhuo-zhi May 26, 2021 • edited Loading

Choose a reason for hiding this comment

mjonss May 26, 2021

Choose a reason for hiding this comment

zhuo-zhi May 26, 2021

Choose a reason for hiding this comment

mjonss May 26, 2021

Choose a reason for hiding this comment

zhuo-zhi May 26, 2021

Choose a reason for hiding this comment

mjonss commented May 27, 2021

rebelice left a comment

Choose a reason for hiding this comment

ti-chi-bot commented May 28, 2021

rebelice commented May 28, 2021

ti-chi-bot commented May 28, 2021

qw4990 commented May 28, 2021

ti-chi-bot commented May 28, 2021

sre-bot commented May 28, 2021

zhuo-zhi commented May 24, 2021 •

edited

Loading

zhuo-zhi May 26, 2021 •

edited

Loading