[task #8917] Implement information_schema.schemata #8993

Tangruilin · 2024-01-25T09:52:50Z

Which issue does this PR close?

Closes #8917 .

Rationale for this change

What changes are included in this PR?

There are some in infomation_schema.rs

Are these changes tested?

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

Tangruilin · 2024-01-25T09:53:46Z

The datafusion_cli test

Tangruilin · 2024-01-25T10:03:52Z

@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. Maybe you can give some suggestion if we need to have them. Now they are null

alamb

Thanks @Tangruilin

I think CI is failing due to the changes in datafusion/sqllogictest/test_files/aggregates_topk.slt

Which is due to #8845

JanKaul

Great work! Thanks for the effort.

If I understand correctly the default_character_set_catalog, default_character_set_schema, default_character_set_name, sql_path columns are not required. So null is okay.

JanKaul · 2024-01-25T13:52:24Z

datafusion/core/src/catalog/information_schema.rs

+                    let schema_owner = match std::env::var("USER") {
+                        Ok(user) => user,
+                        Err(_) => match std::env::var("USERNAME") {
+                            Ok(user) => user,
+                            Err(_) => "".to_owned(),
+                        },
+                    };


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?

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

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:

Leaving owner NULL for now

Add a function to the SchemaProvider to optionally provide a an owner

For example

trait SchemaProvider { // return the "schema owner" of known, reported in `information_schema.schemata` // Returns `None` by default fn owner_name(&self) -> Option<&str> { None } }

alamb

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)?

alamb · 2024-01-25T16:21:55Z

datafusion/core/src/catalog/information_schema.rs

+                    let schema_owner = match std::env::var("USER") {
+                        Ok(user) => user,
+                        Err(_) => match std::env::var("USERNAME") {
+                            Ok(user) => user,
+                            Err(_) => "".to_owned(),
+                        },
+                    };


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:

Leaving owner NULL for now

Add a function to the SchemaProvider to optionally provide a an owner

For example

trait SchemaProvider { // return the "schema owner" of known, reported in `information_schema.schemata` // Returns `None` by default fn owner_name(&self) -> Option<&str> { None } }

Tangruilin · 2024-01-26T01:46:09Z

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

alamb · 2024-01-27T10:18:02Z

How is this PR going? I think it just needs

Remove the use of environment variables
A query test (in information_schema.slt)

Tangruilin · 2024-01-27T11:08:09Z

How is this PR going? I think it just needs

Remove the use of environment variables

A query test (in information_schema.slt)

I will submit a new PR later or tomorrow

Tangruilin · 2024-01-27T11:53:03Z

How is this PR going? I think it just needs

Remove the use of environment variables

A query test (in information_schema.slt)

I have done that ~~~

JanKaul

Looks good to me, thank you for working on it.

alamb

Looks great to me -- thank you @Tangruilin and @JanKaul -- I had some small comment suggestions but I don't think they are needed to merge this PR (they can be done as a follow on PR or never)

Thanks again!

alamb · 2024-01-27T16:31:07Z

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


Suggested change

/// Returns the owner of the Schema, default is None

/// Returns the owner of the Schema, default is None. This value is reported

/// as part of `information_tables.schemata`

alamb · 2024-01-27T16:31:34Z

datafusion/core/src/catalog/information_schema.rs

+            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


Suggested change

// refer to https://www.postgresql.org/docs/current/infoschema-schemata.html, these rows are Applies to a feature not available

// refer to https://www.postgresql.org/docs/current/infoschema-schemata.html,

// these rows apply to a feature that is not implemented in DataFusion

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 ~~~

alamb · 2024-01-27T16:39:37Z

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

Signed-off-by: tangruilin <tang.ruilin@foxmail.com>

alamb

Thanks again @Tangruilin ❤️

github-actions bot added the core Core DataFusion crate label Jan 25, 2024

Tangruilin force-pushed the task#8917 branch 2 times, most recently from 0d3a573 to 6785b69 Compare January 25, 2024 11:35

github-actions bot added the sqllogictest SQL Logic Tests (.slt) label Jan 25, 2024

alamb reviewed Jan 25, 2024

View reviewed changes

alamb mentioned this pull request Jan 25, 2024

Test failure in sqllogictest aggregates_topk.slt on mac #8845

Closed

Tangruilin force-pushed the task#8917 branch from 6785b69 to cba1bad Compare January 25, 2024 13:38

JanKaul reviewed Jan 25, 2024

View reviewed changes

alamb reviewed Jan 25, 2024

View reviewed changes

alamb mentioned this pull request Jan 25, 2024

DataFusion weekly project plan (Andrew Lamb) - Jan 22, 2024 #8933

Closed

9 tasks

Tangruilin force-pushed the task#8917 branch from cba1bad to b38a158 Compare January 27, 2024 11:48

Tangruilin requested review from alamb and JanKaul January 27, 2024 11:52

JanKaul approved these changes Jan 27, 2024

View reviewed changes

alamb approved these changes Jan 27, 2024

View reviewed changes

[task apache#8917] Implement information_schema.schemata

521b630

Signed-off-by: tangruilin <tang.ruilin@foxmail.com>

Tangruilin force-pushed the task#8917 branch from b38a158 to 521b630 Compare January 28, 2024 04:15

alamb approved these changes Jan 28, 2024

View reviewed changes

alamb merged commit a668abf into apache:main Jan 28, 2024
22 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[task #8917] Implement information_schema.schemata #8993

[task #8917] Implement information_schema.schemata #8993

Tangruilin commented Jan 25, 2024

Tangruilin commented Jan 25, 2024

Tangruilin commented Jan 25, 2024

alamb left a comment

JanKaul left a comment

JanKaul Jan 25, 2024

Tangruilin Jan 25, 2024

alamb Jan 25, 2024

alamb left a comment

alamb Jan 25, 2024

Tangruilin commented Jan 26, 2024

alamb commented Jan 27, 2024

Tangruilin commented Jan 27, 2024

Tangruilin commented Jan 27, 2024

JanKaul left a comment

alamb left a comment

alamb Jan 27, 2024

alamb Jan 27, 2024

Tangruilin Jan 27, 2024

alamb commented Jan 27, 2024

alamb left a comment

	/// Returns the owner of the Schema, default is None
	/// Returns the owner of the Schema, default is None. This value is reported
	/// as part of `information_tables.schemata`

	// refer to https://www.postgresql.org/docs/current/infoschema-schemata.html, these rows are Applies to a feature not available
	// refer to https://www.postgresql.org/docs/current/infoschema-schemata.html,
	// these rows apply to a feature that is not implemented in DataFusion

[task #8917] Implement information_schema.schemata #8993

[task #8917] Implement information_schema.schemata #8993

Conversation

Tangruilin commented Jan 25, 2024

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Tangruilin commented Jan 25, 2024

Tangruilin commented Jan 25, 2024

alamb left a comment

Choose a reason for hiding this comment

JanKaul left a comment

Choose a reason for hiding this comment

JanKaul Jan 25, 2024

Choose a reason for hiding this comment

Tangruilin Jan 25, 2024

Choose a reason for hiding this comment

alamb Jan 25, 2024

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

alamb Jan 25, 2024

Choose a reason for hiding this comment

Tangruilin commented Jan 26, 2024

alamb commented Jan 27, 2024

Tangruilin commented Jan 27, 2024

Tangruilin commented Jan 27, 2024

JanKaul left a comment

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

alamb Jan 27, 2024

Choose a reason for hiding this comment

alamb Jan 27, 2024

Choose a reason for hiding this comment

Tangruilin Jan 27, 2024

Choose a reason for hiding this comment

alamb commented Jan 27, 2024

alamb left a comment

Choose a reason for hiding this comment