table, executor: let `SplitIndexValue` work with clustered index table. #53254

Defined2014 · 2024-05-14T08:31:59Z

What problem does this PR solve?

Issue Number: close #49995

Problem Summary: Ref PR #50013, remove insert on duplicate part. /cc @L-maple

What changed and how does it work?

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No need to test
- I checked and no code files have been changed.

Side effects

Performance regression: Consumes more CPU
Performance regression: Consumes more Memory
Breaking backward compatibility

Documentation

Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

None

tiprow · 2024-05-14T08:32:15Z

Hi @Defined2014. Thanks for your PR.

PRs from untrusted users cannot be marked as trusted with /ok-to-test in this repo meaning untrusted PR authors can never trigger tests themselves. Collaborators can still trigger tests on the PR using /test all.

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.

Defined2014 · 2024-05-14T08:33:16Z

/run-check-issue-triage-complete

codecov · 2024-05-14T09:11:29Z

Codecov Report

Attention: Patch coverage is 86.95652% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 75.0451%. Comparing base (a2037fe) to head (abd336d).
Report is 27 commits behind head on master.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #53254        +/-   ##
================================================
+ Coverage   72.4460%   75.0451%   +2.5990%     
================================================
  Files          1493       1499         +6     
  Lines        429361     437121      +7760     
================================================
+ Hits         311055     328038     +16983     
+ Misses        99031      88975     -10056     
- Partials      19275      20108       +833

