ARROW-12037: [Rust] [DataFusion] Support catalogs and schemas for table namespacing by returnString · Pull Request #9762 · apache/arrow

returnString · 2021-03-21T12:30:02Z

This is an implementation of catalog and schema providers to support table namespacing (see the design doc).

I'm creating this draft PR as a supporting implementation for the proposal, to prove out that the work can be done whilst minimising API churn and still allowing for use cases that don't care at all about the notion of catalogs or schemas; in this new setup, the default namespace is datafusion.public, which will be created automatically with the default execution context config and allow for table registration.

Highlights

Datasource map removed in execution context state, replaced with catalog map
Execution context allows for registering new catalog providers
Catalog providers can be queried for their constituent schema providers
Schema providers can be queried for table providers, similarly to the old datasource map
Includes basic implementations of CatalogProvider and SchemaProvider backed by hashmaps
New TableReference enum maps to various ways of referring to a table in sql
- Bare: my_table
- Partial: schema.my_table
- Full: catalog.schema.my_table
Given a default catalog and schema, TableReference instances of any variant can be converted to a ResolvedTableReference, which always include all three components

All tests passing with fallible table management

github-actions · 2021-03-21T12:30:31Z

https://issues.apache.org/jira/browse/ARROW-12037

Dandandan · 2021-03-21T16:50:49Z

This is truly awesome @returnString !
Apart from some tests & remaining cleanup (e.g. of the "quick hack") I really like the direction this is going!

andygrove · 2021-03-21T16:50:49Z

Thanks @returnString and I appreciate the well-written design doc to explain the PR. I didn't go through the code in great detail but I like the design. I noted a couple of unwraps in the code. It would be good to document why they are safe or consider having those methods return Result if they are not safe.

returnString · 2021-03-21T17:20:12Z

Thanks both :)

I noted a couple of unwraps in the code. It would be good to document why they are safe or consider having those methods return Result if they are not safe.

I've just audited all the unwraps that are introduced in these commits, and they fall into two categories:

test cases that aren't already set up to return Result<()>
unwrapping guard objects for Mutex/RwLock - afaik these only error if the lock is poisoned and this usage is consistent with existing lock usage e.g. state access in the ExecutionContext

Apart from some tests & remaining cleanup (e.g. of the "quick hack") I really like the direction this is going!

I think the quick hack (context: retrieving a list of table names from the execution ctx, assuming a flat namespace) can probably be removed outright, as ctx.catalog("my_db")?.schema("my_schema")?.table_names() would be a much more explicit way of doing this going forward. As far as I can tell, ExecutionContext::tables isn't referenced at all in the codebase internally, but this would introduce a bit more API breakage.

As for tests, yes, the coverage so far is basically "didn't break anything" and "all three types of reference to a given table in the default catalog/schema resolve correctly", so I'll try and plan out some more in-depth testing covering e.g. tables of the same name in multiple schemas/catalogs.

codecov-io · 2021-03-21T21:20:44Z

Codecov Report

Merging #9762 (03048c6) into master (29feea0) will increase coverage by 0.00%.
The diff coverage is 80.36%.

@@ Coverage Diff @@## master #9762 +/- ##
========================================
Coverage 82.59% 82.59% ========================================
Files 248 252 +4 Lines 58294 59004 +710 ========================================
+ Hits 48149 48737 +588 - Misses 10145 10267 +122

Impacted Files	Coverage Δ
rust/datafusion/examples/flight_server.rs	`0.00% <0.00%> (ø)`
rust/datafusion/examples/simple_udaf.rs	`0.00% <ø> (ø)`
rust/datafusion/src/datasource/datasource.rs	`100.00% <ø> (ø)`
rust/datafusion/src/datasource/memory.rs	`85.15% <ø> (ø)`
rust/datafusion/src/execution/dataframe_impl.rs	`89.10% <0.00%> (-1.03%)`	⬇️
rust/datafusion/src/optimizer/constant_folding.rs	`92.30% <0.00%> (-0.36%)`	⬇️
...datafusion/src/optimizer/hash_build_probe_order.rs	`53.60% <0.00%> (-1.72%)`	⬇️
...t/datafusion/src/optimizer/projection_push_down.rs	`98.66% <ø> (ø)`
rust/datafusion/src/physical_plan/mod.rs	`88.00% <ø> (ø)`
rust/parquet/src/basic.rs	`88.59% <ø> (+1.36%)`	⬆️
... and 35 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 81d6724...03048c6. Read the comment docs.

returnString · 2021-03-21T22:15:06Z

Whilst I still want to try and conjure up a few more test cases, I think this PR is now in a state where I'm happy to receive actual code reviews, so I've removed the draft status :)

alamb · 2021-03-22T12:35:23Z

I plan to review this PR later today

alamb

All in all this looks great. Thank you @returnString !

I went through it fairly carefully and I have some suggestions but nothing that would prevent us from merging this in my opinion.

alamb · 2021-03-22T19:45:54Z

 start.elapsed().as_millis()
 );
- ctx.register_table(table, Arc::new(memtable));
+ ctx.register_table(*table, Arc::new(memtable))?;


The change to make register_table fallible is a breaking change, but a very reasonable one I think.

alamb · 2021-03-22T19:47:36Z

+// specific language governing permissions and limitations
+// under the License.
+
+//! Describes the interface and built-in implementations of catalogs,


alamb · 2021-03-22T19:49:42Z

+ fn as_any(&self) -> &dyn Any;
+
+ /// Retrieves the list of available schema names in this catalog.
+ fn schema_names(&self) -> Vec<String>;


What do you think about returning something more like Vec<&str> to prevent requiring a copy?
Ideally it would be nice if we could do something like
fn schema_names(&self) -> impl Iterator<Item=&str>
As all uses of the results here will need to iterate over the names I suspect

I iniitally tried to implement these returning &[&str] but with the threading requirements I struggled to get anything working that way. I think this would require a larger refactoring to enable, but I'm not 100% sure on that.

Vec<&str> might be possible

alamb · 2021-03-22T19:50:44Z

+}
+
+/// Simple in-memory implementation of a catalog.
+pub struct MemoryCatalogProvider {


In some other PR perhaps we can put the concrete implementations into their own modules. I don't think this one is big enough to warrant that yet, however; I just wanted to point it out

alamb · 2021-03-22T19:52:32Z

+impl<'a> TryFrom<&'a sqlparser::ast::ObjectName> for TableReference<'a> {
+ type Error = DataFusionError;
+
+ fn try_from(value: &'a sqlparser::ast::ObjectName) -> Result<Self, Self::Error> {


alamb · 2021-03-22T20:01:36Z

 }
+
+ /// Selects a name for the default catalog and schema
+ pub fn with_default_catalog_and_schema(


I like this API. 👍
I wonder if we need an API for getting default_catalog and default_schema?

alamb · 2021-03-22T20:03:24Z

@@ -1922,6 +2035,114 @@ mod tests {
 Ok(())
 }



these are excellent tests . 👍

alamb · 2021-03-22T20:03:45Z

+ &mut ctx,
+ "SELECT cat, SUM(i) AS total FROM (
+ SELECT i, 'a' AS cat FROM catalog_a.schema_a.table_a
+ UNION ALL


@DandandanUNION is already being used!

alamb · 2021-03-22T20:04:40Z

+ for table_ref in &[
+ "nonexistentschema.aggregate_test_100",
+ "nonexistentcatalog.public.aggregate_test_100",
+ "way.too.many.namespaces.as.ident.prefixes.aggregate_test_100",


Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>

alamb · 2021-03-23T13:58:33Z

@returnString do you plan anything else for this PR? I think it is ready to merge but I wanted to check to see if you think there is anything else left to do?

returnString · 2021-03-23T14:12:34Z

@returnString do you plan anything else for this PR? I think it is ready to merge but I wanted to check to see if you think there is anything else left to do?

I was looking to see if I could improve the test setup any further, but on reflection I'm actually pretty happy with the state of things; I don't think we're missing any use cases in the coverage (at least, ones I can conceive of right now). So from my side, happy with things as they are :)

alamb · 2021-03-23T22:19:53Z

Thanks again @returnString ! This is a great feature

returnString · 2021-03-21T12:30:02Z

This is an implementation of catalog and schema providers to support table namespacing (see the design doc).

I'm creating this draft PR as a supporting implementation for the proposal, to prove out that the work can be done whilst minimising API churn and still allowing for use cases that don't care at all about the notion of catalogs or schemas; in this new setup, the default namespace is datafusion.public, which will be created automatically with the default execution context config and allow for table registration.

Highlights

Datasource map removed in execution context state, replaced with catalog map
Execution context allows for registering new catalog providers
Catalog providers can be queried for their constituent schema providers
Schema providers can be queried for table providers, similarly to the old datasource map
Includes basic implementations of CatalogProvider and SchemaProvider backed by hashmaps
New TableReference enum maps to various ways of referring to a table in sql
- Bare: my_table
- Partial: schema.my_table
- Full: catalog.schema.my_table
Given a default catalog and schema, TableReference instances of any variant can be converted to a ResolvedTableReference, which always include all three components

All tests passing with fallible table management

github-actions · 2021-03-21T12:30:31Z

https://issues.apache.org/jira/browse/ARROW-12037

Dandandan · 2021-03-21T16:50:49Z

This is truly awesome @returnString !
Apart from some tests & remaining cleanup (e.g. of the "quick hack") I really like the direction this is going!

andygrove · 2021-03-21T16:50:49Z

Thanks @returnString and I appreciate the well-written design doc to explain the PR. I didn't go through the code in great detail but I like the design. I noted a couple of unwraps in the code. It would be good to document why they are safe or consider having those methods return Result if they are not safe.

returnString · 2021-03-21T17:20:12Z

Thanks both :)

I noted a couple of unwraps in the code. It would be good to document why they are safe or consider having those methods return Result if they are not safe.

I've just audited all the unwraps that are introduced in these commits, and they fall into two categories:

test cases that aren't already set up to return Result<()>
unwrapping guard objects for Mutex/RwLock - afaik these only error if the lock is poisoned and this usage is consistent with existing lock usage e.g. state access in the ExecutionContext

Apart from some tests & remaining cleanup (e.g. of the "quick hack") I really like the direction this is going!

I think the quick hack (context: retrieving a list of table names from the execution ctx, assuming a flat namespace) can probably be removed outright, as ctx.catalog("my_db")?.schema("my_schema")?.table_names() would be a much more explicit way of doing this going forward. As far as I can tell, ExecutionContext::tables isn't referenced at all in the codebase internally, but this would introduce a bit more API breakage.

As for tests, yes, the coverage so far is basically "didn't break anything" and "all three types of reference to a given table in the default catalog/schema resolve correctly", so I'll try and plan out some more in-depth testing covering e.g. tables of the same name in multiple schemas/catalogs.

codecov-io · 2021-03-21T21:20:44Z

Codecov Report

Merging #9762 (03048c6) into master (29feea0) will increase coverage by 0.00%.
The diff coverage is 80.36%.

@@ Coverage Diff @@## master #9762 +/- ##
========================================
Coverage 82.59% 82.59% ========================================
Files 248 252 +4 Lines 58294 59004 +710 ========================================
+ Hits 48149 48737 +588 - Misses 10145 10267 +122

Impacted Files	Coverage Δ
rust/datafusion/examples/flight_server.rs	`0.00% <0.00%> (ø)`
rust/datafusion/examples/simple_udaf.rs	`0.00% <ø> (ø)`
rust/datafusion/src/datasource/datasource.rs	`100.00% <ø> (ø)`
rust/datafusion/src/datasource/memory.rs	`85.15% <ø> (ø)`
rust/datafusion/src/execution/dataframe_impl.rs	`89.10% <0.00%> (-1.03%)`	⬇️
rust/datafusion/src/optimizer/constant_folding.rs	`92.30% <0.00%> (-0.36%)`	⬇️
...datafusion/src/optimizer/hash_build_probe_order.rs	`53.60% <0.00%> (-1.72%)`	⬇️
...t/datafusion/src/optimizer/projection_push_down.rs	`98.66% <ø> (ø)`
rust/datafusion/src/physical_plan/mod.rs	`88.00% <ø> (ø)`
rust/parquet/src/basic.rs	`88.59% <ø> (+1.36%)`	⬆️
... and 35 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 81d6724...03048c6. Read the comment docs.

returnString · 2021-03-21T22:15:06Z

Whilst I still want to try and conjure up a few more test cases, I think this PR is now in a state where I'm happy to receive actual code reviews, so I've removed the draft status :)

alamb · 2021-03-22T12:35:23Z

I plan to review this PR later today

alamb

All in all this looks great. Thank you @returnString !

I went through it fairly carefully and I have some suggestions but nothing that would prevent us from merging this in my opinion.

alamb · 2021-03-22T19:45:54Z

 start.elapsed().as_millis()
 );
