ISSUE #3206: Support SHOW ENGINES Statement #4050

SGZW · 2022-02-04T08:07:16Z

I hereby agree to the terms of the CLA available at: https://databend.rs/dev/policies/cla/

Summary

solve #3206

Changelog

New Feature

Related Issues

Test Plan

Unit Tests

Stateless Tests

databend-bot · 2022-02-04T08:07:19Z

Thanks for the contribution!
I have applied any labels matching special text in your PR Changelog.

Please review the labels and make any necessary changes.

databend-bot · 2022-02-04T08:07:19Z

Thanks for the contribution!
I have applied any labels matching special text in your PR Changelog.

Please review the labels and make any necessary changes.

vercel · 2022-02-04T08:07:20Z

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/databend/databend/HLnFm1JT5om6fhkDULtw8UKJtEph
✅ Preview: Canceled

[Deployment for 01c0fc0 canceled]

BohuTANG · 2022-02-04T08:16:30Z

query/src/storages/system/engines_table.rs

+        let mut engine_support = Vec::with_capacity(1);
+        let mut engine_comment = Vec::with_capacity(1);
+        // fuse engine
+        engine_name.push("FUSE".as_bytes());


Let's get the engines information from the StorageFactory :\

https://github.com/datafuselabs/databend/blob/76e831b753eb85b274314e3ec8c4a30c0be48171/query/src/storages/storage_factory.rs#L51-L73

ok, let me fix it

Looks like we should add desc() to StorageCreator as the comment column:

pub trait StorageCreator: Send + Sync { fn try_create(&self, ctx: StorageContext, table_info: TableInfo) -> Result<Box<dyn Table>>; fn desc(&self) -> &str; }

BohuTANG · 2022-02-05T09:34:33Z

