You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "Tangruilin (via GitHub)" <gi...@apache.org> on 2024/01/25 09:52:54 UTC
[PR] [task #8917] Implement information_schema.schemata [arrow-datafusion]
Tangruilin opened a new pull request, #8993:
URL: https://github.com/apache/arrow-datafusion/pull/8993
## Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
Closes #8917 .
## Rationale for this change
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
## What changes are included in this PR?
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
There are some in `infomation_schema.rs`
## Are these changes tested?
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
2. Serve as another way to document the expected behavior of the code
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
-->
Yes
## Are there any user-facing changes?
There will be a new table named `schemata`, users can `select * from information_schema.schemata;` to get info in it
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!--
If there are any breaking changes to public APIs, please add the `api change` label.
-->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [task #8917] Implement information_schema.schemata [arrow-datafusion]
Posted by "Tangruilin (via GitHub)" <gi...@apache.org>.
Tangruilin commented on PR #8993:
URL: https://github.com/apache/arrow-datafusion/pull/8993#issuecomment-1913118730
> How is this PR going? I think it just needs
>
> 1. Remove the use of environment variables
> 2. A query test (in information_schema.slt)
I will submit a new PR later or tomorrow
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [task #8917] Implement information_schema.schemata [arrow-datafusion]
Posted by "Tangruilin (via GitHub)" <gi...@apache.org>.
Tangruilin commented on PR #8993:
URL: https://github.com/apache/arrow-datafusion/pull/8993#issuecomment-1913132239
> How is this PR going? I think it just needs
>
> 1. Remove the use of environment variables
> 2. A query test (in information_schema.slt)
I have done that ~~~
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [task #8917] Implement information_schema.schemata [arrow-datafusion]
Posted by "Tangruilin (via GitHub)" <gi...@apache.org>.
Tangruilin commented on PR #8993:
URL: https://github.com/apache/arrow-datafusion/pull/8993#issuecomment-1909800862
@JanKaul
Hey! this is the draft, you can review this PR.
The column `default_character_set_catalog` `default_character_set_schema` `default_character_set_name` and `sql_path` Applies to a feature not available refer to [https://www.postgresql.org/docs/current/infoschema-schemata.html](https://www.postgresql.org/docs/current/infoschema-schemata.html). Maybe you can give some suggestion if we need to have them. Now they are null
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [task #8917] Implement information_schema.schemata [arrow-datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #8993:
URL: https://github.com/apache/arrow-datafusion/pull/8993#issuecomment-1913106563
How is this PR going? I think it just needs
1. Remove the use of environment variables
2. A query test (in information_schema.slt)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [task #8917] Implement information_schema.schemata [arrow-datafusion]
Posted by "Tangruilin (via GitHub)" <gi...@apache.org>.
Tangruilin commented on code in PR #8993:
URL: https://github.com/apache/arrow-datafusion/pull/8993#discussion_r1468521848
##########
datafusion/core/src/catalog/information_schema.rs:
##########
@@ -617,6 +646,106 @@ impl InformationSchemaColumnsBuilder {
}
}
+struct InformationSchemata {
+ schema: SchemaRef,
+ config: InformationSchemaConfig,
+}
+
+impl InformationSchemata {
+ fn new(config: InformationSchemaConfig) -> Self {
+ let schema = Arc::new(Schema::new(vec![
+ Field::new("catalog_name", DataType::Utf8, false),
+ Field::new("schema_name", DataType::Utf8, false),
+ Field::new("schema_owner", DataType::Utf8, true),
+ Field::new("default_character_set_catalog", DataType::Utf8, true),
+ Field::new("default_character_set_schema", DataType::Utf8, true),
+ Field::new("default_character_set_name", DataType::Utf8, true),
+ Field::new("sql_path", DataType::Utf8, true),
+ ]));
+ Self { schema, config }
+ }
+
+ fn builder(&self) -> InformationSchemataBuilder {
+ InformationSchemataBuilder {
+ schema: self.schema.clone(),
+ catalog_name: StringBuilder::new(),
+ schema_name: StringBuilder::new(),
+ schema_owner: StringBuilder::new(),
+ default_character_set_catalog: StringBuilder::new(),
+ default_character_set_schema: StringBuilder::new(),
+ default_character_set_name: StringBuilder::new(),
+ sql_path: StringBuilder::new(),
+ }
+ }
+}
+
+struct InformationSchemataBuilder {
+ schema: SchemaRef,
+ catalog_name: StringBuilder,
+ schema_name: StringBuilder,
+ schema_owner: StringBuilder,
+ default_character_set_catalog: StringBuilder,
+ default_character_set_schema: StringBuilder,
+ default_character_set_name: StringBuilder,
+ sql_path: StringBuilder,
+}
+
+impl InformationSchemataBuilder {
+ fn add_schemata(
+ &mut self,
+ catalog_name: &str,
+ schema_name: &str,
+ schema_owner: Option<&str>,
+ ) {
+ self.catalog_name.append_value(catalog_name);
+ self.schema_name.append_value(schema_name);
+ match schema_owner {
+ Some(owner) => self.schema_owner.append_value(owner),
+ None => self.schema_owner.append_null(),
+ }
+ // refer to https://www.postgresql.org/docs/current/infoschema-schemata.html, these rows are Applies to a feature not available
Review Comment:
You can merge it, and i will submit a PR tomorrow to fix these.
Now it is 12:30 here, i may sleep, so sorry ~~~
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [task #8917] Implement information_schema.schemata [arrow-datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #8993:
URL: https://github.com/apache/arrow-datafusion/pull/8993#discussion_r1466618879
##########
datafusion/core/src/catalog/information_schema.rs:
##########
@@ -115,6 +117,33 @@ impl InformationSchemaConfig {
DF_SETTINGS,
TableType::View,
);
+ builder.add_table(
+ &catalog_name,
+ INFORMATION_SCHEMA,
+ SCHEMATA,
+ TableType::View,
+ );
+ }
+ }
+
+ async fn make_schemata(&self, builder: &mut InformationSchemataBuilder) {
+ for catalog_name in self.catalog_list.catalog_names() {
+ let catalog = self.catalog_list.catalog(&catalog_name).unwrap();
+
+ for schema_name in catalog.schema_names() {
+ if schema_name != INFORMATION_SCHEMA {
+ // schema name may not exist in the catalog, so we need to check
+ // then get user from env
+ let schema_owner = match std::env::var("USER") {
+ Ok(user) => user,
+ Err(_) => match std::env::var("USERNAME") {
+ Ok(user) => user,
+ Err(_) => "".to_owned(),
+ },
+ };
Review Comment:
I agree pulling from the environment is not consistent with how the rest of DataFusion works. I think the owner should come from the `SchemaProvider`
However there is no notion of `owner` on `SchemaProvider` now
Thus, I suggest either:
1. Leaving owner `NULL` for now
2. Add a function to the `SchemaProvider` to optionally provide a an owner
For example
```rust
trait SchemaProvider {
// return the "schema owner" of known, reported in `information_schema.schemata`
// Returns `None` by default
fn owner_name(&self) -> Option<&str> {
None
}
}
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [task #8917] Implement information_schema.schemata [arrow-datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #8993:
URL: https://github.com/apache/arrow-datafusion/pull/8993
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [task #8917] Implement information_schema.schemata [arrow-datafusion]
Posted by "Tangruilin (via GitHub)" <gi...@apache.org>.
Tangruilin commented on PR #8993:
URL: https://github.com/apache/arrow-datafusion/pull/8993#issuecomment-1911278112
> Thanks @Tangruilin and @JanKaul -- would it be possible to add a test that actually selects values from `information_schema.schemata` in datafusion/sqllogictest/test_files/information_schema.slt (to test the values)?
I will try this
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [task #8917] Implement information_schema.schemata [arrow-datafusion]
Posted by "JanKaul (via GitHub)" <gi...@apache.org>.
JanKaul commented on code in PR #8993:
URL: https://github.com/apache/arrow-datafusion/pull/8993#discussion_r1466410111
##########
datafusion/core/src/catalog/information_schema.rs:
##########
@@ -115,6 +117,33 @@ impl InformationSchemaConfig {
DF_SETTINGS,
TableType::View,
);
+ builder.add_table(
+ &catalog_name,
+ INFORMATION_SCHEMA,
+ SCHEMATA,
+ TableType::View,
+ );
+ }
+ }
+
+ async fn make_schemata(&self, builder: &mut InformationSchemataBuilder) {
+ for catalog_name in self.catalog_list.catalog_names() {
+ let catalog = self.catalog_list.catalog(&catalog_name).unwrap();
+
+ for schema_name in catalog.schema_names() {
+ if schema_name != INFORMATION_SCHEMA {
+ // schema name may not exist in the catalog, so we need to check
+ // then get user from env
+ let schema_owner = match std::env::var("USER") {
+ Ok(user) => user,
+ Err(_) => match std::env::var("USERNAME") {
+ Ok(user) => user,
+ Err(_) => "".to_owned(),
+ },
+ };
Review Comment:
I'm not really sure about using the environment variables for the schema owner. I guess datafusion doesn't really have the concept of an user and therefore an owner. However, this is a required column and should somehow have a value. Similar to the default catalog we could use "datafusion" here.
@alamb what do you think?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [task #8917] Implement information_schema.schemata [arrow-datafusion]
Posted by "Tangruilin (via GitHub)" <gi...@apache.org>.
Tangruilin commented on PR #8993:
URL: https://github.com/apache/arrow-datafusion/pull/8993#issuecomment-1909784275
The datafusion_cli test
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [task #8917] Implement information_schema.schemata [arrow-datafusion]
Posted by "Tangruilin (via GitHub)" <gi...@apache.org>.
Tangruilin commented on code in PR #8993:
URL: https://github.com/apache/arrow-datafusion/pull/8993#discussion_r1466418460
##########
datafusion/core/src/catalog/information_schema.rs:
##########
@@ -115,6 +117,33 @@ impl InformationSchemaConfig {
DF_SETTINGS,
TableType::View,
);
+ builder.add_table(
+ &catalog_name,
+ INFORMATION_SCHEMA,
+ SCHEMATA,
+ TableType::View,
+ );
+ }
+ }
+
+ async fn make_schemata(&self, builder: &mut InformationSchemataBuilder) {
+ for catalog_name in self.catalog_list.catalog_names() {
+ let catalog = self.catalog_list.catalog(&catalog_name).unwrap();
+
+ for schema_name in catalog.schema_names() {
+ if schema_name != INFORMATION_SCHEMA {
+ // schema name may not exist in the catalog, so we need to check
+ // then get user from env
+ let schema_owner = match std::env::var("USER") {
+ Ok(user) => user,
+ Err(_) => match std::env::var("USERNAME") {
+ Ok(user) => user,
+ Err(_) => "".to_owned(),
+ },
+ };
Review Comment:
Now I get the System User from environment variables, which is same as postgresql.
Maybe you will have some good suggestions? It seems that datafusion do not have a user
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [task #8917] Implement information_schema.schemata [arrow-datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #8993:
URL: https://github.com/apache/arrow-datafusion/pull/8993#discussion_r1468519716
##########
datafusion/core/src/catalog/schema.rs:
##########
@@ -34,6 +34,11 @@ use crate::error::{DataFusionError, Result};
/// [`CatalogProvider`]: super::CatalogProvider
#[async_trait]
pub trait SchemaProvider: Sync + Send {
+ /// Returns the owner of the Schema, default is None
Review Comment:
```suggestion
/// Returns the owner of the Schema, default is None. This value is reported
/// as part of `information_tables.schemata`
```
##########
datafusion/core/src/catalog/information_schema.rs:
##########
@@ -617,6 +646,106 @@ impl InformationSchemaColumnsBuilder {
}
}
+struct InformationSchemata {
+ schema: SchemaRef,
+ config: InformationSchemaConfig,
+}
+
+impl InformationSchemata {
+ fn new(config: InformationSchemaConfig) -> Self {
+ let schema = Arc::new(Schema::new(vec![
+ Field::new("catalog_name", DataType::Utf8, false),
+ Field::new("schema_name", DataType::Utf8, false),
+ Field::new("schema_owner", DataType::Utf8, true),
+ Field::new("default_character_set_catalog", DataType::Utf8, true),
+ Field::new("default_character_set_schema", DataType::Utf8, true),
+ Field::new("default_character_set_name", DataType::Utf8, true),
+ Field::new("sql_path", DataType::Utf8, true),
+ ]));
+ Self { schema, config }
+ }
+
+ fn builder(&self) -> InformationSchemataBuilder {
+ InformationSchemataBuilder {
+ schema: self.schema.clone(),
+ catalog_name: StringBuilder::new(),
+ schema_name: StringBuilder::new(),
+ schema_owner: StringBuilder::new(),
+ default_character_set_catalog: StringBuilder::new(),
+ default_character_set_schema: StringBuilder::new(),
+ default_character_set_name: StringBuilder::new(),
+ sql_path: StringBuilder::new(),
+ }
+ }
+}
+
+struct InformationSchemataBuilder {
+ schema: SchemaRef,
+ catalog_name: StringBuilder,
+ schema_name: StringBuilder,
+ schema_owner: StringBuilder,
+ default_character_set_catalog: StringBuilder,
+ default_character_set_schema: StringBuilder,
+ default_character_set_name: StringBuilder,
+ sql_path: StringBuilder,
+}
+
+impl InformationSchemataBuilder {
+ fn add_schemata(
+ &mut self,
+ catalog_name: &str,
+ schema_name: &str,
+ schema_owner: Option<&str>,
+ ) {
+ self.catalog_name.append_value(catalog_name);
+ self.schema_name.append_value(schema_name);
+ match schema_owner {
+ Some(owner) => self.schema_owner.append_value(owner),
+ None => self.schema_owner.append_null(),
+ }
+ // refer to https://www.postgresql.org/docs/current/infoschema-schemata.html, these rows are Applies to a feature not available
Review Comment:
```suggestion
// refer to https://www.postgresql.org/docs/current/infoschema-schemata.html,
// these rows apply to a feature that is not implemented in DataFusion
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [task #8917] Implement information_schema.schemata [arrow-datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #8993:
URL: https://github.com/apache/arrow-datafusion/pull/8993#issuecomment-1913252394
> Now it is 12:30 here, i may sleep, so sorry ~~~
Yes, no worries, go to 💤 !! We can wait until tomorrow there is no need for you to lose sleep or to rush in the PR
It is part of being a distributed community
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org