- ctx.register_table(table, Arc::new(memtable));
+ ctx.register_table(*table, Arc::new(memtable))?;


The change to make register_table fallible is a breaking change, but a very reasonable one I think.

alamb · 2021-03-22T19:47:36Z

+// specific language governing permissions and limitations
+// under the License.
+
+//! Describes the interface and built-in implementations of catalogs,


alamb · 2021-03-22T19:49:42Z

+ fn as_any(&self) -> &dyn Any;
+
+ /// Retrieves the list of available schema names in this catalog.
+ fn schema_names(&self) -> Vec<String>;


What do you think about returning something more like Vec<&str> to prevent requiring a copy?
Ideally it would be nice if we could do something like
fn schema_names(&self) -> impl Iterator<Item=&str>
As all uses of the results here will need to iterate over the names I suspect

I iniitally tried to implement these returning &[&str] but with the threading requirements I struggled to get anything working that way. I think this would require a larger refactoring to enable, but I'm not 100% sure on that.

Vec<&str> might be possible

alamb · 2021-03-22T19:50:44Z

+}
+
+/// Simple in-memory implementation of a catalog.
+pub struct MemoryCatalogProvider {


In some other PR perhaps we can put the concrete implementations into their own modules. I don't think this one is big enough to warrant that yet, however; I just wanted to point it out

alamb · 2021-03-22T19:52:32Z

+impl<'a> TryFrom<&'a sqlparser::ast::ObjectName> for TableReference<'a> {
+ type Error = DataFusionError;
+
+ fn try_from(value: &'a sqlparser::ast::ObjectName) -> Result<Self, Self::Error> {


alamb · 2021-03-22T20:01:36Z

 }
+
+ /// Selects a name for the default catalog and schema
+ pub fn with_default_catalog_and_schema(


I like this API. 👍
I wonder if we need an API for getting default_catalog and default_schema?

alamb · 2021-03-22T20:03:24Z

@@ -1922,6 +2035,114 @@ mod tests {
 Ok(())
 }



these are excellent tests . 👍

alamb · 2021-03-22T20:03:45Z

+ &mut ctx,
+ "SELECT cat, SUM(i) AS total FROM (
+ SELECT i, 'a' AS cat FROM catalog_a.schema_a.table_a
+ UNION ALL


@DandandanUNION is already being used!

alamb · 2021-03-22T20:04:40Z

+ for table_ref in &[
+ "nonexistentschema.aggregate_test_100",
+ "nonexistentcatalog.public.aggregate_test_100",
+ "way.too.many.namespaces.as.ident.prefixes.aggregate_test_100",


Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>

alamb · 2021-03-23T13:58:33Z

@returnString do you plan anything else for this PR? I think it is ready to merge but I wanted to check to see if you think there is anything else left to do?

returnString · 2021-03-23T14:12:34Z

@returnString do you plan anything else for this PR? I think it is ready to merge but I wanted to check to see if you think there is anything else left to do?

I was looking to see if I could improve the test setup any further, but on reflection I'm actually pretty happy with the state of things; I don't think we're missing any use cases in the coverage (at least, ones I can conceive of right now). So from my side, happy with things as they are :)

alamb · 2021-03-23T22:19:53Z

Thanks again @returnString ! This is a great feature

returnString · 2021-03-21T12:30:02Z

This is an implementation of catalog and schema providers to support table namespacing (see the design doc).

I'm creating this draft PR as a supporting implementation for the proposal, to prove out that the work can be done whilst minimising API churn and still allowing for use cases that don't care at all about the notion of catalogs or schemas; in this new setup, the default namespace is datafusion.public, which will be created automatically with the default execution context config and allow for table registration.

Highlights

Datasource map removed in execution context state, replaced with catalog map
Execution context allows for registering new catalog providers
Catalog providers can be queried for their constituent schema providers
Schema providers can be queried for table providers, similarly to the old datasource map
Includes basic implementations of CatalogProvider and SchemaProvider backed by hashmaps
New TableReference enum maps to various ways of referring to a table in sql
- Bare: my_table
- Partial: schema.my_table
- Full: catalog.schema.my_table
Given a default catalog and schema, TableReference instances of any variant can be converted to a ResolvedTableReference, which always include all three components

All tests passing with fallible table management

github-actions · 2021-03-21T12:30:31Z

https://issues.apache.org/jira/browse/ARROW-12037

Dandandan · 2021-03-21T16:50:49Z

This is truly awesome @returnString !
Apart from some tests & remaining cleanup (e.g. of the "quick hack") I really like the direction this is going!

andygrove · 2021-03-21T16:50:49Z

Thanks @returnString and I appreciate the well-written design doc to explain the PR. I didn't go through the code in great detail but I like the design. I noted a couple of unwraps in the code. It would be good to document why they are safe or consider having those methods return Result if they are not safe.

returnString · 2021-03-21T17:20:12Z

Thanks both :)

I noted a couple of unwraps in the code. It would be good to document why they are safe or consider having those methods return Result if they are not safe.

I've just audited all the unwraps that are introduced in these commits, and they fall into two categories:

test cases that aren't already set up to return Result<()>
unwrapping guard objects for Mutex/RwLock - afaik these only error if the lock is poisoned and this usage is consistent with existing lock usage e.g. state access in the ExecutionContext

Apart from some tests & remaining cleanup (e.g. of the "quick hack") I really like the direction this is going!

I think the quick hack (context: retrieving a list of table names from the execution ctx, assuming a flat namespace) can probably be removed outright, as ctx.catalog("my_db")?.schema("my_schema")?.table_names() would be a much more explicit way of doing this going forward. As far as I can tell, ExecutionContext::tables isn't referenced at all in the codebase internally, but this would introduce a bit more API breakage.

As for tests, yes, the coverage so far is basically "didn't break anything" and "all three types of reference to a given table in the default catalog/schema resolve correctly", so I'll try and plan out some more in-depth testing covering e.g. tables of the same name in multiple schemas/catalogs.

codecov-io · 2021-03-21T21:20:44Z

Codecov Report

Merging #9762 (03048c6) into master (29feea0) will increase coverage by 0.00%.
The diff coverage is 80.36%.

@@ Coverage Diff @@## master #9762 +/- ##
========================================
Coverage 82.59% 82.59% ========================================
Files 248 252 +4 Lines 58294 59004 +710 ========================================
+ Hits 48149 48737 +588 - Misses 10145 10267 +122

Impacted Files	Coverage Δ
rust/datafusion/examples/flight_server.rs	`0.00% <0.00%> (ø)`
rust/datafusion/examples/simple_udaf.rs	`0.00% <ø> (ø)`
rust/datafusion/src/datasource/datasource.rs	`100.00% <ø> (ø)`
rust/datafusion/src/datasource/memory.rs	`85.15% <ø> (ø)`
rust/datafusion/src/execution/dataframe_impl.rs	`89.10% <0.00%> (-1.03%)`	⬇️
rust/datafusion/src/optimizer/constant_folding.rs	`92.30% <0.00%> (-0.36%)`	⬇️
...datafusion/src/optimizer/hash_build_probe_order.rs	`53.60% <0.00%> (-1.72%)`	⬇️
...t/datafusion/src/optimizer/projection_push_down.rs	`98.66% <ø> (ø)`
rust/datafusion/src/physical_plan/mod.rs	`88.00% <ø> (ø)`
rust/parquet/src/basic.rs	`88.59% <ø> (+1.36%)`	⬆️
... and 35 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 81d6724...03048c6. Read the comment docs.

returnString · 2021-03-21T22:15:06Z

Whilst I still want to try and conjure up a few more test cases, I think this PR is now in a state where I'm happy to receive actual code reviews, so I've removed the draft status :)

alamb · 2021-03-22T12:35:23Z

I plan to review this PR later today

alamb

All in all this looks great. Thank you @returnString !

I went through it fairly carefully and I have some suggestions but nothing that would prevent us from merging this in my opinion.

alamb · 2021-03-22T19:45:54Z

 start.elapsed().as_millis()
 );
