ARROW-12037: [Rust] [DataFusion] Support catalogs and schemas for table namespacing #9762

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

rust/benchmarks/src/bin/tpch.rs

                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

rust/datafusion/src/catalog/catalog.rs

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


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

rust/datafusion/src/catalog/catalog.rs

+    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

rust/datafusion/src/catalog/catalog.rs

+}
+
+/// 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

rust/datafusion/src/catalog/mod.rs

+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

rust/datafusion/src/execution/context.rs

    }
+
+    /// 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?

rust/datafusion/src/execution/context.rs

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

rust/datafusion/src/execution/context.rs

        df.collect().await.expect_err("query not supported");
        Ok(())
    }



these are excellent tests . 👍

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

rust/datafusion/src/execution/context.rs

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


@Dandandan UNION is already being used!

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

rust/datafusion/tests/sql.rs

+    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-actions bot 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 14441db Mar 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!

ARROW-12037: [Rust] [DataFusion] Support catalogs and schemas for table namespacing #9762

ARROW-12037: [Rust] [DataFusion] Support catalogs and schemas for table namespacing #9762

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-actions bot 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

returnString commented Mar 21, 2021 •

edited

Loading

returnString commented Mar 21, 2021 •

edited

Loading

codecov-io commented Mar 21, 2021 •

edited

Loading