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.

Sign up for GitHub

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

Jump to bottom

executor: let information_schema be the first database in ShowDatabases #7938

Merged
merged 3 commits into from
Oct 23, 2018
Merged

executor: let information_schema be the first database in ShowDatabases #7938

merged 3 commits into from
Oct 23, 2018

Conversation

parastorli
Copy link
Contributor

@parastorli parastorli commented Oct 18, 2018 •
edited
Loading

What problem does this PR solve?

show databases;
INFORMATION_SCHEMA is not first database

What is changed and how it works?

Add one function to replace INFORMATION_SCHEMA to the first database if the first database is not INFORMATION_SCHEMA

Check List

Tests

  • Unit test
  • Integration test

@sre-bot
Copy link
Contributor

sre-bot commented Oct 18, 2018

Hi contributor, thanks for your PR.

This patch needs to be approved by someone of admins. They should reply with "/ok-to-test" to accept this PR for running test automatically.

@CLAassistant
Copy link

CLAassistant commented Oct 18, 2018 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@shenli
Copy link
Member

shenli commented Oct 18, 2018

@parastorli Thanks! Please make the description clear and add a test case.

@shenli
Copy link
Member

shenli commented Oct 18, 2018

/ok-to-test

@shenli shenli added contribution This PR is from a community contributor. type/compatibility labels Oct 18, 2018
@shenli
Copy link
Member

shenli commented Oct 18, 2018

/run-all-tests

@parastorli
Copy link
Contributor Author

parastorli commented Oct 18, 2018 •
edited
Loading

@shenli, Thanks.
make the description clear, is about commit or comment? please further point
please give me a link about how to add test and run the unit test to the tidb

@shenli
Copy link
Member

shenli commented Oct 18, 2018

I mean the checklist part. Please remove the items that are not applicable.

@parastorli parastorli force-pushed the feature_information_first_db branch 2 times, most recently from bf3cb6d to 0640a22 Compare October 18, 2018 06:02
@parastorli
Copy link
Contributor Author

parastorli commented Oct 18, 2018 •
edited
Loading

@shenli
Make the checklist clear and add the test case.

zz-jason
zz-jason reviewed Oct 18, 2018
View reviewed changes
executor/show.go Outdated Show resolved Hide resolved
zz-jason
zz-jason reviewed Oct 18, 2018
View reviewed changes
executor/show.go Outdated Show resolved Hide resolved
@parastorli
Copy link
Contributor Author

@ngaut
@zz-jason
@shenli
please review the new pr.

zz-jason
zz-jason reviewed Oct 18, 2018
View reviewed changes
executor/show.go Outdated Show resolved Hide resolved
executor/show.go Outdated Show resolved Hide resolved
executor/show.go Outdated Show resolved Hide resolved
@parastorli
Copy link
Contributor Author

parastorli commented Oct 18, 2018 •
edited
Loading

@zz-jason
I want to replace to copy(dbs[1:i+1], dbs[0:i]) before. but the i+1 in the dbs[1:i+1] is not in the work Interval, it is dangerous, so I changed to the code in the file, but it is ugly.

@parastorli parastorli force-pushed the feature_information_first_db branch 3 times, most recently from 88cc66e to ad28773 Compare October 18, 2018 14:01
@parastorli
Copy link
Contributor Author

@zz-jason, please review the new pr.

zz-jason
zz-jason reviewed Oct 18, 2018
View reviewed changes
Copy link
Member

@zz-jason zz-jason left a comment

Choose a reason for hiding this comment

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

LGTM

@zz-jason
Copy link
Member

@XuHuaiyu PTAL

@zz-jason zz-jason added sig/execution SIG execution status/LGT1 Indicates that a PR has LGTM 1. labels Oct 18, 2018
XuHuaiyu
XuHuaiyu reviewed Oct 19, 2018
View reviewed changes
Copy link
Contributor

@XuHuaiyu XuHuaiyu left a 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.
Addressing the comments makes this a LGTM~

