-
Notifications
You must be signed in to change notification settings - Fork 328
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: impl show table status #4303
feat: impl show table status #4303
Conversation
WalkthroughThe overall changes integrate a new Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
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.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (10)
- src/catalog/src/information_schema/tables.rs (1 hunks)
- src/frontend/src/instance.rs (1 hunks)
- src/operator/src/statement.rs (1 hunks)
- src/operator/src/statement/show.rs (2 hunks)
- src/query/src/sql.rs (3 hunks)
- src/sql/src/parsers/show_parser.rs (4 hunks)
- src/sql/src/statements/show.rs (11 hunks)
- src/sql/src/statements/statement.rs (3 hunks)
- tests/cases/standalone/common/show/show_databases_tables.result (1 hunks)
- tests/cases/standalone/common/show/show_databases_tables.sql (1 hunks)
Files skipped from review due to trivial changes (2)
- src/catalog/src/information_schema/tables.rs
- tests/cases/standalone/common/show/show_databases_tables.result
Additional comments not posted (9)
tests/cases/standalone/common/show/show_databases_tables.sql (1)
9-10
: LGTM!The added SQL commands for
SHOW TABLE STATUS
and its variations are syntactically correct and align with the feature implementation.Also applies to: 11-12, 14-15, 17-18, 20-21, 23-24
src/sql/src/statements/statement.rs (1)
35-35
: LGTM!The addition of the
ShowTableStatus
variant in theStatement
enum and itsDisplay
implementation is consistent with other variants and correctly implemented.Also applies to: 74-75, 124-124
src/operator/src/statement/show.rs (1)
25-25
: LGTM!The addition of the
show_table_status
method inStatementExecutor
is correctly implemented and consistent with other methods.Also applies to: 58-67
src/sql/src/statements/show.rs (1)
32-33
: LGTM!The addition of the
ShowTableStatus
struct, itsDisplay
implementation, and theformat_kind!
macro are correctly implemented and consistent with other similar entities.Also applies to: 40-46, 70-75, 89-95, 109-116, 134-141, 144-160
src/operator/src/statement.rs (1)
129-130
: LGTM!The addition of
Statement::ShowTableStatus
is consistent with the existing pattern of handling otherStatement
variants.src/frontend/src/instance.rs (1)
523-525
: LGTM!The addition of
Statement::ShowTableStatus
is consistent with the existing pattern of handling otherStatement
variants. The macrovalidate_db_permission!
is used appropriately.src/sql/src/parsers/show_parser.rs (2)
39-46
: LGTM!The addition for handling
SHOW TABLE STATUS
is consistent with the existing pattern of handling otherSHOW
statements. The functionparse_show_table_status
is called appropriately.
353-377
: LGTM!The
parse_show_table_status
function correctly parses theSHOW TABLE STATUS
statement and handles the optional database andShowKind
clauses. The implementation is consistent with the otherparse_show_*
functions.src/query/src/sql.rs (1)
493-548
: Ensure completeness and correctness of theshow_table_status
function.The function
show_table_status
appears to be well-structured and follows the pattern used in other similar functions. However, ensure that all necessary fields are being queried and returned as per the MySQL documentation forSHOW TABLE STATUS
.Additionally, consider the following potential improvements:
- Error Handling: Ensure that any potential errors are properly handled and propagated.
- Code Comments: Adding comments to explain the purpose of each section of the code can improve readability and maintainability.
Verification successful
Ensure completeness and correctness of the
show_table_status
function.The function
show_table_status
correctly includes all necessary fields as per the MySQL documentation forSHOW TABLE STATUS
. However, consider the following potential improvements:
- Error Handling: Ensure that any potential errors are properly handled and propagated.
- Code Comments: Adding comments to explain the purpose of each section of the code can improve readability and maintainability.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all necessary fields are being queried and returned in the `show_table_status` function. # Test: Search for the fields being queried. Expect: All necessary fields are present. rg --type rust $'tables::TABLE_NAME|tables::ENGINE|tables::VERSION|tables::ROW_FORMAT|tables::TABLE_ROWS|tables::AVG_ROW_LENGTH|tables::DATA_LENGTH|tables::MAX_DATA_LENGTH|tables::INDEX_LENGTH|tables::DATA_FREE|tables::AUTO_INCREMENT|tables::CREATE_TIME|tables::UPDATE_TIME|tables::CHECK_TIME|tables::TABLE_COLLATION|tables::CHECKSUM|tables::CREATE_OPTIONS|tables::TABLE_COMMENT'Length of output: 2011
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.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- src/sql/src/parsers/show_parser.rs (4 hunks)
- src/sql/src/statements/show.rs (11 hunks)
Files skipped from review as they are similar to previous changes (2)
- src/sql/src/parsers/show_parser.rs
- src/sql/src/statements/show.rs
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4303 +/- ##
==========================================
- Coverage 85.23% 84.93% -0.30%
==========================================
Files 1059 1059
Lines 187111 187230 +119
==========================================
- Hits 159480 159020 -460
- Misses 27631 28210 +579 |
7b2ed13
to
9b9602b
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.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (10)
- src/catalog/src/information_schema/tables.rs (1 hunks)
- src/frontend/src/instance.rs (1 hunks)
- src/operator/src/statement.rs (1 hunks)
- src/operator/src/statement/show.rs (2 hunks)
- src/query/src/sql.rs (3 hunks)
- src/sql/src/parsers/show_parser.rs (4 hunks)
- src/sql/src/statements/show.rs (11 hunks)
- src/sql/src/statements/statement.rs (3 hunks)
- tests/cases/standalone/common/show/show_databases_tables.result (1 hunks)
- tests/cases/standalone/common/show/show_databases_tables.sql (1 hunks)
Files skipped from review as they are similar to previous changes (8)
- src/operator/src/statement.rs
- src/operator/src/statement/show.rs
- src/query/src/sql.rs
- src/sql/src/parsers/show_parser.rs
- src/sql/src/statements/show.rs
- src/sql/src/statements/statement.rs
- tests/cases/standalone/common/show/show_databases_tables.result
- tests/cases/standalone/common/show/show_databases_tables.sql
Additional comments not posted (2)
src/catalog/src/information_schema/tables.rs (1)
67-67
: Visibility change approved.The
ENGINE
constant is now public, which aligns with the newSHOW TABLE STATUS
feature requirements.src/frontend/src/instance.rs (1)
523-525
: New permission check forShowTableStatus
approved.The new
ShowTableStatus
statement is now included in thecheck_permission
function, ensuring proper permission validation.
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.
look good!
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.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- src/common/wal/src/config.rs (2 hunks)
- src/common/wal/src/config/kafka/datanode.rs (1 hunks)
- src/common/wal/src/config/kafka/standalone.rs (1 hunks)
Additional comments not posted (4)
src/common/wal/src/config/kafka/datanode.rs (1)
49-49
: Changed default compression setting for Kafka WAL configurations.The default compression setting has been changed from
Compression::Lz4
toCompression::NoCompression
. This might impact the performance and storage efficiency. Ensure this change aligns with the overall system requirements.src/common/wal/src/config/kafka/standalone.rs (1)
70-70
: Changed default compression setting for standalone Kafka WAL configurations.The default compression setting has been changed from
Compression::Lz4
toCompression::NoCompression
. This might impact the performance and storage efficiency. Ensure this change aligns with the overall system requirements.src/common/wal/src/config.rs (2)
210-210
: Changed compression setting for Kafka WAL configurations in test cases.The compression setting for Kafka WAL configurations in the test cases has been changed from
Compression::Lz4
toCompression::NoCompression
. This might impact the performance and storage efficiency. Ensure this change aligns with the overall system requirements.
232-232
: Changed compression setting for standalone Kafka WAL configurations in test cases.The compression setting for standalone Kafka WAL configurations in the test cases has been changed from
Compression::Lz4
toCompression::NoCompression
. This might impact the performance and storage efficiency. Ensure this change aligns with the overall system requirements.
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
I hereby agree to the terms of the GreptimeDB CLA.
Refer to a related PR or issue link (optional)
#4017 #3354
What's changed and what's your intention?
show table status [IN database] [LIKE name | WHERE ..]
.ShowKind
display, theAll
should not display, it's just a placeholder.Checklist
Summary by CodeRabbit
New Features
SHOW TABLE STATUS
command to retrieve information about table statuses within the database.Enhancements
SHOW
commands to provide more consistent and clearer output.Configuration Changes
Lz4
toNoCompression
for both Datanode and Standalone configurations.Tests
SHOW TABLES LIKE
,SHOW FULL TABLES
, andSHOW TABLE STATUS
commands.