-
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
infoschema: introduce MetaOnlyInfoSchema
to provide meta only information schema
#52070
Conversation
Hi @lcwangchao. 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/test-infra repository. |
b879cb9
to
7a5f444
Compare
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #52070 +/- ##
=================================================
- Coverage 72.3876% 54.7236% -17.6641%
=================================================
Files 1485 1599 +114
Lines 365503 613536 +248033
=================================================
+ Hits 264579 335749 +71170
- Misses 81362 254456 +173094
- Partials 19562 23331 +3769
Flags with carried forward coverage won't be shown. Click here to find out more.
|
104e790
to
77c4a18
Compare
pkg/ddl/schematracker/info_store.go
Outdated
@@ -181,3 +181,9 @@ func (i InfoStoreAdaptor) TableByName(schema, table model.CIStr) (t table.Table, | |||
} | |||
return tables.MockTableFromMeta(tableInfo), nil | |||
} | |||
|
|||
// TableInfoByName implements the InfoSchema interface. | |||
// nolint:unused |
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.
public function will not trigger "unused" linter?
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.
I'm not sure, just copied from exists method like TableByName
...
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.
How about trying to remove it?
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.
ok, I'll try to remove it
[LGTM Timeline notifier]Timeline:
|
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
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lance6716, tangenta, tiancaiamao, windtalker 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:
Approvers can indicate their approval by writing |
What problem does this PR solve?
Issue Number: close #52072
Problem Summary:
The API of
infoschema.InfoSchema
introduces too many dependencies. The main reason is that it not only provides "meta" of tables but also returnstable.Table
for table mutations. It brings two problems.The first problem is the dependency cycle.
infoschema.InfoSchema
is a basic lib and is required by many other packages. This may cause a dependency cycle easily.Another problem is that, if we want to break the dependency cycle. We have to use some "magic ways". For example, return
infoschema.InfoSchemaVersion
instead ofinfoschema.InfoSchema
in function signature but cast it toinfoschema.InfoSchema
when using it. However, the castingis.(infoschema.InfoSchema)
is ugly and may lost the benefit of static checks in compile phase.What changed and how does it work?
In this PR, we extended
infoschema.InfoschemaVersion
toinfoschema.MetaOnlyInfoSchema
to provide more common use methods in API and avoid importing thetable
package there. In most scenes, we only need the table meta and do not want to modify it, soinfoschema.MetaOnlyInfoSchema
is enough.We also added some methods to provide table meta only:
Check List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.