Flag	Coverage Δ
integration	`49.4781% <86.9565%> (?)`
unit	`71.4707% <82.6087%> (+0.1284%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
dumpling	`53.9957% <ø> (ø)`
parser	`∅ <ø> (∅)`
br	`50.4343% <ø> (+9.0041%)`	⬆️

Defined2014 · 2024-05-14T09:23:35Z

/retest

tiprow · 2024-05-14T09:23:56Z

@Defined2014: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

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.

Defined2014 · 2024-05-14T09:33:08Z

/retest

tiprow · 2024-05-14T09:33:33Z

@Defined2014: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

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.

L-maple · 2024-05-15T06:08:47Z

pkg/table/tables/index.go

@@ -412,15 +412,12 @@ func (c *index) Delete(ctx table.MutateContext, txn kv.Transaction, indexedValue
 		// Only when pid in value equals to c.physicalID, the key can be deleted.
 		if c.idxInfo.Global {


L413~L425 are useless now, which can be deleted. ref: #52442

L-maple · 2024-05-15T06:16:40Z

pkg/tablecodec/tablecodec.go

+}
+
+// splitIndexValueForIndexValueVersion0 splits index value into segments.
+func splitIndexValueForIndexValueVersion0(value []byte) (segs IndexValueSegments) {


not get the name splitIndexValueForIndexValueVersion0 logic behind? this is a extensible index value format.

See

tidb/pkg/tablecodec/tablecodec.go

Line 1408 in 2d552c0

// +-- IndexValueVersion0 (with restore data, or common handle, or index is global)

, we have three different index value types.

L-maple · 2024-05-15T06:38:47Z

SplitIndexValue is really nice as a facade, thumb up for you @Defined2014

mjonss

LGTM, with some questions:

So the default of creating a partitioned table with a global index, will still be non-clustered?
Should we also create a PR for supporting clustered tables for DDL, which seems to be disabled here?

mjonss · 2024-05-15T07:35:08Z

/retest

tiprow · 2024-05-15T07:35:31Z

@mjonss: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

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.

Defined2014 · 2024-05-15T07:41:31Z

So the default of creating a partitioned table with a global index, will still be non-clustered?

The problem is we run set @@tidb_enable_clustered_index='int_only' in mysql-tester, link

Should we also create a PR for supporting clustered tables for DDL, which seems to be disabled here?

This line means we couldn't create an index is global and also is a clustered primary key. Because the table id of a clustered index key should equals to partition id. But if table has a clustered index and want to add an global index, it's ok. I think this line wants to make error message more cleaner.

@mjonss

zimulala

LGTM

Defined2014 · 2024-05-16T04:52:01Z

/retest

tiprow · 2024-05-16T04:52:27Z

@Defined2014: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

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 · 2024-05-16T08:16:05Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mjonss, zimulala

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [mjonss,zimulala]
~~pkg/table/OWNERS~~ [zimulala]
~~pkg/tablecodec/OWNERS~~ [zimulala]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ti-chi-bot · 2024-05-16T08:16:08Z

[LGTM Timeline notifier]

Timeline:

2024-05-16 04:00:23.784201274 +0000 UTC m=+1712177.541336844: ☑️ agreed by zimulala.
2024-05-16 08:16:07.547252836 +0000 UTC m=+1727521.304388410: ☑️ agreed by mjonss.

Defined2014 · 2024-05-16T11:16:26Z

/retest

tiprow · 2024-05-16T11:16:51Z

@Defined2014: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

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.

…e. (pingcap#53254) close pingcap#49995

ti-chi-bot bot added do-not-merge/needs-triage-completed release-note-none size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 14, 2024

ti-chi-bot bot removed the do-not-merge/needs-triage-completed label May 14, 2024

Defined2014 added 3 commits May 14, 2024 20:13

update

f2786c2

update

27892c9

add test cases

0a8a662

Defined2014 force-pushed the fix-50013 branch from 1339cb0 to 0a8a662 Compare May 14, 2024 12:13

add comments

4f10f11

Defined2014 mentioned this pull request May 15, 2024

executor: fix point get under clustered table && insert err for global index #50013

Closed

13 tasks

Defined2014 requested review from tiancaiamao, zimulala and mjonss May 15, 2024 03:03

Defined2014 added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 15, 2024

L-maple reviewed May 15, 2024

View reviewed changes

update

a648370

mjonss reviewed May 15, 2024

View reviewed changes

Defined2014 removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 16, 2024

zimulala approved these changes May 16, 2024

View reviewed changes

ti-chi-bot bot added approved needs-1-more-lgtm labels May 16, 2024

update

abd336d

mjonss approved these changes May 16, 2024

View reviewed changes

ti-chi-bot bot added lgtm and removed needs-1-more-lgtm labels May 16, 2024

ti-chi-bot bot merged commit 2809439 into pingcap:master May 16, 2024
23 checks passed

terry1purcell pushed a commit to terry1purcell/tidb that referenced this pull request May 17, 2024

table, executor: let SplitIndexValue work with clustered index tabl…

196784d

…e. (pingcap#53254) close pingcap#49995

RidRisR pushed a commit to RidRisR/tidb that referenced this pull request May 23, 2024

table, executor: let SplitIndexValue work with clustered index tabl…

2413af9

…e. (pingcap#53254) close pingcap#49995

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

table, executor: let `SplitIndexValue` work with clustered index table. #53254

table, executor: let `SplitIndexValue` work with clustered index table. #53254

Defined2014 commented May 14, 2024

tiprow bot commented May 14, 2024

Defined2014 commented May 14, 2024

codecov bot commented May 14, 2024 •

edited

Defined2014 commented May 14, 2024

tiprow bot commented May 14, 2024

Defined2014 commented May 14, 2024

tiprow bot commented May 14, 2024

L-maple May 15, 2024

Defined2014 May 15, 2024

L-maple May 15, 2024

Defined2014 May 15, 2024

L-maple May 15, 2024

L-maple commented May 15, 2024

mjonss left a comment

mjonss commented May 15, 2024

tiprow bot commented May 15, 2024

Defined2014 commented May 15, 2024 •

edited

zimulala left a comment

Defined2014 commented May 16, 2024

tiprow bot commented May 16, 2024

ti-chi-bot bot commented May 16, 2024

ti-chi-bot bot commented May 16, 2024

Defined2014 commented May 16, 2024

tiprow bot commented May 16, 2024

		@@ -412,15 +412,12 @@ func (c *index) Delete(ctx table.MutateContext, txn kv.Transaction, indexedValue
		// Only when pid in value equals to c.physicalID, the key can be deleted.
		if c.idxInfo.Global {

table, executor: let SplitIndexValue work with clustered index table. #53254

table, executor: let SplitIndexValue work with clustered index table. #53254

Conversation

Defined2014 commented May 14, 2024

What problem does this PR solve?

What changed and how does it work?

Check List

Release note

tiprow bot commented May 14, 2024

Defined2014 commented May 14, 2024

codecov bot commented May 14, 2024 • edited

Codecov Report

Defined2014 commented May 14, 2024

tiprow bot commented May 14, 2024

Defined2014 commented May 14, 2024

tiprow bot commented May 14, 2024

L-maple May 15, 2024

Choose a reason for hiding this comment

Defined2014 May 15, 2024

Choose a reason for hiding this comment

L-maple May 15, 2024

Choose a reason for hiding this comment

Defined2014 May 15, 2024

Choose a reason for hiding this comment

L-maple May 15, 2024

Choose a reason for hiding this comment

L-maple commented May 15, 2024

mjonss left a comment

Choose a reason for hiding this comment

mjonss commented May 15, 2024

tiprow bot commented May 15, 2024

Defined2014 commented May 15, 2024 • edited

zimulala left a comment

Choose a reason for hiding this comment

Defined2014 commented May 16, 2024

tiprow bot commented May 16, 2024

ti-chi-bot bot commented May 16, 2024

ti-chi-bot bot commented May 16, 2024

[LGTM Timeline notifier]

Defined2014 commented May 16, 2024

tiprow bot commented May 16, 2024

table, executor: let `SplitIndexValue` work with clustered index table. #53254

table, executor: let `SplitIndexValue` work with clustered index table. #53254

codecov bot commented May 14, 2024 •

edited

Defined2014 commented May 15, 2024 •

edited