You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2022/01/28 17:04:23 UTC
[GitHub] [arrow-datafusion] cpcloud opened a new pull request #1699: Implement TableProvider for DataFrameImpl
cpcloud opened a new pull request #1699:
URL: https://github.com/apache/arrow-datafusion/pull/1699
# Which issue does this PR close?
Closes #1698.
# Rationale for this change
See #1698.
# What changes are included in this PR?
1. An implementation of `TableProvider` for `DataFrameImpl`
2. A test to verify that a table can be registered in context and that the
registered table's plan is equivalent to the one used for registration
# Are there any user-facing changes?
Yes, the ability to register plan through `DataFrameImpl` is a new addition to the API.
I think rustdoc should take care of listing the additional `impl` of `TableProvider`.
Let me know if there's another place I should add docs.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] cpcloud commented on a change in pull request #1699: Implement TableProvider for DataFrameImpl
Posted by GitBox <gi...@apache.org>.
cpcloud commented on a change in pull request #1699:
URL: https://github.com/apache/arrow-datafusion/pull/1699#discussion_r795250108
##########
File path: datafusion/src/execution/dataframe_impl.rs
##########
@@ -62,6 +68,60 @@ impl DataFrameImpl {
}
}
+#[async_trait]
+impl TableProvider for DataFrameImpl {
Review comment:
I don't think there's any way to achieve this behavior
```rust
let df = test_table().await?.select_columns(&["c1", "c12"])?;
let mut ctx = ExecutionContext::new();
// register a dataframe as a table
ctx.register_table("test_table", df)?;
```
without `trait_upcasting` 😞, even with a blanket `impl` plus an additional trait bound. I'll put up a draft PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] houqp commented on a change in pull request #1699: Implement TableProvider for DataFrameImpl
Posted by GitBox <gi...@apache.org>.
houqp commented on a change in pull request #1699:
URL: https://github.com/apache/arrow-datafusion/pull/1699#discussion_r794994473
##########
File path: datafusion/src/execution/dataframe_impl.rs
##########
@@ -62,6 +68,60 @@ impl DataFrameImpl {
}
}
+#[async_trait]
+impl TableProvider for DataFrameImpl {
Review comment:
perhaps we can add `TableProvider` as one of the trait bounds for the `Dataframe` trait so the trait object can be used to register tables without casting to a concrete type.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] cpcloud commented on a change in pull request #1699: Implement TableProvider for DataFrameImpl
Posted by GitBox <gi...@apache.org>.
cpcloud commented on a change in pull request #1699:
URL: https://github.com/apache/arrow-datafusion/pull/1699#discussion_r795221151
##########
File path: datafusion/src/execution/dataframe_impl.rs
##########
@@ -62,6 +68,60 @@ impl DataFrameImpl {
}
}
+#[async_trait]
+impl TableProvider for DataFrameImpl {
Review comment:
I think I may have a prototype, I'll push something up here in a bit.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb merged pull request #1699: Implement TableProvider for DataFrameImpl
Posted by GitBox <gi...@apache.org>.
alamb merged pull request #1699:
URL: https://github.com/apache/arrow-datafusion/pull/1699
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on a change in pull request #1699: Implement TableProvider for DataFrameImpl
Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #1699:
URL: https://github.com/apache/arrow-datafusion/pull/1699#discussion_r794748183
##########
File path: datafusion/src/execution/dataframe_impl.rs
##########
@@ -62,6 +68,65 @@ impl DataFrameImpl {
}
}
+#[async_trait]
+impl TableProvider for DataFrameImpl {
+ fn as_any(&self) -> &dyn Any {
+ self
+ }
+
+ fn schema(&self) -> SchemaRef {
+ Arc::new(Schema::new(
+ self.plan
+ .schema()
+ .fields()
+ .iter()
+ .map(|f| f.field().clone())
+ .collect(),
+ ))
+ }
Review comment:
I think you can use some rust-fu to make this shorter if you want
```suggestion
fn schema(&self) -> SchemaRef {
let schema: Schema = self.plan.schema().as_ref().into();
Arc::new(schema)
}
```
##########
File path: datafusion/src/execution/dataframe_impl.rs
##########
@@ -62,6 +68,65 @@ impl DataFrameImpl {
}
}
+#[async_trait]
+impl TableProvider for DataFrameImpl {
+ fn as_any(&self) -> &dyn Any {
+ self
+ }
+
+ fn schema(&self) -> SchemaRef {
+ Arc::new(Schema::new(
+ self.plan
+ .schema()
+ .fields()
+ .iter()
+ .map(|f| f.field().clone())
+ .collect(),
+ ))
+ }
+
+ fn table_type(&self) -> TableType {
+ TableType::View
+ }
+
+ async fn scan(
+ &self,
+ projection: &Option<Vec<usize>>,
+ filters: &[Expr],
+ limit: Option<usize>,
+ ) -> Result<Arc<dyn ExecutionPlan>> {
+ let expr = projection
+ .as_ref()
+ // construct projections
+ .map_or_else(
+ || Ok(Arc::new(Self::new(self.ctx_state.clone(), &self.plan)) as Arc<_>),
+ |projection| {
+ let schema = TableProvider::schema(self).project(projection)?;
+ let names = schema
+ .fields()
+ .iter()
+ .map(|field| field.name().as_str())
+ .collect::<Vec<_>>();
+ self.select_columns(names.as_slice())
Review comment:
this is cool
##########
File path: datafusion/src/execution/dataframe_impl.rs
##########
@@ -488,6 +553,17 @@ mod tests {
Ok(())
}
+ #[tokio::test]
+ async fn register_table() -> Result<()> {
+ let df = test_table().await?.select_columns(&["c1", "c3"])?;
+ let mut ctx = ExecutionContext::new();
+ let df_impl = DataFrameImpl::new(ctx.state.clone(), &df.to_logical_plan());
+ ctx.register_table("test_table", Arc::new(df_impl))?;
+ let table = ctx.table("test_table")?;
+ assert_same_plan(&table.to_logical_plan(), &table.to_logical_plan());
+ Ok(())
Review comment:
I would also recommend at test that runs the plan too (either via `SQL` or via `df_impl.collect()`) to exercise the `scan` code.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] cpcloud commented on a change in pull request #1699: Implement TableProvider for DataFrameImpl
Posted by GitBox <gi...@apache.org>.
cpcloud commented on a change in pull request #1699:
URL: https://github.com/apache/arrow-datafusion/pull/1699#discussion_r795038010
##########
File path: datafusion/src/execution/dataframe_impl.rs
##########
@@ -488,6 +553,17 @@ mod tests {
Ok(())
}
+ #[tokio::test]
+ async fn register_table() -> Result<()> {
+ let df = test_table().await?.select_columns(&["c1", "c3"])?;
+ let mut ctx = ExecutionContext::new();
+ let df_impl = DataFrameImpl::new(ctx.state.clone(), &df.to_logical_plan());
+ ctx.register_table("test_table", Arc::new(df_impl))?;
+ let table = ctx.table("test_table")?;
+ assert_same_plan(&table.to_logical_plan(), &table.to_logical_plan());
+ Ok(())
Review comment:
@alamb @houqp
It looks like I can't compare the plans directly, because they are referring to different tables:
```
Projection: #aggregate_test_100.c1, #aggregate_test_100.c12
TableScan: aggregate_test_100 projection=None
Projection: #test_table.c1, #test_table.c12
TableScan: test_table projection=None
```
Does it suffice to compare only the results of execution?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] houqp commented on a change in pull request #1699: Implement TableProvider for DataFrameImpl
Posted by GitBox <gi...@apache.org>.
houqp commented on a change in pull request #1699:
URL: https://github.com/apache/arrow-datafusion/pull/1699#discussion_r794994159
##########
File path: datafusion/src/execution/dataframe_impl.rs
##########
@@ -488,6 +548,17 @@ mod tests {
Ok(())
}
+ #[tokio::test]
+ async fn register_table() -> Result<()> {
+ let df = test_table().await?.select_columns(&["c1", "c3"])?;
+ let mut ctx = ExecutionContext::new();
+ let df_impl = DataFrameImpl::new(ctx.state.clone(), &df.to_logical_plan());
+ ctx.register_table("test_table", Arc::new(df_impl))?;
+ let table = ctx.table("test_table")?;
+ assert_same_plan(&table.to_logical_plan(), &table.to_logical_plan());
Review comment:
did you mean to compare `table.to_logical_plan()` with `df.to_logical_plan()` instead?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] cpcloud commented on a change in pull request #1699: Implement TableProvider for DataFrameImpl
Posted by GitBox <gi...@apache.org>.
cpcloud commented on a change in pull request #1699:
URL: https://github.com/apache/arrow-datafusion/pull/1699#discussion_r795218939
##########
File path: datafusion/src/execution/dataframe_impl.rs
##########
@@ -62,6 +68,60 @@ impl DataFrameImpl {
}
}
+#[async_trait]
+impl TableProvider for DataFrameImpl {
Review comment:
Interesting, I don't think you can impl a trait for a trait (since the introduction of the `dyn` requirement) but we might be able to do a blanket impl like
```rust
impl<D: DataFrame> TableProvider for D {
...
}
```
Along with the trait bound, I think this will work to allow any DataFrame implementation to be used as a table provider.
The main question for me is: given that the `scan` method returns a physical plan, do I need to add a new DataFrame method to get a physical plan? Or is there another way to get that with existing APIs?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] cpcloud commented on a change in pull request #1699: Implement TableProvider for DataFrameImpl
Posted by GitBox <gi...@apache.org>.
cpcloud commented on a change in pull request #1699:
URL: https://github.com/apache/arrow-datafusion/pull/1699#discussion_r795036649
##########
File path: datafusion/src/execution/dataframe_impl.rs
##########
@@ -488,6 +548,17 @@ mod tests {
Ok(())
}
+ #[tokio::test]
+ async fn register_table() -> Result<()> {
+ let df = test_table().await?.select_columns(&["c1", "c3"])?;
+ let mut ctx = ExecutionContext::new();
+ let df_impl = DataFrameImpl::new(ctx.state.clone(), &df.to_logical_plan());
+ ctx.register_table("test_table", Arc::new(df_impl))?;
+ let table = ctx.table("test_table")?;
+ assert_same_plan(&table.to_logical_plan(), &table.to_logical_plan());
Review comment:
I did! Thanks and nice catch.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on a change in pull request #1699: Implement TableProvider for DataFrameImpl
Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #1699:
URL: https://github.com/apache/arrow-datafusion/pull/1699#discussion_r795040558
##########
File path: datafusion/src/execution/dataframe_impl.rs
##########
@@ -488,6 +553,17 @@ mod tests {
Ok(())
}
+ #[tokio::test]
+ async fn register_table() -> Result<()> {
+ let df = test_table().await?.select_columns(&["c1", "c3"])?;
+ let mut ctx = ExecutionContext::new();
+ let df_impl = DataFrameImpl::new(ctx.state.clone(), &df.to_logical_plan());
+ ctx.register_table("test_table", Arc::new(df_impl))?;
+ let table = ctx.table("test_table")?;
+ assert_same_plan(&table.to_logical_plan(), &table.to_logical_plan());
+ Ok(())
Review comment:
> Does it suffice to compare only the results of execution?
Yes, sorry this is what I meant
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on a change in pull request #1699: Implement TableProvider for DataFrameImpl
Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #1699:
URL: https://github.com/apache/arrow-datafusion/pull/1699#discussion_r795161808
##########
File path: datafusion/src/execution/dataframe_impl.rs
##########
@@ -62,6 +68,60 @@ impl DataFrameImpl {
}
}
+#[async_trait]
+impl TableProvider for DataFrameImpl {
Review comment:
I think @houqp was suggesting changing
```rust
trait DataFrame {
...
}
```
To something like
```rust
trait DataFrame:: TableProvider {
}
```
And then `impl TableProvider` for `DataFrame` (rather than `DataFrameImpl`) -- which would mean that any `&dyn DataFrame` could be used as a table provider
If this is interesting, I am happy to file a ticket describing it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on a change in pull request #1699: Implement TableProvider for DataFrameImpl
Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #1699:
URL: https://github.com/apache/arrow-datafusion/pull/1699#discussion_r795161808
##########
File path: datafusion/src/execution/dataframe_impl.rs
##########
@@ -62,6 +68,60 @@ impl DataFrameImpl {
}
}
+#[async_trait]
+impl TableProvider for DataFrameImpl {
Review comment:
I think @houqp was suggesting changing
```rust
trait DataFrame {
...
}
```
To something like
```rust
trait DataFrame:: TableProvider {
}
```
And then `impl TableProvider` for `DataFrame` (rather than `DataFrameImpl`)
If this is interesting, I am happy to file a ticket describing it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] cpcloud commented on a change in pull request #1699: Implement TableProvider for DataFrameImpl
Posted by GitBox <gi...@apache.org>.
cpcloud commented on a change in pull request #1699:
URL: https://github.com/apache/arrow-datafusion/pull/1699#discussion_r795037968
##########
File path: datafusion/src/execution/dataframe_impl.rs
##########
@@ -62,6 +68,60 @@ impl DataFrameImpl {
}
}
+#[async_trait]
+impl TableProvider for DataFrameImpl {
Review comment:
Can you give an example of what you mean?
It won't be possible to register `Arc<dyn DataFrame>`s directly even if `DataFrame` has a trait bound of `TableProvider`, because that would require trait upcasting IIUC, which is still an unstable feature.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on a change in pull request #1699: Implement TableProvider for DataFrameImpl
Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #1699:
URL: https://github.com/apache/arrow-datafusion/pull/1699#discussion_r795040657
##########
File path: datafusion/src/execution/dataframe_impl.rs
##########
@@ -488,6 +547,44 @@ mod tests {
Ok(())
}
+ #[tokio::test]
+ async fn register_table() -> Result<()> {
+ let df = test_table().await?.select_columns(&["c1", "c12"])?;
+ let mut ctx = ExecutionContext::new();
+ let df_impl = DataFrameImpl::new(ctx.state.clone(), &df.to_logical_plan());
+
+ // register a dataframe as a table
+ ctx.register_table("test_table", Arc::new(df_impl))?;
+
+ // pull the table out and compare the plans
+ let table = ctx.table("test_table")?;
+
+ // check that we correctly read from the table
+ let df_results = &table
Review comment:
```suggestion
let df_results = &df_impl
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org