- ctx.register_table(table, Arc::new(memtable));
+ ctx.register_table(*table, Arc::new(memtable))?;


The change to make register_table fallible is a breaking change, but a very reasonable one I think.

alamb · 2021-03-22T19:47:36Z

+// specific language governing permissions and limitations
+// under the License.
+
+//! Describes the interface and built-in implementations of catalogs,


alamb · 2021-03-22T19:49:42Z

+ fn as_any(&self) -> &dyn Any;
+
+ /// Retrieves the list of available schema names in this catalog.
+ fn schema_names(&self) -> Vec<String>;


What do you think about returning something more like Vec<&str> to prevent requiring a copy?
Ideally it would be nice if we could do something like
fn schema_names(&self) -> impl Iterator<Item=&str>
As all uses of the results here will need to iterate over the names I suspect

I iniitally tried to implement these returning &[&str] but with the threading requirements I struggled to get anything working that way. I think this would require a larger refactoring to enable, but I'm not 100% sure on that.

Vec<&str> might be possible

alamb · 2021-03-22T19:50:44Z

+}
+
+/// Simple in-memory implementation of a catalog.
+pub struct MemoryCatalogProvider {


In some other PR perhaps we can put the concrete implementations into their own modules. I don't think this one is big enough to warrant that yet, however; I just wanted to point it out

alamb · 2021-03-22T19:52:32Z

+impl<'a> TryFrom<&'a sqlparser::ast::ObjectName> for TableReference<'a> {
+ type Error = DataFusionError;
+
+ fn try_from(value: &'a sqlparser::ast::ObjectName) -> Result<Self, Self::Error> {


alamb · 2021-03-22T20:01:36Z

 }
+
+ /// Selects a name for the default catalog and schema
+ pub fn with_default_catalog_and_schema(


I like this API. 👍
I wonder if we need an API for getting default_catalog and default_schema?

alamb · 2021-03-22T20:03:24Z

@@ -1922,6 +2035,114 @@ mod tests {
 Ok(())
 }



these are excellent tests . 👍

alamb · 2021-03-22T20:03:45Z

+ &mut ctx,
+ "SELECT cat, SUM(i) AS total FROM (
+ SELECT i, 'a' AS cat FROM catalog_a.schema_a.table_a
+ UNION ALL


@DandandanUNION is already being used!

alamb · 2021-03-22T20:04:40Z

+ for table_ref in &[
+ "nonexistentschema.aggregate_test_100",
+ "nonexistentcatalog.public.aggregate_test_100",
+ "way.too.many.namespaces.as.ident.prefixes.aggregate_test_100",


Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>

alamb · 2021-03-23T13:58:33Z

@returnString do you plan anything else for this PR? I think it is ready to merge but I wanted to check to see if you think there is anything else left to do?

returnString · 2021-03-23T14:12:34Z

@returnString do you plan anything else for this PR? I think it is ready to merge but I wanted to check to see if you think there is anything else left to do?

I was looking to see if I could improve the test setup any further, but on reflection I'm actually pretty happy with the state of things; I don't think we're missing any use cases in the coverage (at least, ones I can conceive of right now). So from my side, happy with things as they are :)

alamb · 2021-03-23T22:19:53Z

Thanks again @returnString ! This is a great feature

returnString · 2021-03-21T12:30:02Z

This is an implementation of catalog and schema providers to support table namespacing (see the design doc).

I'm creating this draft PR as a supporting implementation for the proposal, to prove out that the work can be done whilst minimising API churn and still allowing for use cases that don't care at all about the notion of catalogs or schemas; in this new setup, the default namespace is datafusion.public, which will be created automatically with the default execution context config and allow for table registration.

Highlights

Datasource map removed in execution context state, replaced with catalog map
Execution context allows for registering new catalog providers
Catalog providers can be queried for their constituent schema providers
Schema providers can be queried for table providers, similarly to the old datasource map
Includes basic implementations of CatalogProvider and SchemaProvider backed by hashmaps
New TableReference enum maps to various ways of referring to a table in sql
- Bare: my_table
- Partial: schema.my_table
- Full: catalog.schema.my_table
Given a default catalog and schema, TableReference instances of any variant can be converted to a ResolvedTableReference, which always include all three components

All tests passing with fallible table management

github-actions · 2021-03-21T12:30:31Z

https://issues.apache.org/jira/browse/ARROW-12037

Dandandan · 2021-03-21T16:50:49Z

This is truly awesome @returnString !
Apart from some tests & remaining cleanup (e.g. of the "quick hack") I really like the direction this is going!

andygrove · 2021-03-21T16:50:49Z

Thanks @returnString and I appreciate the well-written design doc to explain the PR. I didn't go through the code in great detail but I like the design. I noted a couple of unwraps in the code. It would be good to document why they are safe or consider having those methods return Result if they are not safe.

returnString · 2021-03-21T17:20:12Z

Thanks both :)

I noted a couple of unwraps in the code. It would be good to document why they are safe or consider having those methods return Result if they are not safe.

I've just audited all the unwraps that are introduced in these commits, and they fall into two categories:

test cases that aren't already set up to return Result<()>
unwrapping guard objects for Mutex/RwLock - afaik these only error if the lock is poisoned and this usage is consistent with existing lock usage e.g. state access in the ExecutionContext

Apart from some tests & remaining cleanup (e.g. of the "quick hack") I really like the direction this is going!

I think the quick hack (context: retrieving a list of table names from the execution ctx, assuming a flat namespace) can probably be removed outright, as ctx.catalog("my_db")?.schema("my_schema")?.table_names() would be a much more explicit way of doing this going forward. As far as I can tell, ExecutionContext::tables isn't referenced at all in the codebase internally, but this would introduce a bit more API breakage.

As for tests, yes, the coverage so far is basically "didn't break anything" and "all three types of reference to a given table in the default catalog/schema resolve correctly", so I'll try and plan out some more in-depth testing covering e.g. tables of the same name in multiple schemas/catalogs.

codecov-io · 2021-03-21T21:20:44Z

Codecov Report

Merging #9762 (03048c6) into master (29feea0) will increase coverage by 0.00%.
The diff coverage is 80.36%.

@@ Coverage Diff @@## master #9762 +/- ##
========================================
Coverage 82.59% 82.59% ========================================
Files 248 252 +4 Lines 58294 59004 +710 ========================================
+ Hits 48149 48737 +588 - Misses 10145 10267 +122

Impacted Files	Coverage Δ
rust/datafusion/examples/flight_server.rs	`0.00% <0.00%> (ø)`
rust/datafusion/examples/simple_udaf.rs	`0.00% <ø> (ø)`
rust/datafusion/src/datasource/datasource.rs	`100.00% <ø> (ø)`
rust/datafusion/src/datasource/memory.rs	`85.15% <ø> (ø)`
rust/datafusion/src/execution/dataframe_impl.rs	`89.10% <0.00%> (-1.03%)`	⬇️
rust/datafusion/src/optimizer/constant_folding.rs	`92.30% <0.00%> (-0.36%)`	⬇️
...datafusion/src/optimizer/hash_build_probe_order.rs	`53.60% <0.00%> (-1.72%)`	⬇️
...t/datafusion/src/optimizer/projection_push_down.rs	`98.66% <ø> (ø)`
rust/datafusion/src/physical_plan/mod.rs	`88.00% <ø> (ø)`
rust/parquet/src/basic.rs	`88.59% <ø> (+1.36%)`	⬆️
... and 35 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 81d6724...03048c6. Read the comment docs.

returnString · 2021-03-21T22:15:06Z

Whilst I still want to try and conjure up a few more test cases, I think this PR is now in a state where I'm happy to receive actual code reviews, so I've removed the draft status :)

alamb · 2021-03-22T12:35:23Z

I plan to review this PR later today

alamb

All in all this looks great. Thank you @returnString !

I went through it fairly carefully and I have some suggestions but nothing that would prevent us from merging this in my opinion.

alamb · 2021-03-22T19:45:54Z

 start.elapsed().as_millis()
 );
- ctx.register_table(table, Arc::new(memtable));
+ ctx.register_table(*table, Arc::new(memtable))?;


The change to make register_table fallible is a breaking change, but a very reasonable one I think.

alamb · 2021-03-22T19:47:36Z

+// specific language governing permissions and limitations
+// under the License.
+
+//! Describes the interface and built-in implementations of catalogs,


alamb · 2021-03-22T19:49:42Z