executor/show_test.go Show resolved Hide resolved
executor/show_test.go Outdated Show resolved Hide resolved
executor/show.go Outdated
@@ -166,11 +166,28 @@ func (e *ShowExec) fetchShowEngines() error {
return nil
}

// move information_schema to the first, and the other are sorted in the origin ascending order.
Copy link
Contributor

Choose a reason for hiding this comment

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

s/ move/ moveInfoSchemaToFront moves
s/ the other/ the others

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good!

executor/show.go Outdated Show resolved Hide resolved
winoros
winoros reviewed Oct 19, 2018
View reviewed changes
executor/show.go Outdated Show resolved Hide resolved
@parastorli parastorli force-pushed the feature_information_first_db branch 3 times, most recently from 6801fa2 to 5808764 Compare October 19, 2018 09:21
@parastorli
Copy link
Contributor Author

@XuHuaiyu
The new unit test is added in the executor/pkg_test.go

@parastorli parastorli force-pushed the feature_information_first_db branch 9 times, most recently from 99b78e2 to 489fc71 Compare October 23, 2018 00:44
@parastorli
Copy link
Contributor Author

@XuHuaiyu
@winoros
PTAL

winoros
winoros reviewed Oct 23, 2018
View reviewed changes
Copy link
Member

@winoros winoros left a comment

Choose a reason for hiding this comment

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

lgtm

@zz-jason zz-jason added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Oct 23, 2018
@zz-jason
Copy link
Member

/run-all-tests

@parastorli
Copy link
Contributor Author

parastorli commented Oct 23, 2018 •
edited
Loading

@zz-jason
jenkins-ci-tidb/integration-compatibility-test and jenkins-ci-tidb/integration-ddl-test are failed,
but they are not related to the pr，why?

@XuHuaiyu
Copy link
Contributor

Hi, @parastorli
It has nothing to do with this PR, we are working on it.

@ngaut ngaut merged commit e81f855 into pingcap:master Oct 23, 2018
iamzhoug37 pushed a commit to iamzhoug37/tidb that referenced this pull request Oct 25, 2018
@iamzhoug37
fix issue 7518
f15bb5c 
bugfix fixed pingcap#7518