query/src/storages/storage_factory.rs

 #[derive(Default)]
 pub struct StorageFactory {
    creators: RwLock<HashMap<String, Arc<dyn StorageCreator>>>,
+    descriptors: Vec<StorageDescriptor>,


Thanks for the descriptor idea.
I think StorageDescriptor should be a part of table engine, it explicitly tell what this engine is, as defined by the engine's module not the Factory.

what if every table engine implementes StorageCreateor trait and StorageCreator add interface desc() to return StorageDescriptor?

what if every table engine implementes StorageCreateor trait and StorageCreator add interface desc() to return StorageDescriptor?

Agreed.

Update:
It seems we only need add desc() function to the StorageCreator.

BohuTANG · 2022-02-05T09:40:04Z

query/src/storages/system/engines_table.rs

+    pub fn create(table_id: u64) -> Self {
+        let schema = DataSchemaRefExt::create(vec![
+            DataField::new("Engine", DataType::String, false),
+            DataField::new("Support", DataType::String, false),


We could remove the Support column here, when the engine who is not registered to the factory it means we don't want it to show up, this is a bit different from MySQL engines.

BohuTANG · 2022-02-05T13:43:23Z

query/src/storages/system/engines_table.rs

+        _ctx: Arc<QueryContext>,
+        _plan: &ReadDataSourcePlan,
+    ) -> Result<SendableDataBlockStream> {
+        let descriptors = _ctx.get_catalog().get_storage_descriptors();


Why we add StorageDescriptor to catalog and impl it for every table engine, how about get the table engine name and descriptor from self.ctx.storage_factory?
It seems we only add a function like desc()->&str for the table engine.

i think i don't get the real meaning of this. can you explain it more clearly? i think every engine should has it's own creator

the reason of why we add StorageDescriptor to catalog is that storage_factory is only MutableCatalog's member variable，so database catalog should expose a api to get descriptors, or we can get descriptors from storage_factory and construct the EnginesTable

Descrptor is Attributes of engine，so I impl it for every table engine...

How about only add one function like description() to table engine instead of the Descriptior trait? And we register the description to storage factory, this looks more clearly.
Then we can add one trait function named 'get_table_engines' to Catalog, which getting the engine details from the mutable catalog.

codecov-commenter · 2022-02-05T14:11:50Z

Codecov Report

Merging #4050 (01c0fc0) into main (76e831b) will increase coverage by 0%.
The diff coverage is 76%.

@@          Coverage Diff           @@
##            main   #4050    +/-   ##
======================================
  Coverage     57%     57%            
======================================
  Files        821     825     +4     
  Lines      43468   43623   +155     
======================================
+ Hits       24805   24931   +126     
- Misses     18663   18692    +29

Impacted Files	Coverage Δ
query/src/catalogs/catalog.rs	`29% <0%> (-3%)`	⬇️
query/src/sql/sql_statement.rs	`30% <0%> (-1%)`	⬇️
query/src/sql/statements/analyzer_statement.rs	`90% <0%> (-1%)`	⬇️
query/src/sql/statements/statement_show_engines.rs	`0% <0%> (ø)`
query/src/storages/storage_factory.rs	`78% <80%> (-3%)`	⬇️
query/src/storages/fuse/table.rs	`93% <83%> (-1%)`	⬇️
query/src/storages/github/github_table.rs	`8% <83%> (+8%)`	⬆️
query/src/storages/memory/memory_table.rs	`96% <83%> (-1%)`	⬇️
query/src/storages/null/null_table.rs	`90% <83%> (-1%)`	⬇️
query/src/storages/system/engines_table.rs	`88% <88%> (ø)`
... and 16 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 76e831b...01c0fc0. Read the comment docs.

BohuTANG · 2022-02-06T00:53:20Z

/lgtm
Thank you @SGZW !

ISSUE databendlabs#3206: Support SHOW ENGINES Statement

191c264

SGZW requested a review from BohuTANG as a code owner February 4, 2022 08:07

databend-bot added the pr-feature this PR introduces a new feature to the codebase label Feb 4, 2022

databend-bot added the need-review label Feb 4, 2022

vercel bot temporarily deployed to Preview February 4, 2022 08:07 Inactive

BohuTANG reviewed Feb 4, 2022

View reviewed changes

get the engines information from the StorageFactory

947b8a1

vercel bot temporarily deployed to Preview February 5, 2022 07:52 Inactive

SGZW requested a review from BohuTANG February 5, 2022 07:52

BohuTANG reviewed Feb 5, 2022

View reviewed changes

fix

7cf178c

vercel bot temporarily deployed to Preview February 5, 2022 13:25 Inactive

SGZW requested a review from BohuTANG February 5, 2022 13:26

BohuTANG reviewed Feb 5, 2022

View reviewed changes

only add description to every table engine

01c0fc0

vercel bot temporarily deployed to Preview February 5, 2022 15:39 Inactive

SGZW requested a review from BohuTANG February 5, 2022 15:40

BohuTANG merged commit c340c45 into databendlabs:main Feb 6, 2022

databend-bot approved these changes Feb 6, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ISSUE #3206: Support SHOW ENGINES Statement #4050

ISSUE #3206: Support SHOW ENGINES Statement #4050

SGZW commented Feb 4, 2022

databend-bot commented Feb 4, 2022

databend-bot commented Feb 4, 2022

vercel bot commented Feb 4, 2022 •

edited

Loading

BohuTANG Feb 4, 2022

BohuTANG Feb 4, 2022

SGZW Feb 4, 2022

BohuTANG Feb 4, 2022

BohuTANG Feb 5, 2022

SGZW Feb 5, 2022

BohuTANG Feb 5, 2022 •

edited

Loading

BohuTANG Feb 5, 2022

SGZW Feb 5, 2022

BohuTANG Feb 5, 2022

SGZW Feb 5, 2022

SGZW Feb 5, 2022

SGZW Feb 5, 2022

BohuTANG Feb 5, 2022

codecov-commenter commented Feb 5, 2022 •

edited

Loading

BohuTANG commented Feb 6, 2022

ISSUE #3206: Support SHOW ENGINES Statement #4050

ISSUE #3206: Support SHOW ENGINES Statement #4050

Conversation

SGZW commented Feb 4, 2022

Summary

Changelog

Related Issues

Test Plan

databend-bot commented Feb 4, 2022

databend-bot commented Feb 4, 2022

vercel bot commented Feb 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BohuTANG Feb 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Feb 5, 2022 • edited Loading

Codecov Report

BohuTANG commented Feb 6, 2022

vercel bot commented Feb 4, 2022 •

edited

Loading

BohuTANG Feb 5, 2022 •

edited

Loading

codecov-commenter commented Feb 5, 2022 •

edited

Loading