+ fn as_any(&self) -> &dyn Any;
+
+ /// Retrieves the list of available schema names in this catalog.
+ fn schema_names(&self) -> Vec<String>;


What do you think about returning something more like Vec<&str> to prevent requiring a copy?
Ideally it would be nice if we could do something like
fn schema_names(&self) -> impl Iterator<Item=&str>
As all uses of the results here will need to iterate over the names I suspect

I iniitally tried to implement these returning &[&str] but with the threading requirements I struggled to get anything working that way. I think this would require a larger refactoring to enable, but I'm not 100% sure on that.

Vec<&str> might be possible

alamb · 2021-03-22T19:50:44Z

+}
+
+/// Simple in-memory implementation of a catalog.
+pub struct MemoryCatalogProvider {


In some other PR perhaps we can put the concrete implementations into their own modules. I don't think this one is big enough to warrant that yet, however; I just wanted to point it out

alamb · 2021-03-22T19:52:32Z

+impl<'a> TryFrom<&'a sqlparser::ast::ObjectName> for TableReference<'a> {
+ type Error = DataFusionError;
+
+ fn try_from(value: &'a sqlparser::ast::ObjectName) -> Result<Self, Self::Error> {


alamb · 2021-03-22T20:01:36Z

 }
+
+ /// Selects a name for the default catalog and schema
+ pub fn with_default_catalog_and_schema(


I like this API. 👍
I wonder if we need an API for getting default_catalog and default_schema?

alamb · 2021-03-22T20:03:24Z

@@ -1922,6 +2035,114 @@ mod tests {
 Ok(())
 }



these are excellent tests . 👍

alamb · 2021-03-22T20:03:45Z

+ &mut ctx,
+ "SELECT cat, SUM(i) AS total FROM (
+ SELECT i, 'a' AS cat FROM catalog_a.schema_a.table_a
+ UNION ALL


@DandandanUNION is already being used!

alamb · 2021-03-22T20:04:40Z

+ for table_ref in &[
+ "nonexistentschema.aggregate_test_100",
+ "nonexistentcatalog.public.aggregate_test_100",
+ "way.too.many.namespaces.as.ident.prefixes.aggregate_test_100",


Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>

alamb · 2021-03-23T13:58:33Z

@returnString do you plan anything else for this PR? I think it is ready to merge but I wanted to check to see if you think there is anything else left to do?

returnString · 2021-03-23T14:12:34Z

@returnString do you plan anything else for this PR? I think it is ready to merge but I wanted to check to see if you think there is anything else left to do?

I was looking to see if I could improve the test setup any further, but on reflection I'm actually pretty happy with the state of things; I don't think we're missing any use cases in the coverage (at least, ones I can conceive of right now). So from my side, happy with things as they are :)

alamb · 2021-03-23T22:19:53Z

Thanks again @returnString ! This is a great feature

returnString · 2021-03-21T12:30:02Z

This is an implementation of catalog and schema providers to support table namespacing (see the design doc).

I'm creating this draft PR as a supporting implementation for the proposal, to prove out that the work can be done whilst minimising API churn and still allowing for use cases that don't care at all about the notion of catalogs or schemas; in this new setup, the default namespace is datafusion.public, which will be created automatically with the default execution context config and allow for table registration.

Highlights

Datasource map removed in execution context state, replaced with catalog map
Execution context allows for registering new catalog providers
Catalog providers can be queried for their constituent schema providers
Schema providers can be queried for table providers, similarly to the old datasource map
Includes basic implementations of CatalogProvider and SchemaProvider backed by hashmaps
New TableReference enum maps to various ways of referring to a table in sql
- Bare: my_table
- Partial: schema.my_table
- Full: catalog.schema.my_table
Given a default catalog and schema, TableReference instances of any variant can be converted to a ResolvedTableReference, which always include all three components

All tests passing with fallible table management

github-actions · 2021-03-21T12:30:31Z

https://issues.apache.org/jira/browse/ARROW-12037

Dandandan · 2021-03-21T16:50:49Z

This is truly awesome @returnString !
Apart from some tests & remaining cleanup (e.g. of the "quick hack") I really like the direction this is going!

andygrove · 2021-03-21T16:50:49Z

Thanks @returnString and I appreciate the well-written design doc to explain the PR. I didn't go through the code in great detail but I like the design. I noted a couple of unwraps in the code. It would be good to document why they are safe or consider having those methods return Result if they are not safe.

returnString · 2021-03-21T17:20:12Z

Thanks both :)

I noted a couple of unwraps in the code. It would be good to document why they are safe or consider having those methods return Result if they are not safe.

I've just audited all the unwraps that are introduced in these commits, and they fall into two categories:

test cases that aren't already set up to return Result<()>
unwrapping guard objects for Mutex/RwLock - afaik these only error if the lock is poisoned and this usage is consistent with existing lock usage e.g. state access in the ExecutionContext

Apart from some tests & remaining cleanup (e.g. of the "quick hack") I really like the direction this is going!

I think the quick hack (context: retrieving a list of table names from the execution ctx, assuming a flat namespace) can probably be removed outright, as ctx.catalog("my_db")?.schema("my_schema")?.table_names() would be a much more explicit way of doing this going forward. As far as I can tell, ExecutionContext::tables isn't referenced at all in the codebase internally, but this would introduce a bit more API breakage.

As for tests, yes, the coverage so far is basically "didn't break anything" and "all three types of reference to a given table in the default catalog/schema resolve correctly", so I'll try and plan out some more in-depth testing covering e.g. tables of the same name in multiple schemas/catalogs.

codecov-io · 2021-03-21T21:20:44Z

Codecov Report

Merging #9762 (03048c6) into master (29feea0) will increase coverage by 0.00%.
The diff coverage is 80.36%.

@@ Coverage Diff @@## master #9762 +/- ##
========================================
Coverage 82.59% 82.59% ========================================
Files 248 252 +4 Lines 58294 59004 +710 ========================================
+ Hits 48149 48737 +588 - Misses 10145 10267 +122

Impacted Files	Coverage Δ
rust/datafusion/examples/flight_server.rs	`0.00% <0.00%> (ø)`
rust/datafusion/examples/simple_udaf.rs	`0.00% <ø> (ø)`
rust/datafusion/src/datasource/datasource.rs	`100.00% <ø> (ø)`
rust/datafusion/src/datasource/memory.rs	`85.15% <ø> (ø)`
rust/datafusion/src/execution/dataframe_impl.rs	`89.10% <0.00%> (-1.03%)`	⬇️
rust/datafusion/src/optimizer/constant_folding.rs	`92.30% <0.00%> (-0.36%)`	⬇️
...datafusion/src/optimizer/hash_build_probe_order.rs	`53.60% <0.00%> (-1.72%)`	⬇️
...t/datafusion/src/optimizer/projection_push_down.rs	`98.66% <ø> (ø)`
rust/datafusion/src/physical_plan/mod.rs	`88.00% <ø> (ø)`
rust/parquet/src/basic.rs	`88.59% <ø> (+1.36%)`	⬆️
... and 35 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 81d6724...03048c6. Read the comment docs.

returnString · 2021-03-21T22:15:06Z

Whilst I still want to try and conjure up a few more test cases, I think this PR is now in a state where I'm happy to receive actual code reviews, so I've removed the draft status :)

alamb · 2021-03-22T12:35:23Z

I plan to review this PR later today

alamb

All in all this looks great. Thank you @returnString !

I went through it fairly carefully and I have some suggestions but nothing that would prevent us from merging this in my opinion.

alamb · 2021-03-22T19:45:54Z

 start.elapsed().as_millis()
 );
- ctx.register_table(table, Arc::new(memtable));
+ ctx.register_table(*table, Arc::new(memtable))?;


The change to make register_table fallible is a breaking change, but a very reasonable one I think.

alamb · 2021-03-22T19:47:36Z

+// specific language governing permissions and limitations
+// under the License.
+
+//! Describes the interface and built-in implementations of catalogs,


alamb · 2021-03-22T19:49:42Z

+ fn as_any(&self) -> &dyn Any;
+
+ /// Retrieves the list of available schema names in this catalog.
+ fn schema_names(&self) -> Vec<String>;


What do you think about returning something more like Vec<&str> to prevent requiring a copy?
Ideally it would be nice if we could do something like
fn schema_names(&self) -> impl Iterator<Item=&str>
As all uses of the results here will need to iterate over the names I suspect

I iniitally tried to implement these returning &[&str] but with the threading requirements I struggled to get anything working that way. I think this would require a larger refactoring to enable, but I'm not 100% sure on that.

Vec<&str> might be possible

alamb · 2021-03-22T19:50:44Z

