refactor: catalog crate#331
Merged
Merged
Conversation
v0y4g3r
force-pushed
the
refactor/catalog-crate
branch
from
cf2b97b to
fbea07e
Compare
v0y4g3r
force-pushed
the
refactor/catalog-crate
branch
from
b5790f7 to
c9f39d5
Compare
v0y4g3r
requested review from
MichaelScofield and
evenyag
and removed request for
evenyag
Codecov Report
@@ Coverage Diff @@
## develop #331 +/- ##
===========================================
- Coverage 84.41% 84.32% -0.09%
===========================================
Files 351 354 +3
Lines 32273 32528 +255
===========================================
+ Hits 27243 27430 +187
- Misses 5030 5098 +68
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
evenyag
reviewed
Oct 20, 2022
MichaelScofield
approved these changes
Oct 21, 2022
waynexia
reviewed
Oct 21, 2022
waynexia
left a comment
waynexia
left a comment
Member
There was a problem hiding this comment.
CreateTableRequest's catalog_name/schema_name is changed from Option to String, as for table engine, it should not be aware of the "default catalog name" stuff, catalog name and schema name must be explicitly given.
Good to see the third point. We should go depth with it 👍
I've skimmed this patch roughly, it looks good to me except the signature of CatalogManager::table()
fengjiachun
reviewed
Oct 21, 2022
paomian
pushed a commit
to paomian/greptimedb
that referenced
this pull request
Oct 19, 2023
* chore: refactor dir for local catalog manager * refactor: CatalogProvider returns Result * refactor: SchemaProvider returns Result * feat: add kv operations to remote catalog * chore: refactor some code * feat: impl catalog initialization * feat: add register table and register system table function * refactor: add table_info method for Table trait * chore: add some tests * chore: add register schema test * chore: fix build issue after rebase onto develop * refactor: mock to separate file * build: failed to compile * fix: use a container struct to bridge KvBackend and Accessor trait * feat: upgrade opendal to 0.17 * test: add more tests * chore: add catalog name and schema name to table info * chore: add catalog name and schema name to table info * chore: rebase onto develop * refactor: common-catalog crate * refactor: remove remote catalog related files * fix: compilation * feat: add table version to TableKey * feat: add node id to TableValue * fix: some CR comments * chore: change async fn create_expr_to_request to sync * fix: add backtrace to errors * fix: code style * fix: CatalogManager::table also requires both catalog_name and schema_name * chore: merge develop
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
CatalogList/CatalogProvider/SchemaProvider's methods now returns aResult<Option<T>>instead ofOption<T>. This is also a follow-up of arrow-datafusion's API change.CreateTableRequest'scatalog_name/schema_nameis changed fromOption<String>toString, as for table engine, it should not be aware of the "default catalog name" stuff, catalog name and schema name must be explicitly given.