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

feat: support show database and show tables #78

Merged
merged 13 commits into from Jul 7, 2022
Merged

Conversation

dust1
Copy link
Contributor

@dust1 dust1 commented Jul 2, 2022

Which issue does this PR close?

Closes #66

Rationale for this change

What changes are included in this PR?

In order to satisfy the current three Show statements: show create table, show tables and show database, I modified the Plan part where show create table is located and combined it into a ShowPlan.

Are there any user-facing changes?

For users, two new sql statements are added to find schema and table in ceresDB.

Effect

SHOW TABLES

First, use show tables to view existing tables:
show tables;
image
Next, use create table sql to create a new table:

CREATE TABLE `hello_show` (`name` string TAG, `value` double NOT NULL, `t` timestamp NOT NULL, TIMESTAMP KEY(t)) ENGINE=Analytic with (enable_ttl='false');

image
Last view effect
show tables;
image

SHOW DATABASE

exec show database;
image

How does this change test

add 2 unit test:

  1. test_show_tables_statement_to_plan
  2. test_show_database_statement_to_plan

Copy link
Member

@ShiKaiWi ShiKaiWi left a comment

Choose a reason for hiding this comment

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

Maybe ShowDatabases is a better match to ShowTables compared with ShowDatabase.

interpreters/src/show.rs Outdated Show resolved Hide resolved
sql/src/parser.rs Outdated Show resolved Hide resolved
sql/src/parser.rs Outdated Show resolved Hide resolved
sql/src/planner.rs Outdated Show resolved Hide resolved
sql/src/planner.rs Outdated Show resolved Hide resolved
interpreters/src/show.rs Outdated Show resolved Hide resolved
interpreters/src/show.rs Outdated Show resolved Hide resolved
interpreters/src/show.rs Outdated Show resolved Hide resolved
interpreters/src/show.rs Outdated Show resolved Hide resolved
@ShiKaiWi
Copy link
Member

ShiKaiWi commented Jul 4, 2022

@dust1 Thanks a lot. You can check our comment reviews if you have time. No need in a hurry. 😜

@dust1
Copy link
Contributor Author

dust1 commented Jul 5, 2022

@ShiKaiWi hello, I have done all modifications, but the weird thing is that my workflow is canceled, can you reboot? I remember that the reviewer can do this

@ShiKaiWi
Copy link
Member

ShiKaiWi commented Jul 5, 2022

@ShiKaiWi hello, I have done all modifications, but the weird thing is that my workflow is canceled, can you reboot? I remember that the reviewer can do this

Sorry for that. Actually, a short timeout (30m) is set by #76 which may lead to a failed ci. And @waynexia will adjust the timeout to 1h.

interpreters/src/show.rs Outdated Show resolved Hide resolved
interpreters/src/create.rs Outdated Show resolved Hide resolved
interpreters/src/create.rs Outdated Show resolved Hide resolved
@waynexia waynexia mentioned this pull request Jul 5, 2022
@waynexia
Copy link
Member

waynexia commented Jul 5, 2022

@ShiKaiWi hello, I have done all modifications, but the weird thing is that my workflow is canceled, can you reboot? I remember that the reviewer can do this

Sorry for that. Actually, a short timeout (30m) is set by #76 which may lead to a failed ci. And @waynexia will adjust the timeout to 1h.

Sorry for the inconvenience, I've adjust the threshold in #84

Copy link
Member

@ShiKaiWi ShiKaiWi left a comment

Choose a reason for hiding this comment

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

@dust1 Basically, this PR is good enough for merge expect some issues about naming. For example, it seems there are some ShowDatabse or show_database and maybe it is better to use ShowDatabases and show_databases which matches well with ShowTables and show_tables.

interpreters/src/show.rs Outdated Show resolved Hide resolved
interpreters/src/show.rs Outdated Show resolved Hide resolved
interpreters/src/show.rs Outdated Show resolved Hide resolved
sql/src/parser.rs Show resolved Hide resolved
@dust1
Copy link
Contributor Author

dust1 commented Jul 6, 2022

@waynexia hello, I have a strange problem, the github workflow err:

Caused by:
  process didn't exit successfully: `/github/home/target/debug/debug/deps/analytic_engine-f3dcc5bef1a624bd` (signal: 6, SIGABRT: process abort signal)
make: *** [Makefile:36: test-ut] Error 101
Error: Process completed with exit code 2.

I can't find the cause of the problem, and when I run make test-ut locally I get an error because of the env variable was not found.😥

@waynexia
Copy link
Member

waynexia commented Jul 6, 2022

@waynexia hello, I have a strange problem, the github workflow err:

Caused by:
  process didn't exit successfully: `/github/home/target/debug/debug/deps/analytic_engine-f3dcc5bef1a624bd` (signal: 6, SIGABRT: process abort signal)
make: *** [Makefile:36: test-ut] Error 101
Error: Process completed with exit code 2.

This is caused by #85 . I've rerun the CI (I suppose we need to keep rerun it till success before that issue got solved...😵

I can't find the cause of the problem, and when I run make test-ut locally I get an error because of the env variable was not found.😥

That makefile is for CI. In local env the command is cargo test --workspace

interpreters/src/show.rs Outdated Show resolved Hide resolved
interpreters/src/show.rs Outdated Show resolved Hide resolved
@ShiKaiWi
Copy link
Member

ShiKaiWi commented Jul 6, 2022

@dust1 This pr is basically OK and we can merge it after the two final comments fixed! Thanks again for your contributions.

@dust1
Copy link
Contributor Author

dust1 commented Jul 6, 2022

thank you every reviewer too!😘

Copy link
Contributor

@jiacai2050 jiacai2050 left a comment

Choose a reason for hiding this comment

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

LGTM.

@waynexia waynexia merged commit 79986a2 into apache:main Jul 7, 2022
@waynexia
Copy link
Member

waynexia commented Jul 7, 2022

Thanks @dust1 ❤️

@dust1 dust1 deleted the issue66 branch July 7, 2022 03:40
chunshao90 pushed a commit to chunshao90/ceresdb that referenced this pull request May 15, 2023
* feat: support show database and show tables

* add: add unit test

* fix: SHOW DATABASE to SHOW DATABASES. with others

* fix: unit test

* fix: edit reboot ci

* edit: Description and function name modification

* fix: reboot workflow

* edit: define the get_default_catalog/get_default_schema error in this mod

* edit: reboot workflow

* edit: edit FromCreateError desc

Co-authored-by: Ruihang Xia <waynestxia@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support show database show tables
4 participants