+}
+
+/// Simple in-memory implementation of a catalog.
+pub struct MemoryCatalogProvider {


In some other PR perhaps we can put the concrete implementations into their own modules. I don't think this one is big enough to warrant that yet, however; I just wanted to point it out

alamb · 2021-03-22T19:52:32Z

+impl<'a> TryFrom<&'a sqlparser::ast::ObjectName> for TableReference<'a> {
+ type Error = DataFusionError;
+
+ fn try_from(value: &'a sqlparser::ast::ObjectName) -> Result<Self, Self::Error> {


alamb · 2021-03-22T20:01:36Z

 }
+
+ /// Selects a name for the default catalog and schema
+ pub fn with_default_catalog_and_schema(


I like this API. 👍
I wonder if we need an API for getting default_catalog and default_schema?

alamb · 2021-03-22T20:03:24Z

@@ -1922,6 +2035,114 @@ mod tests {
 Ok(())
 }



these are excellent tests . 👍

alamb · 2021-03-22T20:03:45Z

+ &mut ctx,
+ "SELECT cat, SUM(i) AS total FROM (
+ SELECT i, 'a' AS cat FROM catalog_a.schema_a.table_a
+ UNION ALL


@DandandanUNION is already being used!

alamb · 2021-03-22T20:04:40Z

+ for table_ref in &[
+ "nonexistentschema.aggregate_test_100",
+ "nonexistentcatalog.public.aggregate_test_100",
+ "way.too.many.namespaces.as.ident.prefixes.aggregate_test_100",


Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>

alamb · 2021-03-23T13:58:33Z

@returnString do you plan anything else for this PR? I think it is ready to merge but I wanted to check to see if you think there is anything else left to do?

returnString · 2021-03-23T14:12:34Z

@returnString do you plan anything else for this PR? I think it is ready to merge but I wanted to check to see if you think there is anything else left to do?

I was looking to see if I could improve the test setup any further, but on reflection I'm actually pretty happy with the state of things; I don't think we're missing any use cases in the coverage (at least, ones I can conceive of right now). So from my side, happy with things as they are :)

alamb · 2021-03-23T22:19:53Z

Thanks again @returnString ! This is a great feature

returnString · 2021-03-21T12:30:02Z

This is an implementation of catalog and schema providers to support table namespacing (see the design doc).

I'm creating this draft PR as a supporting implementation for the proposal, to prove out that the work can be done whilst minimising API churn and still allowing for use cases that don't care at all about the notion of catalogs or schemas; in this new setup, the default namespace is datafusion.public, which will be created automatically with the default execution context config and allow for table registration.

Highlights

Datasource map removed in execution context state, replaced with catalog map
Execution context allows for registering new catalog providers
Catalog providers can be queried for their constituent schema providers
Schema providers can be queried for table providers, similarly to the old datasource map
Includes basic implementations of CatalogProvider and SchemaProvider backed by hashmaps
New TableReference enum maps to various ways of referring to a table in sql
- Bare: my_table
- Partial: schema.my_table
- Full: catalog.schema.my_table
Given a default catalog and schema, TableReference instances of any variant can be converted to a ResolvedTableReference, which always include all three components

All tests passing with fallible table management

github-actions · 2021-03-21T12:30:31Z

https://issues.apache.org/jira/browse/ARROW-12037

Dandandan · 2021-03-21T16:50:49Z

This is truly awesome @returnString !
Apart from some tests & remaining cleanup (e.g. of the "quick hack") I really like the direction this is going!

andygrove · 2021-03-21T16:50:49Z

Thanks @returnString and I appreciate the well-written design doc to explain the PR. I didn't go through the code in great detail but I like the design. I noted a couple of unwraps in the code. It would be good to document why they are safe or consider having those methods return Result if they are not safe.

returnString · 2021-03-21T17:20:12Z

Thanks both :)

I noted a couple of unwraps in the code. It would be good to document why they are safe or consider having those methods return Result if they are not safe.

I've just audited all the unwraps that are introduced in these commits, and they fall into two categories:

test cases that aren't already set up to return Result<()>
unwrapping guard objects for Mutex/RwLock - afaik these only error if the lock is poisoned and this usage is consistent with existing lock usage e.g. state access in the ExecutionContext

Apart from some tests & remaining cleanup (e.g. of the "quick hack") I really like the direction this is going!

I think the quick hack (context: retrieving a list of table names from the execution ctx, assuming a flat namespace) can probably be removed outright, as ctx.catalog("my_db")?.schema("my_schema")?.table_names() would be a much more explicit way of doing this going forward. As far as I can tell, ExecutionContext::tables isn't referenced at all in the codebase internally, but this would introduce a bit more API breakage.

As for tests, yes, the coverage so far is basically "didn't break anything" and "all three types of reference to a given table in the default catalog/schema resolve correctly", so I'll try and plan out some more in-depth testing covering e.g. tables of the same name in multiple schemas/catalogs.

codecov-io · 2021-03-21T21:20:44Z

Codecov Report

Merging #9762 (03048c6) into master (29feea0) will increase coverage by 0.00%.
The diff coverage is 80.36%.

@@ Coverage Diff @@## master #9762 +/- ##
========================================
Coverage 82.59% 82.59% ========================================
Files 248 252 +4 Lines 58294 59004 +710 ========================================
+ Hits 48149 48737 +588 - Misses 10145 10267 +122

Impacted Files	Coverage Δ
rust/datafusion/examples/flight_server.rs	`0.00% <0.00%> (ø)`
rust/datafusion/examples/simple_udaf.rs	`0.00% <ø> (ø)`
rust/datafusion/src/datasource/datasource.rs	`100.00% <ø> (ø)`
rust/datafusion/src/datasource/memory.rs	`85.15% <ø> (ø)`
rust/datafusion/src/execution/dataframe_impl.rs	`89.10% <0.00%> (-1.03%)`	⬇️
rust/datafusion/src/optimizer/constant_folding.rs	`92.30% <0.00%> (-0.36%)`	⬇️
...datafusion/src/optimizer/hash_build_probe_order.rs	`53.60% <0.00%> (-1.72%)`	⬇️
...t/datafusion/src/optimizer/projection_push_down.rs	`98.66% <ø> (ø)`
rust/datafusion/src/physical_plan/mod.rs	`88.00% <ø> (ø)`
rust/parquet/src/basic.rs	`88.59% <ø> (+1.36%)`	⬆️
... and 35 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 81d6724...03048c6. Read the comment docs.

returnString · 2021-03-21T22:15:06Z

Whilst I still want to try and conjure up a few more test cases, I think this PR is now in a state where I'm happy to receive actual code reviews, so I've removed the draft status :)

alamb · 2021-03-22T12:35:23Z

I plan to review this PR later today

alamb

All in all this looks great. Thank you @returnString !

I went through it fairly carefully and I have some suggestions but nothing that would prevent us from merging this in my opinion.

alamb · 2021-03-22T19:45:54Z

 start.elapsed().as_millis()
 );
- ctx.register_table(table, Arc::new(memtable));
+ ctx.register_table(*table, Arc::new(memtable))?;


The change to make register_table fallible is a breaking change, but a very reasonable one I think.

alamb · 2021-03-22T19:47:36Z

+// specific language governing permissions and limitations
+// under the License.
+
+//! Describes the interface and built-in implementations of catalogs,


alamb · 2021-03-22T19:49:42Z

+ fn as_any(&self) -> &dyn Any;
+
+ /// Retrieves the list of available schema names in this catalog.
+ fn schema_names(&self) -> Vec<String>;


What do you think about returning something more like Vec<&str> to prevent requiring a copy?
Ideally it would be nice if we could do something like
fn schema_names(&self) -> impl Iterator<Item=&str>
As all uses of the results here will need to iterate over the names I suspect

I iniitally tried to implement these returning &[&str] but with the threading requirements I struggled to get anything working that way. I think this would require a larger refactoring to enable, but I'm not 100% sure on that.

Vec<&str> might be possible

alamb · 2021-03-22T19:50:44Z

