-
Notifications
You must be signed in to change notification settings - Fork 333
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
fix: error on show databases in non-default catalog #4316
fix: error on show databases in non-default catalog #4316
Conversation
WalkthroughThe recent changes involve updates to schema handling and testing functions. In the 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 (2)
- src/catalog/src/information_schema/schemata.rs (2 hunks)
- tests-integration/src/tests/instance_test.rs (2 hunks)
Additional comments not posted (4)
src/catalog/src/information_schema/schemata.rs (2)
36-36
: Removal ofSchemaNotFoundSnafu
from imports.The
SchemaNotFoundSnafu
import has been removed, indicating that this error is no longer being handled. Ensure that this change does not affect the overall error handling logic in the code.
175-182
: Verify correctness of schema options retrieval logic.The schema options retrieval logic has been updated to handle the absence of
table_metadata_manager
and formatschema_opts
. Ensure that the new logic correctly handles these cases and that the formattedschema_opts
are as expected.Verification successful
Verify correctness of schema options retrieval logic.
The schema options retrieval logic has been updated to handle the absence of
table_metadata_manager
and formatschema_opts
. Ensure that the new logic correctly handles these cases and that the formattedschema_opts
are as expected.
Verified correctness of schema options retrieval logic.
The schema options retrieval logic correctly handles the absence of
table_metadata_manager
and formatsschema_opts
as expected.
- The
table_metadata_manager
is checked forSome
before proceeding.- The schema options are formatted using
format!("{schema_opts}")
.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correctness of schema options retrieval logic. # Test: Search for the usage of `table_metadata_manager` and `schema_opts` formatting. rg --type rust 'table_metadata_manager|schema_opts'Length of output: 34474
Script:
#!/bin/bash # Extract the implementation of the make_schemata method and related methods from schemata.rs ast-grep --lang rust --pattern 'impl $_ { fn make_schemata($$) -> $_ { $$$ } }' > make_schemata_impl.txtLength of output: 89
Script:
#!/bin/bash # Search for the make_schemata method directly and include context lines rg --type rust 'fn make_schemata' -A 10 -B 10 src/catalog/src/information_schema/schemata.rsLength of output: 1257
tests-integration/src/tests/instance_test.rs (2)
19-19
: ImportDEFAULT_SCHEMA_NAME
fromclient
module.This import is necessary to use the default schema name in the test function.
1883-1917
: LGTM! Verify the function integration within the test suite.The code changes are approved.
However, ensure that the new test function
test_show_databases
is properly integrated and executed within the test suite.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4316 +/- ##
==========================================
- Coverage 85.13% 84.86% -0.28%
==========================================
Files 1061 1061
Lines 187834 187830 -4
==========================================
- Hits 159911 159395 -516
- Misses 27923 28435 +512 |
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
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)
What's changed and what's your intention?
Fixes issue introduced in #4174 that throws error on non-default catalog when running
show databases
. Reproducible tests included.Checklist
Summary by CodeRabbit
Bug Fixes
Tests
test_show_databases
to validate SQL queries for showing databases.