generalize table provider df impl

[cpcloud]

This is a draft PR following up a discussion on #1699.

Issues:

1. The execution context state is using Default, it compiles and tests pass,
   but I don't know if that's the right thing to do there.
2. It doesn't appear that it's possible to allow registration of dyn DataFrame without the trait_upcasting feature, which is unstable.
3. The schema methods are ambiguous when use datafusion::prelude::* is used (because both traits have a schema method), which makes using either's method without fully qualified syntax pretty ugly.

[houqp]

@cpcloud good point on lack of access to the trait_upcasting feature :( I found an interesting workaround for this online based on https://articles.bchlr.de/traits-dynamic-dispatch-upcasting:

diff --git a/datafusion/src/datasource/datasource.rs b/datafusion/src/datasource/datasource.rs
index 1b59c857f..703d14493 100644
--- a/datafusion/src/datasource/datasource.rs
+++ b/datafusion/src/datasource/datasource.rs
@@ -55,9 +55,15 @@ pubenumTableType{Temporary,}
+pubtraitAsDynTableProvider{
+ fn as_dyn_table<'a>(self:Arc<Self>) -> Arc<dynTableProvider + 'a>
+ where
+ Self:'a;
+}
+
/// Source table #[async_trait]
-pubtraitTableProvider:Sync + Send{
+pubtraitTableProvider:Sync + Send + AsDynTableProvider{/// Returns the table provider as [`Any`](std::any::Any) so that it can be/// downcast to a specific implementation.fnas_any(&self) -> &dynAny;
@@ -94,3 +100,12 @@ pubtraitTableProvider:Sync + Send{Ok(TableProviderFilterPushDown::Unsupported)}}
+
+impl<T:TableProvider + Sized>AsDynTableProviderforT{
+ fnas_dyn_table<'a>(self:Arc<Self>) -> Arc<dynTableProvider + 'a>
+ where
+ Self:'a,
+ {
+ self
+ }
+}
diff --git a/datafusion/src/execution/dataframe_impl.rs b/datafusion/src/execution/dataframe_impl.rs
index c1933adaa..eb59e3a93 100644
--- a/datafusion/src/execution/dataframe_impl.rs
+++ b/datafusion/src/execution/dataframe_impl.rs
@@ -550,7 +550,7 @@ modtests{letmut ctx = ExecutionContext::new();// register a dataframe as a table
- ctx.register_table("test_table", df)?;
+ ctx.register_table("test_table", df.as_dyn_table())?;Ok(())}

The schema method name conflict is unfortunate and I don't have a good solution for that other than renaming one of the methods :( Perhaps other contributors can jump in to help suggest workarounds.

[houqp]

As for the first issue you mentioned, I think we need to avoid referencing both DataFrameImpl and ExecutionContext within the default TableProvider implementation because these two types are specific to the DataFrameImpl implementation of the trait.

However, now that we don't have a separate Dataframe implementation in ballista anymore, I am not sure what we gain from having the DataFrame trait defined. @andygrove@realno@yahoNanJing@alamb@Dandandan do you think it makes sense to just replace DataFrameImpl with Dataframe now and get rid of all the trait object overheads?

[alamb]

do you think it makes sense to just replace DataFrameImpl with Dataframe now and get rid of all the trait object overheads?

It seems to make sense to me -- it seems to me if someone wanted a different dataframe implementation they would probably just implement it themselves rather than trying to conform to an interface defined by DataFusion. However, I may not be a good arbiter of what is useful and what is not as I don't use the DataFrame impl much

I can't seem to find where the DataFrame trait was defined now (the history gets murky for me when the code got moved around)

[realno]

do you think it makes sense to just replace DataFrameImpl with Dataframe now and get rid of all the trait object overheads?

I like the idea of simplifying interfaces - given the issues with this PR I think it seems to be a good idea to just consolidate DataFrame. Please take this as an opinion given my limited knowledge of the code base. Another thought is, will this have any impact on things like Python library?

[houqp]

@alamb it's defined in https://github.com/apache/arrow-datafusion/blob/master/datafusion/src/dataframe.rs.

@realno should have no impact to the python binding since traits are implementation details in the Rust land that won't get exposed to the python runtime.

Let's leave this on for couple days to see if @andygrove@jimexist@yahoNanJing has a strong opinion on this or not.

[alamb]

@alamb it's defined in https://github.com/apache/arrow-datafusion/blob/master/datafusion/src/dataframe.rs.

Thanks @houqp -- looking at the history of that file the DataFrame trait appears to have been around a long time, lending credence to the idea that it might be due for some refactoring

[matthewmturner]

While working on #1922 i also ran into some issues with the DataFrame trait that i wasnt expecting. Since I hadnt really worked on that part of the code base before i was also surprised it was a trait and not a struct. I would be supportive of moving DataFrame to a struct as well.

[houqp]

Great, I filed #1962 to track the work.

[andygrove]

I think my original intent was to provide different implementations for DataFusion vs Ballista but we have a better solution now with the DataFusion context allowing a pluggable query planner.

[houqp]

thank you for your research on this @cpcloud , I believe #1998 has this covered now :)