-
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
executor: Add Global to SHOW INDEX output | tidb-test=pr/2391 #56028
executor: Add Global to SHOW INDEX output | tidb-test=pr/2391 #56028
Conversation
Welcome @samba-rgb! |
Hi @samba-rgb. Thanks for your PR. I'm waiting for a pingcap member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the 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. |
Hi @samba-rgb. 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. |
/ok-to-test |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #56028 +/- ##
=================================================
- Coverage 72.9003% 56.9314% -15.9689%
=================================================
Files 1604 1783 +179
Lines 446755 640621 +193866
=================================================
+ Hits 325686 364715 +39029
- Misses 101023 250956 +149933
- Partials 20046 24950 +4904
Flags with carried forward coverage won't be shown. Click here to find out more.
|
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.
Looks good, with some comments.
There are also more tests that needs to update (since the SHOW INDEX output changed).
You can run all unit tests with make dev
and you can run integration tests by first building the tidb-server
binary with make
and then (cd tests/integrationtest ; ./run-tests.sh -s ../../bin/tidb-server)
pkg/executor/show.go
Outdated
isPartitioned := pi != nil && pi.Type != pmodel.PartitionTypeNone | ||
|
||
if isPartitioned && e.Ctx().GetSessionVars().EnableGlobalIndex { | ||
isGlobalIndex = "YES" |
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.
If PKIsHandle, then it is by definition a LOCAL index, i.e. each partition will have the clustered index, and the PK column must be a part of the partitioning expression.
Test by:
set tidb_enable_global_index = 0;
create table t (a int primary key clustered, b int) partition by hash (a) partitions 2;
show create table t;
show index from t;
set tidb_enable_global_index = 1;
show index from t;
pkg/executor/show.go
Outdated
@@ -822,6 +832,7 @@ func (e *ShowExec) fetchShowIndex() error { | |||
"YES", // Index_visible | |||
nil, // Expression | |||
"YES", // Clustered | |||
isGlobalIndex, // Global_index |
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.
So here you can just use "NO"
isGlobalIndex, // Global_index | |
"NO", // Global_index |
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.
Ah, sorry, I had not indented it correctly :(
@samba-rgb I can fix the idc-jenkins-ci-tidb/mysql-test test failures. Can you fix the other failed tests? |
@mjonss Thank you for helping with my SQL test fix. I will now resume working on the fixes for integration and unit tests, along with addressing the comments mentioned above. |
35ba52f
to
75e0e81
Compare
Hi @mjonss i added the Global column in the integrationtest/r//.result path BTW when i ran |
@samba-rgb I assume you are using a mac? md5sum is md5 in macOs..., you can also run only the unit tests with I will try to find which unit tests still needing to be updated. |
pkg/ddl/db_change_test.go:857 To run the test you must also enable failpoints, |
75e0e81
to
efbca50
Compare
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.
Thank you for your contribution! Approved.
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lance6716, mjonss, winoros 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?
No info about index if its global or not , added in show index in this pr
Issue Number: close #55452
Problem Summary:
What changed and how does it work?
added Global index from index info
Check List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.