+}
+
+/// Simple in-memory implementation of a catalog.
+pub struct MemoryCatalogProvider {


In some other PR perhaps we can put the concrete implementations into their own modules. I don't think this one is big enough to warrant that yet, however; I just wanted to point it out

alamb · 2021-03-22T19:52:32Z

+impl<'a> TryFrom<&'a sqlparser::ast::ObjectName> for TableReference<'a> {
+ type Error = DataFusionError;
+
+ fn try_from(value: &'a sqlparser::ast::ObjectName) -> Result<Self, Self::Error> {


alamb · 2021-03-22T20:01:36Z

 }
+
+ /// Selects a name for the default catalog and schema
+ pub fn with_default_catalog_and_schema(


I like this API. 👍
I wonder if we need an API for getting default_catalog and default_schema?

alamb · 2021-03-22T20:03:24Z

@@ -1922,6 +2035,114 @@ mod tests {
 Ok(())
 }



these are excellent tests . 👍

alamb · 2021-03-22T20:03:45Z

+ &mut ctx,
+ "SELECT cat, SUM(i) AS total FROM (
+ SELECT i, 'a' AS cat FROM catalog_a.schema_a.table_a
+ UNION ALL


@DandandanUNION is already being used!

alamb · 2021-03-22T20:04:40Z

+ for table_ref in &[
+ "nonexistentschema.aggregate_test_100",
+ "nonexistentcatalog.public.aggregate_test_100",
+ "way.too.many.namespaces.as.ident.prefixes.aggregate_test_100",


Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>

alamb · 2021-03-23T13:58:33Z

@returnString do you plan anything else for this PR? I think it is ready to merge but I wanted to check to see if you think there is anything else left to do?

returnString · 2021-03-23T14:12:34Z

@returnString do you plan anything else for this PR? I think it is ready to merge but I wanted to check to see if you think there is anything else left to do?

I was looking to see if I could improve the test setup any further, but on reflection I'm actually pretty happy with the state of things; I don't think we're missing any use cases in the coverage (at least, ones I can conceive of right now). So from my side, happy with things as they are :)

alamb · 2021-03-23T22:19:53Z

Thanks again @returnString ! This is a great feature

returnString · 2021-03-21T12:30:02Z

This is an implementation of catalog and schema providers to support table namespacing (see the design doc).

I'm creating this draft PR as a supporting implementation for the proposal, to prove out that the work can be done whilst minimising API churn and still allowing for use cases that don't care at all about the notion of catalogs or schemas; in this new setup, the default namespace is datafusion.public, which will be created automatically with the default execution context config and allow for table registration.

Highlights

Datasource map removed in execution context state, replaced with catalog map
Execution context allows for registering new catalog providers
Catalog providers can be queried for their constituent schema providers
Schema providers can be queried for table providers, similarly to the old datasource map
Includes basic implementations of CatalogProvider and SchemaProvider backed by hashmaps
New TableReference enum maps to various ways of referring to a table in sql
- Bare: my_table
- Partial: schema.my_table
- Full: catalog.schema.my_table
Given a default catalog and schema, TableReference instances of any variant can be converted to a ResolvedTableReference, which always include all three components

All tests passing with fallible table management

github-actions · 2021-03-21T12:30:31Z

https://issues.apache.org/jira/browse/ARROW-12037

Dandandan · 2021-03-21T16:50:49Z

This is truly awesome @returnString !
Apart from some tests & remaining cleanup (e.g. of the "quick hack") I really like the direction this is going!

andygrove · 2021-03-21T16:50:49Z

Thanks @returnString and I appreciate the well-written design doc to explain the PR. I didn't go through the code in great detail but I like the design. I noted a couple of unwraps in the code. It would be good to document why they are safe or consider having those methods return Result if they are not safe.

returnString · 2021-03-21T17:20:12Z

Thanks both :)

I noted a couple of unwraps in the code. It would be good to document why they are safe or consider having those methods return Result if they are not safe.

I've just audited all the unwraps that are introduced in these commits, and they fall into two categories:

test cases that aren't already set up to return Result<()>
unwrapping guard objects for Mutex/RwLock - afaik these only error if the lock is poisoned and this usage is consistent with existing lock usage e.g. state access in the ExecutionContext

Apart from some tests & remaining cleanup (e.g. of the "quick hack") I really like the direction this is going!

I think the quick hack (context: retrieving a list of table names from the execution ctx, assuming a flat namespace) can probably be removed outright, as ctx.catalog("my_db")?.schema("my_schema")?.table_names() would be a much more explicit way of doing this going forward. As far as I can tell, ExecutionContext::tables isn't referenced at all in the codebase internally, but this would introduce a bit more API breakage.

As for tests, yes, the coverage so far is basically "didn't break anything" and "all three types of reference to a given table in the default catalog/schema resolve correctly", so I'll try and plan out some more in-depth testing covering e.g. tables of the same name in multiple schemas/catalogs.

codecov-io · 2021-03-21T21:20:44Z

Codecov Report

Merging #9762 (03048c6) into master (29feea0) will increase coverage by 0.00%.
The diff coverage is 80.36%.

@@ Coverage Diff @@## master #9762 +/- ##
========================================
Coverage 82.59% 82.59% ========================================
Files 248 252 +4 Lines 58294 59004 +710 ========================================
+ Hits 48149 48737 +588 - Misses 10145 10267 +122

Impacted Files	Coverage Δ
rust/datafusion/examples/flight_server.rs	`0.00% <0.00%> (ø)`
rust/datafusion/examples/simple_udaf.rs	`0.00% <ø> (ø)`
rust/datafusion/src/datasource/datasource.rs	`100.00% <ø> (ø)`
rust/datafusion/src/datasource/memory.rs	`85.15% <ø> (ø)`
rust/datafusion/src/execution/dataframe_impl.rs	`89.10% <0.00%> (-1.03%)`	⬇️
rust/datafusion/src/optimizer/constant_folding.rs	`92.30% <0.00%> (-0.36%)`	⬇️
...datafusion/src/optimizer/hash_build_probe_order.rs	`53.60% <0.00%> (-1.72%)`	⬇️
...t/datafusion/src/optimizer/projection_push_down.rs	`98.66% <ø> (ø)`
rust/datafusion/src/physical_plan/mod.rs	`88.00% <ø> (ø)`
rust/parquet/src/basic.rs	`88.59% <ø> (+1.36%)`	⬆️
... and 35 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 81d6724...03048c6. Read the comment docs.

returnString · 2021-03-21T22:15:06Z

Whilst I still want to try and conjure up a few more test cases, I think this PR is now in a state where I'm happy to receive actual code reviews, so I've removed the draft status :)

alamb · 2021-03-22T12:35:23Z

I plan to review this PR later today

alamb

All in all this looks great. Thank you @returnString !

I went through it fairly carefully and I have some suggestions but nothing that would prevent us from merging this in my opinion.

alamb · 2021-03-22T19:45:54Z

 start.elapsed().as_millis()
 );
- ctx.register_table(table, Arc::new(memtable));
+ ctx.register_table(*table, Arc::new(memtable))?;


The change to make register_table fallible is a breaking change, but a very reasonable one I think.

alamb · 2021-03-22T19:47:36Z

+// specific language governing permissions and limitations
+// under the License.
+
+//! Describes the interface and built-in implementations of catalogs,


alamb · 2021-03-22T19:49:42Z

+ fn as_any(&self) -> &dyn Any;
+
+ /// Retrieves the list of available schema names in this catalog.
+ fn schema_names(&self) -> Vec<String>;


What do you think about returning something more like Vec<&str> to prevent requiring a copy?
Ideally it would be nice if we could do something like
fn schema_names(&self) -> impl Iterator<Item=&str>
As all uses of the results here will need to iterate over the names I suspect

I iniitally tried to implement these returning &[&str] but with the threading requirements I struggled to get anything working that way. I think this would require a larger refactoring to enable, but I'm not 100% sure on that.

Vec<&str> might be possible

alamb · 2021-03-22T19:50:44Z

+}
+
+/// Simple in-memory implementation of a catalog.
+pub struct MemoryCatalogProvider {


In some other PR perhaps we can put the concrete implementations into their own modules. I don't think this one is big enough to warrant that yet, however; I just wanted to point it out

alamb · 2021-03-22T19:52:32Z

+impl<'a> TryFrom<&'a sqlparser::ast::ObjectName> for TableReference<'a> {
+ type Error = DataFusionError;
+
+ fn try_from(value: &'a sqlparser::ast::ObjectName) -> Result<Self, Self::Error> {


alamb · 2021-03-22T20:01:36Z

 }
+
+ /// Selects a name for the default catalog and schema
+ pub fn with_default_catalog_and_schema(


I like this API. 👍
I wonder if we need an API for getting default_catalog and default_schema?

alamb · 2021-03-22T20:03:24Z

@@ -1922,6 +2035,114 @@ mod tests {
 Ok(())
 }



these are excellent tests . 👍

alamb · 2021-03-22T20:03:45Z

+ &mut ctx,
+ "SELECT cat, SUM(i) AS total FROM (
+ SELECT i, 'a' AS cat FROM catalog_a.schema_a.table_a
+ UNION ALL


@DandandanUNION is already being used!

alamb · 2021-03-22T20:04:40Z

+ for table_ref in &[
+ "nonexistentschema.aggregate_test_100",
+ "nonexistentcatalog.public.aggregate_test_100",
+ "way.too.many.namespaces.as.ident.prefixes.aggregate_test_100",


Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>

alamb · 2021-03-23T13:58:33Z

@returnString do you plan anything else for this PR? I think it is ready to merge but I wanted to check to see if you think there is anything else left to do?

returnString · 2021-03-23T14:12:34Z

@returnString do you plan anything else for this PR? I think it is ready to merge but I wanted to check to see if you think there is anything else left to do?

I was looking to see if I could improve the test setup any further, but on reflection I'm actually pretty happy with the state of things; I don't think we're missing any use cases in the coverage (at least, ones I can conceive of right now). So from my side, happy with things as they are :)

alamb · 2021-03-23T22:19:53Z

Thanks again @returnString ! This is a great feature

returnString · 2021-03-21T12:30:02Z

This is an implementation of catalog and schema providers to support table namespacing (see the design doc).

I'm creating this draft PR as a supporting implementation for the proposal, to prove out that the work can be done whilst minimising API churn and still allowing for use cases that don't care at all about the notion of catalogs or schemas; in this new setup, the default namespace is datafusion.public, which will be created automatically with the default execution context config and allow for table registration.

Highlights

Datasource map removed in execution context state, replaced with catalog map
Execution context allows for registering new catalog providers
Catalog providers can be queried for their constituent schema providers
Schema providers can be queried for table providers, similarly to the old datasource map
Includes basic implementations of CatalogProvider and SchemaProvider backed by hashmaps
New TableReference enum maps to various ways of referring to a table in sql
- Bare: my_table
- Partial: schema.my_table
- Full: catalog.schema.my_table
Given a default catalog and schema, TableReference instances of any variant can be converted to a ResolvedTableReference, which always include all three components

All tests passing with fallible table management

github-actions · 2021-03-21T12:30:31Z

https://issues.apache.org/jira/browse/ARROW-12037

Dandandan · 2021-03-21T16:50:49Z

This is truly awesome @returnString !
Apart from some tests & remaining cleanup (e.g. of the "quick hack") I really like the direction this is going!

andygrove · 2021-03-21T16:50:49Z

Thanks @returnString and I appreciate the well-written design doc to explain the PR. I didn't go through the code in great detail but I like the design. I noted a couple of unwraps in the code. It would be good to document why they are safe or consider having those methods return Result if they are not safe.

returnString · 2021-03-21T17:20:12Z

Thanks both :)

I noted a couple of unwraps in the code. It would be good to document why they are safe or consider having those methods return Result if they are not safe.

I've just audited all the unwraps that are introduced in these commits, and they fall into two categories:

test cases that aren't already set up to return Result<()>
unwrapping guard objects for Mutex/RwLock - afaik these only error if the lock is poisoned and this usage is consistent with existing lock usage e.g. state access in the ExecutionContext

Apart from some tests & remaining cleanup (e.g. of the "quick hack") I really like the direction this is going!

I think the quick hack (context: retrieving a list of table names from the execution ctx, assuming a flat namespace) can probably be removed outright, as ctx.catalog("my_db")?.schema("my_schema")?.table_names() would be a much more explicit way of doing this going forward. As far as I can tell, ExecutionContext::tables isn't referenced at all in the codebase internally, but this would introduce a bit more API breakage.

As for tests, yes, the coverage so far is basically "didn't break anything" and "all three types of reference to a given table in the default catalog/schema resolve correctly", so I'll try and plan out some more in-depth testing covering e.g. tables of the same name in multiple schemas/catalogs.

codecov-io · 2021-03-21T21:20:44Z

Codecov Report

Merging #9762 (03048c6) into master (29feea0) will increase coverage by 0.00%.
The diff coverage is 80.36%.

@@ Coverage Diff @@## master #9762 +/- ##
========================================
Coverage 82.59% 82.59% ========================================
Files 248 252 +4 Lines 58294 59004 +710 ========================================
+ Hits 48149 48737 +588 - Misses 10145 10267 +122

Impacted Files	Coverage Δ
rust/datafusion/examples/flight_server.rs	`0.00% <0.00%> (ø)`
rust/datafusion/examples/simple_udaf.rs	`0.00% <ø> (ø)`
rust/datafusion/src/datasource/datasource.rs	`100.00% <ø> (ø)`
rust/datafusion/src/datasource/memory.rs	`85.15% <ø> (ø)`
rust/datafusion/src/execution/dataframe_impl.rs	`89.10% <0.00%> (-1.03%)`	⬇️
rust/datafusion/src/optimizer/constant_folding.rs	`92.30% <0.00%> (-0.36%)`	⬇️
...datafusion/src/optimizer/hash_build_probe_order.rs	`53.60% <0.00%> (-1.72%)`	⬇️
...t/datafusion/src/optimizer/projection_push_down.rs	`98.66% <ø> (ø)`
rust/datafusion/src/physical_plan/mod.rs	`88.00% <ø> (ø)`
rust/parquet/src/basic.rs	`88.59% <ø> (+1.36%)`	⬆️
... and 35 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 81d6724...03048c6. Read the comment docs.

returnString · 2021-03-21T22:15:06Z

Whilst I still want to try and conjure up a few more test cases, I think this PR is now in a state where I'm happy to receive actual code reviews, so I've removed the draft status :)

alamb · 2021-03-22T12:35:23Z

I plan to review this PR later today

alamb

All in all this looks great. Thank you @returnString !

I went through it fairly carefully and I have some suggestions but nothing that would prevent us from merging this in my opinion.

alamb · 2021-03-22T19:45:54Z

 start.elapsed().as_millis()
 );
