Skip to content
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

*: refactor table.Table interface, clean up unnecessay methods #22430

Merged
merged 15 commits into from
Mar 2, 2021

Conversation

tiancaiamao
Copy link
Contributor

What problem does this PR solve?

The bigger the interface, the weaker the abstraction – Rob Pike

There are so many methods in the Table interface definition, make it less flexible.

What is changed and how it works?

Clean up unnecessary methods from the table.Table interface.

What's Changed:

	// Seek returns the handle greater or equal to h.	
	Seek(ctx sessionctx.Context, h kv.Handle) (handle kv.Handle, found bool, err error)

This one is only used in one place and is in the test code, so remove it.

	// DeletableIndices returns delete-only, write-only and public indices of the table.	
	DeletableIndices() []Index	

This one is not really used.

	// RecordKey returns the key in KV storage for the row.	
	RecordKey(h kv.Handle) kv.Key	

This can be replaced by tablecodec.EncodeRecordKey(RecordPrefix(), h) provide that RecordPrefix() is given.

	// FirstKey returns the first key.	
	FirstKey() kv.Key	

This one is a simple wrap on RecordKey(), it's not necessary to be an interface method.

	// RecordPrefix returns the record key prefix.	
	RecordPrefix() kv.Key	

	// IndexPrefix returns the index key prefix.	
	IndexPrefix() kv.Key	

Those are implementation-related, the interface should be abstract.
We may have temporary table, memory table, partition table, and many more, not all of them have the concept of RecordPrefix

	// RebaseAutoID rebases the auto_increment ID base.		
	// If allocIDs is true, it will allocate some IDs and save to the cache.		
	// If allocIDs is false, it will not allocate IDs.		
	RebaseAutoID(ctx sessionctx.Context, newBase int64, allocIDs bool, tp autoid.AllocatorType) error

We already provide Allocator() method in the Table interface, this one is just a simple wrap on Allocator(), so it's duplicated.

How it Works:

Remove RecordPrefix and IndexPrefix involves a lot of changes, maybe I should do it in another PR.

Related changes

  • PR to update pingcap/docs/pingcap/docs-cn:
  • Need to cherry-pick to the release branch

Check List

Tests

  • No code

Side effects

  • Breaking backward compatibility

API change

Release note

  • Code refactor, clean up the table.Table interface

@github-actions github-actions bot added sig/execution SIG execution sig/sql-infra SIG: SQL Infra labels Jan 19, 2021
@tiancaiamao tiancaiamao marked this pull request as ready for review January 23, 2021 02:28
@tiancaiamao tiancaiamao requested a review from a team as a code owner January 23, 2021 02:28
@tiancaiamao tiancaiamao requested review from lzmhhh123 and removed request for a team January 23, 2021 02:28
@tiancaiamao
Copy link
Contributor Author

IterRecords() method is also moved out from the table.Table interface:

t.IterRecords(..) =>  table.IterRecords(t, ...)

@tiancaiamao
Copy link
Contributor Author

tiancaiamao commented Jan 23, 2021

The remained methods for Table now is:

// Table is used to retrieve and modify rows in table.
type Table interface {
	columnAPI

	// Indices returns the indices of the table.
	Indices() []Index

	// RecordPrefix returns the record key prefix.
	RecordPrefix() kv.Key

	// AddRecord inserts a row which should contain only public columns
	AddRecord(ctx sessionctx.Context, r []types.Datum, opts ...AddRecordOption) (recordID kv.Handle, err error)

	// UpdateRecord updates a row which should contain only writable columns.
	UpdateRecord(ctx context.Context, sctx sessionctx.Context, h kv.Handle, currData, newData []types.Datum, touched []bool) error

	// RemoveRecord removes a row in the table.
	RemoveRecord(ctx sessionctx.Context, h kv.Handle, r []types.Datum) error

	// Allocators returns all allocators.
	Allocators(ctx sessionctx.Context) autoid.Allocators

	// RebaseAutoID rebases the auto_increment ID base.
	// If allocIDs is true, it will allocate some IDs and save to the cache.
	// If allocIDs is false, it will not allocate IDs.
	RebaseAutoID(ctx sessionctx.Context, newBase int64, allocIDs bool, tp autoid.AllocatorType) error

	// Meta returns TableInfo.
	Meta() *model.TableInfo

	// Type returns the type of table
	Type() Type
}

RecordPrefix() kv.Key and RebaseAutoID(ctx sessionctx.Context ...) is not really necessary, but I'll keep it here to make the changes friendly for reviews.

@tiancaiamao tiancaiamao requested a review from bb7133 January 23, 2021 02:35
@xhebox
Copy link
Contributor

xhebox commented Jan 25, 2021

Maybe it is worth to remove writableIndices based on the fact that it is not widely used. A helper method like index.IsWritable would help.

@tiancaiamao
Copy link
Contributor Author

Now WritableIndices() is also removed. @xhebox

table/tables/tables.go Outdated Show resolved Hide resolved
@tiancaiamao
Copy link
Contributor Author

PTAL @xhebox @tangenta

Copy link
Contributor

@xhebox xhebox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rest LGTM

infoschema/perfschema/tables.go Show resolved Hide resolved
infoschema/tables.go Show resolved Hide resolved
@xhebox
Copy link
Contributor

xhebox commented Feb 26, 2021

/lgtm

@ti-chi-bot ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Feb 26, 2021
@ti-chi-bot ti-chi-bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Feb 26, 2021
@ti-chi-bot ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 2, 2021
Copy link
Contributor

@tangenta tangenta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@tangenta
Copy link
Contributor

tangenta commented Mar 2, 2021

/lgtm

@ti-chi-bot
Copy link
Member

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • tangenta
  • xhebox

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.

@ti-chi-bot 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 Mar 2, 2021
@tiancaiamao
Copy link
Contributor Author

/merge

@ti-chi-bot
Copy link
Member

@tiancaiamao: It seems you want to merge this PR, I will help you trigger all the tests:

/run-all-tests

You only need to trigger /merge once, and 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.

If you have any questions about the PR merge process, please refer to pr process.

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.

@ti-chi-bot
Copy link
Member

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

Commit hash: 5781f11

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Mar 2, 2021
@ti-chi-bot
Copy link
Member

@tiancaiamao: 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

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.

@ti-chi-bot ti-chi-bot merged commit 24c9df1 into pingcap:master Mar 2, 2021
@tiancaiamao tiancaiamao deleted the refactor-table branch March 2, 2021 12:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sig/execution SIG execution sig/sql-infra SIG: SQL Infra size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants