-
Notifications
You must be signed in to change notification settings - Fork 313
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: table resolving logic related to pg_catalog #4580
fix: table resolving logic related to pg_catalog #4580
Conversation
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 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 using PR comments)
Other keywords and placeholders
Documentation and Community
|
@sunng87 could you check this pr. It a quiet big change and I'm not sure is it worth to do for such small feature? Or we could find a way that's affect less modules? Also, I wonder is it possible to specify protocol to pg in sqlness. The modification makes the sqlness test of pg_catalog unusable. |
Thank you so much for your effort. We do have protocol information in I'm not 100% sure if there is always a |
I'm pretty sure that there is no |
93a9bb3
to
63da2ac
Compare
@sunng87 I've updated the implementation to reuse the |
Sorry for being late. I have been travelling these days. The first approach sounds good to me. We also have some MySQL specific scenario to cover. Another approach comes to my mind is to provide a http parameter to override the But I think it can be implemented in a separated patch. |
Can we just push forward this pr and keep the the pg specific behaviors untested for a while? I will try to improve sqlness in another patch. |
I think so. Current tests should guarantee that it will not break existed functionalities. |
src/catalog/src/system_schema/information_schema/region_peers.rs
Outdated
Show resolved
Hide resolved
@sunng87 This pr is now ready for reviewed. |
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 the effort. It looks good to me mostly except the one I left comment.
@J0HN50N133 seems we still have a test to fix. @killme2008 @fengjiachun @WenyXu @evenyag PTAL when you got time. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4580 +/- ##
==========================================
- Coverage 84.88% 84.56% -0.33%
==========================================
Files 1105 1105
Lines 199116 199280 +164
==========================================
- Hits 169023 168519 -504
- Misses 30093 30761 +668 |
* fix: table resolving logic related to pg_catalog refer to GreptimeTeam#3560 (comment) and GreptimeTeam#4543 * refactor: remove CatalogProtocol type * fix: sqlness * fix: forbid create database pg_catalog with mysql client * refactor: use QueryContext as arguments rather than Channel * refactor: pass None as default behaviour in information_schema * test: fix test
refer to
#3560 (comment) and #4543
I hereby agree to the terms of the GreptimeDB CLA.
Refer to a related PR or issue link (optional)
close #4543 and #3560 (comment).
What's changed and what's your intention?
!!! DO NOT LEAVE THIS BLOCK EMPTY !!!
This is a quiet huge change. Now query to catalog_manager have to bring a catalog_protocol arguments. So that, the catalog_manager can decide the table resolve logic according to the protocol.
Please explain IN DETAIL what the changes are in this PR and why they are needed:
Checklist