expression: MySQL compatible current_user function (pingcap#7801)

plan: propagate constant over outer join (pingcap#7794)

- extract `outerCol = const` from join conditions and filter conditions,
  substitute `outerCol` in join conditions with `const`;
- extract `outerCol = innerCol` from join conditions, derive new join
  conditions based on this column equal condition and `outerCol` related
  expressions in join conditions and filter conditions;

util/timeutil: fix data race caused by forgetting set stats lease to 0 (pingcap#7901)

stats: handle ddl event for partition table (pingcap#7903)

plan: implement Operand and Pattern of cascades planner. (pingcap#7910)

planner: not convert to TableDual if empty range is derived from deferred constants (pingcap#7808)

plan: move projEliminate behind aggEliminate (pingcap#7909)

admin: fix admin check table bug of byte compare (pingcap#7887)

* admin: remove reflect deepEqual

stats: fix panic caused by empty histogram (pingcap#7912)

plan: fix panic caused by empty schema of LogicalTableDual (pingcap#7906)

* fix drop view if exist error (pingcap#7833)

executor: refine `explain analyze` (pingcap#7888)

executor: add an variable to compatible with MySQL insert for OGG (pingcap#7863)

expression: maintain `DeferredExpr` in aggressive constant folding. (pingcap#7915)

stats: fix histogram boundaries overflow error (pingcap#7883)

ddl:support the definition of `null` change to `not null` using `alter table` (pingcap#7771)

* ddl:support the definition of null change to not null using alter table

ddl: add check when create table with foreign key. (pingcap#7885)

* ddl: add check when create table with foreign key

planner: eliminate if null on non null column (pingcap#7924)

executor: fix a bug in point get (pingcap#7934)

planner, executor: refine ColumnPrune for LogicalUnionAll (pingcap#7930)

executor: fix panic when limit is too large (pingcap#7936)

ddl: add TiDB version to metrics (pingcap#7902)

stats: limit the length of sample values (pingcap#7931)

vendor: update tipb (pingcap#7893)

planner: support the Group and GroupExpr for the cascades planner (pingcap#7917)

store/tikv: log more information when other err occurs (pingcap#7948)

types: fix date time parse (pingcap#7933)

ddl: just print error message when ddl job is normal to calcel, to eliminate noisy log (pingcap#7875)

stats: update delta info for partition table (pingcap#7947)

explaintest: add explain test for partition pruning (pingcap#7505)

util: move disjoint set to util package (pingcap#7950)

util: add PreAlloc4Row and Insert for Chunk and List (pingcap#7916)

executor: add the slow log for commit (pingcap#7951)

expression: add builtin json_keys (pingcap#7776)

privilege: add USAGE in `show grants` for mysql compatibility (pingcap#7955)

ddl: fix invailid ddl job panic (pingcap#7940)

*: move ast.NewValueExpr to standalone parser_driver package (pingcap#7952)

Make the ast package get rid of the dependency of types.Datum

server: allow cors http request (pingcap#7939)

*: move `Statement` and `RecordSet` from ast to sqlexec package (pingcap#7970)

pr suggestion update

executor/aggfuncs: split unit tests to corresponding file (pingcap#7993)

store/tikv: fix typo (pingcap#7990)

executor, planner: clone proj schema for different children in buildProj4Union (pingcap#7999)

executor: let information_schema be the first database in ShowDatabases (pingcap#7938)

stats: use local feedback for partition table (pingcap#7963)

executor: add unit test for aggfuncs (pingcap#7966)

server: add log for binary execute statement (pingcap#7987)

admin: refine admin check decoder (pingcap#7862)

executor: improve wide table insert & update performance (pingcap#7935)

ddl: fix reassigned partition id in `truncate table` does not take effect (pingcap#7919)

fix reassigned partition id in truncate table does not take effect

add changelog for 2.1.0 rc4 (pingcap#8020)

*: make parser package dependency as small as possible (pingcap#7989)

parser: support `:=` in the `set` syntax (pingcap#8018)

According to MySQL document, `set` use the = assignment operator,
but the := assignment operator is also permitted

stats: garbage collect stats for partition table (pingcap#7962)

docs: add the proposal for the column pool (pingcap#7988)

expression: refine built-in func truncate to support uint arg (pingcap#8000)

stats: support show stats for partition table (pingcap#8023)

stats: update error rate for partition table (pingcap#8022)

stats: fix estimation for out of range point queries (pingcap#8015)

*: move parser to a separate repository (pingcap#8036)

executor: fix wrong result when index join on union scan. (pingcap#8031)

Do not modify Plan of dataReaderBuilder directly, because it would
impact next batch of outer rows, as well as other concurrent inner
workers. Instead, build a local child builder to store the child plan.

planner: fix a panic of a cached prepared statement with IndexScan (pingcap#8017)

*: fix the issue of executing DDL after executing SQL failure in txn (pingcap#8044)

* ddl, executor: fix the issue of executing DDL after executing SQL failure in txn

add unit test

remove debug info

add like evaluator case sensitive test

ddl, domain: make schema correct after canceling jobs (pingcap#7997)

unit test fix

code format

proposal: maintaining histograms in plan. (pingcap#7605)

support _tidb_rowid for table scan range (pingcap#8047)

var rename fix
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ngaut ngaut ngaut left review comments

@winoros winoros winoros left review comments

@XuHuaiyu XuHuaiyu XuHuaiyu left review comments

@zz-jason zz-jason zz-jason approved these changes

Assignees
No one assigned
Labels
contribution This PR is from a community contributor. sig/execution SIG execution status/LGT2 Indicates that a PR has LGTM 2. type/compatibility
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@parastorli @sre-bot @CLAassistant @shenli @zz-jason @XuHuaiyu @ngaut @winoros