- ctx.register_table(table, Arc::new(memtable));
+ ctx.register_table(*table, Arc::new(memtable))?;


The change to make register_table fallible is a breaking change, but a very reasonable one I think.

alamb · 2021-03-22T19:47:36Z

+// specific language governing permissions and limitations
+// under the License.
+
+//! Describes the interface and built-in implementations of catalogs,


alamb · 2021-03-22T19:49:42Z

+ fn as_any(&self) -> &dyn Any;
+
+ /// Retrieves the list of available schema names in this catalog.
+ fn schema_names(&self) -> Vec<String>;


What do you think about returning something more like Vec<&str> to prevent requiring a copy?
Ideally it would be nice if we could do something like
fn schema_names(&self) -> impl Iterator<Item=&str>
As all uses of the results here will need to iterate over the names I suspect

I iniitally tried to implement these returning &[&str] but with the threading requirements I struggled to get anything working that way. I think this would require a larger refactoring to enable, but I'm not 100% sure on that.

Vec<&str> might be possible

alamb · 2021-03-22T19:50:44Z

+}
+
+/// Simple in-memory implementation of a catalog.
+pub struct MemoryCatalogProvider {


In some other PR perhaps we can put the concrete implementations into their own modules. I don't think this one is big enough to warrant that yet, however; I just wanted to point it out

alamb · 2021-03-22T19:52:32Z

+impl<'a> TryFrom<&'a sqlparser::ast::ObjectName> for TableReference<'a> {
+ type Error = DataFusionError;
+
+ fn try_from(value: &'a sqlparser::ast::ObjectName) -> Result<Self, Self::Error> {


alamb · 2021-03-22T20:01:36Z

 }
+
+ /// Selects a name for the default catalog and schema
+ pub fn with_default_catalog_and_schema(


I like this API. 👍
I wonder if we need an API for getting default_catalog and default_schema?

alamb · 2021-03-22T20:03:24Z

@@ -1922,6 +2035,114 @@ mod tests {
 Ok(())
 }



these are excellent tests . 👍

alamb · 2021-03-22T20:03:45Z

+ &mut ctx,
+ "SELECT cat, SUM(i) AS total FROM (
+ SELECT i, 'a' AS cat FROM catalog_a.schema_a.table_a
+ UNION ALL


@DandandanUNION is already being used!

alamb · 2021-03-22T20:04:40Z

+ for table_ref in &[
+ "nonexistentschema.aggregate_test_100",
+ "nonexistentcatalog.public.aggregate_test_100",
+ "way.too.many.namespaces.as.ident.prefixes.aggregate_test_100",


Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>

alamb · 2021-03-23T13:58:33Z

@returnString do you plan anything else for this PR? I think it is ready to merge but I wanted to check to see if you think there is anything else left to do?

returnString · 2021-03-23T14:12:34Z

@returnString do you plan anything else for this PR? I think it is ready to merge but I wanted to check to see if you think there is anything else left to do?

I was looking to see if I could improve the test setup any further, but on reflection I'm actually pretty happy with the state of things; I don't think we're missing any use cases in the coverage (at least, ones I can conceive of right now). So from my side, happy with things as they are :)

alamb · 2021-03-23T22:19:53Z

Thanks again @returnString ! This is a great feature

returnString added 12 commits March 20, 2021 12:21

Require TableProvider to always implement Send + Sync
36b1703

Initial implementation of catalogs and schemas
866e85f

Implement table references
7ab647e
All tests passing with fallible table management

Add register_catalog method to context
02e83b3

Remove unnecessary unwraps from tests
4f000f3

Add test for all table reference types
fecf498

Allow downcasting of catalog/schema providers
1460598

Add doc comment for catalogs in execution state
8d30ee4

Remove unnecessary table resolution in mock context provider
5e1e942

Removed unused Display impls for table references
cf5b846

Add test for invalid qualified table references
0f33b6f

Add test case for invalid compound ident table ref
58451a0

github-actionsBot added Component: Rust - DataFusion Component: Rust labels Mar 21, 2021

returnString added 3 commits March 21, 2021 19:11

Clean up schema retrieval and comments, deprecate tables method
d79bdd5

Add tests for custom catalog and schema
66de8d3

Fix [de]register_table signatures
a18829f

Add test for cross-catalog queries
03048c6

returnString marked this pull request as ready for review March 21, 2021 22:02

alamb approved these changes Mar 22, 2021
View reviewed changes

Review suggestion for doc comment
0870acf
Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>

alamb closed this in 14441dbMar 23, 2021

returnString deleted the catalog branch March 24, 2021 19:38

returnString mentioned this pull request Mar 29, 2021
ARROW-12106: [Rust][DataFusion] Support SELECT * from information_schema.tables#9818
Closed

asfimport mentioned this pull request Mar 23, 2021
[Rust] [DataFusion] Support catalogs and schemas for table namespacing #27866
Closed

returnString added 12 commits March 20, 2021 12:21

Require TableProvider to always implement Send + Sync
36b1703

Initial implementation of catalogs and schemas
866e85f

Implement table references
7ab647e
All tests passing with fallible table management

Add register_catalog method to context
02e83b3

Remove unnecessary unwraps from tests
4f000f3

Add test for all table reference types
fecf498

Allow downcasting of catalog/schema providers
1460598

Add doc comment for catalogs in execution state
8d30ee4

Remove unnecessary table resolution in mock context provider
5e1e942

Removed unused Display impls for table references
cf5b846

Add test for invalid qualified table references
0f33b6f

Add test case for invalid compound ident table ref
58451a0

github-actionsBot added Component: Rust - DataFusion Component: Rust labels Mar 21, 2021

returnString added 3 commits March 21, 2021 19:11

Clean up schema retrieval and comments, deprecate tables method
d79bdd5

Add tests for custom catalog and schema
66de8d3

Fix [de]register_table signatures
a18829f

Add test for cross-catalog queries
03048c6

returnString marked this pull request as ready for review March 21, 2021 22:02

alamb approved these changes Mar 22, 2021
View reviewed changes

Review suggestion for doc comment
0870acf
Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>

alamb closed this in 14441dbMar 23, 2021

returnString deleted the catalog branch March 24, 2021 19:38

returnString mentioned this pull request Mar 29, 2021
ARROW-12106: [Rust][DataFusion] Support SELECT * from information_schema.tables#9818
Closed

asfimport mentioned this pull request Mar 23, 2021
[Rust] [DataFusion] Support catalogs and schemas for table namespacing #27866
Closed

returnString added 12 commits March 20, 2021 12:21

Require TableProvider to always implement Send + Sync
36b1703

Initial implementation of catalogs and schemas
866e85f

Implement table references
7ab647e
All tests passing with fallible table management

Add register_catalog method to context
02e83b3

Remove unnecessary unwraps from tests
4f000f3

Add test for all table reference types
fecf498

Allow downcasting of catalog/schema providers
1460598

Add doc comment for catalogs in execution state
8d30ee4

Remove unnecessary table resolution in mock context provider
5e1e942

Removed unused Display impls for table references
cf5b846

Add test for invalid qualified table references
0f33b6f

Add test case for invalid compound ident table ref
58451a0

github-actionsBot added Component: Rust - DataFusion Component: Rust labels Mar 21, 2021

returnString added 3 commits March 21, 2021 19:11

Clean up schema retrieval and comments, deprecate tables method
d79bdd5

Add tests for custom catalog and schema
66de8d3

Fix [de]register_table signatures
a18829f

Add test for cross-catalog queries
03048c6

returnString marked this pull request as ready for review March 21, 2021 22:02

alamb approved these changes Mar 22, 2021
View reviewed changes

Review suggestion for doc comment
0870acf
Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>

alamb closed this in 14441dbMar 23, 2021

returnString deleted the catalog branch March 24, 2021 19:38

returnString mentioned this pull request Mar 29, 2021
ARROW-12106: [Rust][DataFusion] Support SELECT * from information_schema.tables#9818
Closed

asfimport mentioned this pull request Mar 23, 2021
[Rust] [DataFusion] Support catalogs and schemas for table namespacing #27866
Closed

returnString added 12 commits March 20, 2021 12:21

Require TableProvider to always implement Send + Sync
36b1703

Initial implementation of catalogs and schemas
866e85f

Implement table references
7ab647e
All tests passing with fallible table management

Add register_catalog method to context
02e83b3

Remove unnecessary unwraps from tests
4f000f3

Add test for all table reference types
fecf498

Allow downcasting of catalog/schema providers
1460598

Add doc comment for catalogs in execution state
8d30ee4

Remove unnecessary table resolution in mock context provider
5e1e942

Removed unused Display impls for table references
cf5b846

Add test for invalid qualified table references
0f33b6f

Add test case for invalid compound ident table ref
58451a0

github-actionsBot added Component: Rust - DataFusion Component: Rust labels Mar 21, 2021

returnString added 3 commits March 21, 2021 19:11

Clean up schema retrieval and comments, deprecate tables method
d79bdd5

Add tests for custom catalog and schema
66de8d3

Fix [de]register_table signatures
a18829f

Add test for cross-catalog queries
03048c6

returnString marked this pull request as ready for review March 21, 2021 22:02

alamb approved these changes Mar 22, 2021
View reviewed changes

Review suggestion for doc comment
0870acf
Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>

alamb closed this in 14441dbMar 23, 2021

returnString deleted the catalog branch March 24, 2021 19:38

returnString mentioned this pull request Mar 29, 2021
ARROW-12106: [Rust][DataFusion] Support SELECT * from information_schema.tables#9818
Closed

asfimport mentioned this pull request Mar 23, 2021
[Rust] [DataFusion] Support catalogs and schemas for table namespacing #27866
Closed

returnString added 12 commits March 20, 2021 12:21

Require TableProvider to always implement Send + Sync
36b1703

Initial implementation of catalogs and schemas
866e85f

Implement table references
7ab647e
All tests passing with fallible table management

Add register_catalog method to context
02e83b3

Remove unnecessary unwraps from tests
4f000f3

Add test for all table reference types
fecf498

Allow downcasting of catalog/schema providers
1460598

Add doc comment for catalogs in execution state
8d30ee4

Remove unnecessary table resolution in mock context provider
5e1e942

Removed unused Display impls for table references
cf5b846

Add test for invalid qualified table references
0f33b6f

Add test case for invalid compound ident table ref
58451a0

github-actionsBot added Component: Rust - DataFusion Component: Rust labels Mar 21, 2021

returnString added 3 commits March 21, 2021 19:11

Clean up schema retrieval and comments, deprecate tables method
d79bdd5

Add tests for custom catalog and schema
66de8d3

Fix [de]register_table signatures
a18829f

Add test for cross-catalog queries
03048c6

returnString marked this pull request as ready for review March 21, 2021 22:02

alamb approved these changes Mar 22, 2021
View reviewed changes

Review suggestion for doc comment
0870acf
Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>

alamb closed this in 14441dbMar 23, 2021

returnString deleted the catalog branch March 24, 2021 19:38

returnString mentioned this pull request Mar 29, 2021
ARROW-12106: [Rust][DataFusion] Support SELECT * from information_schema.tables#9818
Closed

asfimport mentioned this pull request Mar 23, 2021
[Rust] [DataFusion] Support catalogs and schemas for table namespacing #27866
Closed

returnString added 12 commits March 20, 2021 12:21

Require TableProvider to always implement Send + Sync
36b1703

Initial implementation of catalogs and schemas
866e85f

Implement table references
7ab647e
All tests passing with fallible table management

Add register_catalog method to context
02e83b3

Remove unnecessary unwraps from tests
4f000f3

Add test for all table reference types
fecf498

Allow downcasting of catalog/schema providers
1460598

Add doc comment for catalogs in execution state
8d30ee4

Remove unnecessary table resolution in mock context provider
5e1e942

Removed unused Display impls for table references
cf5b846

Add test for invalid qualified table references
0f33b6f

Add test case for invalid compound ident table ref
58451a0

github-actionsBot added Component: Rust - DataFusion Component: Rust labels Mar 21, 2021

returnString added 3 commits March 21, 2021 19:11

Clean up schema retrieval and comments, deprecate tables method
d79bdd5

Add tests for custom catalog and schema
66de8d3

Fix [de]register_table signatures
a18829f

Add test for cross-catalog queries
03048c6

returnString marked this pull request as ready for review March 21, 2021 22:02

alamb approved these changes Mar 22, 2021
View reviewed changes

Review suggestion for doc comment
0870acf
Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>

alamb closed this in 14441dbMar 23, 2021

returnString deleted the catalog branch March 24, 2021 19:38

returnString mentioned this pull request Mar 29, 2021
ARROW-12106: [Rust][DataFusion] Support SELECT * from information_schema.tables#9818
Closed

asfimport mentioned this pull request Mar 23, 2021
[Rust] [DataFusion] Support catalogs and schemas for table namespacing #27866
Closed

returnString added 12 commits March 20, 2021 12:21

Require TableProvider to always implement Send + Sync
36b1703

Initial implementation of catalogs and schemas
866e85f

Implement table references
7ab647e
All tests passing with fallible table management

Add register_catalog method to context
02e83b3

Remove unnecessary unwraps from tests
4f000f3

Add test for all table reference types
fecf498

Allow downcasting of catalog/schema providers
1460598

Add doc comment for catalogs in execution state
8d30ee4

Remove unnecessary table resolution in mock context provider
5e1e942

Removed unused Display impls for table references
cf5b846

Add test for invalid qualified table references
0f33b6f

Add test case for invalid compound ident table ref
58451a0

github-actionsBot added Component: Rust - DataFusion Component: Rust labels Mar 21, 2021

returnString added 3 commits March 21, 2021 19:11

Clean up schema retrieval and comments, deprecate tables method
d79bdd5

Add tests for custom catalog and schema
66de8d3

Fix [de]register_table signatures
a18829f

Add test for cross-catalog queries
03048c6

returnString marked this pull request as ready for review March 21, 2021 22:02

alamb approved these changes Mar 22, 2021
View reviewed changes

Review suggestion for doc comment
0870acf
Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>

alamb closed this in 14441dbMar 23, 2021

returnString deleted the catalog branch March 24, 2021 19:38

returnString mentioned this pull request Mar 29, 2021
ARROW-12106: [Rust][DataFusion] Support SELECT * from information_schema.tables#9818
Closed

asfimport mentioned this pull request Mar 23, 2021
[Rust] [DataFusion] Support catalogs and schemas for table namespacing #27866
Closed

returnString added 12 commits March 20, 2021 12:21

Require TableProvider to always implement Send + Sync
36b1703

Initial implementation of catalogs and schemas
866e85f

Implement table references
7ab647e
All tests passing with fallible table management

Add register_catalog method to context
02e83b3

Remove unnecessary unwraps from tests
4f000f3

Add test for all table reference types
fecf498

Allow downcasting of catalog/schema providers
1460598

Add doc comment for catalogs in execution state
8d30ee4

Remove unnecessary table resolution in mock context provider
5e1e942

Removed unused Display impls for table references
cf5b846

Add test for invalid qualified table references
0f33b6f

Add test case for invalid compound ident table ref
58451a0

github-actionsBot added Component: Rust - DataFusion Component: Rust labels Mar 21, 2021

returnString added 3 commits March 21, 2021 19:11

Clean up schema retrieval and comments, deprecate tables method
d79bdd5

Add tests for custom catalog and schema
66de8d3

Fix [de]register_table signatures
a18829f

Add test for cross-catalog queries
03048c6

returnString marked this pull request as ready for review March 21, 2021 22:02

alamb approved these changes Mar 22, 2021
View reviewed changes

Review suggestion for doc comment
0870acf
Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>

alamb closed this in 14441dbMar 23, 2021

returnString deleted the catalog branch March 24, 2021 19:38

returnString mentioned this pull request Mar 29, 2021
ARROW-12106: [Rust][DataFusion] Support SELECT * from information_schema.tables#9818
Closed

asfimport mentioned this pull request Mar 23, 2021
[Rust] [DataFusion] Support catalogs and schemas for table namespacing #27866
Closed

Uh oh!

Conversation

returnString commented Mar 21, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Highlights

Uh oh!

github-actionsBot commented Mar 21, 2021

Uh oh!

Dandandan commented Mar 21, 2021

Uh oh!

andygrove commented Mar 21, 2021

Uh oh!

returnString commented Mar 21, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Mar 21, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

returnString commented Mar 21, 2021

Uh oh!

alamb commented Mar 22, 2021

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb commented Mar 23, 2021

Uh oh!

returnString commented Mar 23, 2021

Uh oh!

alamb commented Mar 23, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Uh oh!

Conversation

returnString commented Mar 21, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Highlights

Uh oh!

github-actionsBot commented Mar 21, 2021

Uh oh!

Dandandan commented Mar 21, 2021

Uh oh!

andygrove commented Mar 21, 2021

Uh oh!

returnString commented Mar 21, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Mar 21, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

returnString commented Mar 21, 2021•
edited
Loading

returnString commented Mar 21, 2021•
edited
Loading

codecov-io commented Mar 21, 2021•
edited
Loading

returnString commented Mar 21, 2021•
edited
Loading

returnString commented Mar 21, 2021•
edited
Loading

codecov-io commented Mar 21, 2021•
edited
Loading