You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "cisaacson (via GitHub)" <gi...@apache.org> on 2024/04/07 20:32:12 UTC
[PR] Further clarification of the supports_filters_pushdown doc [arrow-datafusion]
cisaacson opened a new pull request, #9988:
URL: https://github.com/apache/arrow-datafusion/pull/9988
## Which issue does this PR close?
Closes #9987.
## Rationale for this change
<!--
To make it easier to understand how this fn should be used.
-->
## What changes are included in this PR?
Doc change only.
## Are these changes tested?
Did check that the code compiles.
## Are there any user-facing changes?
Doc change
--
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
Re: [PR] Further clarification of the supports_filters_pushdown documentation [arrow-datafusion]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #9988:
URL: https://github.com/apache/arrow-datafusion/pull/9988#discussion_r1558419795
##########
datafusion/core/src/datasource/provider.rs:
##########
@@ -161,20 +161,73 @@ pub trait TableProvider: Sync + Send {
/// Specify if DataFusion should provide filter expressions to the
/// TableProvider to apply *during* the scan.
///
+ /// Some TableProviders can evaluate filters more efficiently than the
+ /// `Filter` operator in DataFusion, for example by using an index.
+ ///
/// The return value must have one element for each filter expression passed
/// in. The value of each element indicates if the TableProvider can apply
- /// that particular filter during the scan.
+ /// that particular filter during the scan. The position in the return value
+ /// Vec corresponds to the expression in the `filters` input.
+ ///
+ /// Here is an example of how this can be done:
+ ///
+ /// ```rust
+ /// use datafusion::error::{Result, DataFusionError};
+ /// use datafusion_expr::{Expr, TableProviderFilterPushDown};
+ ///
+ /// // Override the supports_filters_pushdown to evaluate which expressions
+ /// // to accept as pushdown predicates.
+ /// fn supports_filters_pushdown(&self, filters: &[&Expr]) -> Result<Vec<TableProviderFilterPushDown>> {
Review Comment:
This doesn't compile for me when I run `cargo test --doc` locally and is also failing in CI. One issue here is that this function needs to be part of an `impl` block for a struct rather than a standalone function, otherwise there is no `self` to refer to. I also added some comments for smaller issues that caused compilation issues locally.
--
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
Re: [PR] Further clarification of the supports_filters_pushdown doc [arrow-datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #9988:
URL: https://github.com/apache/arrow-datafusion/pull/9988#discussion_r1555586423
##########
datafusion/core/src/datasource/provider.rs:
##########
@@ -161,20 +161,34 @@ pub trait TableProvider: Sync + Send {
/// Specify if DataFusion should provide filter expressions to the
/// TableProvider to apply *during* the scan.
///
- /// The return value must have one element for each filter expression passed
- /// in. The value of each element indicates if the TableProvider can apply
- /// that particular filter during the scan.
- ///
/// Some TableProviders can evaluate filters more efficiently than the
/// `Filter` operator in DataFusion, for example by using an index.
+ ///
+ /// The return value must have one element for each filter expression passed
+ /// in. The value of each element indicates if the TableProvider can apply
+ /// that particular filter during the scan. The position in the return value
+ /// Vec corresponds to the expression in the `filters` input. One approach is to
+ /// allocate a result Vec equal to the size of the `filters` length, then
+ /// loop the `filters` array and indicate the proper level of support (or not)
+ /// for each individual filter expression according to the capabilities
+ /// of the TableProvider.
Review Comment:
While accurate, I do wonder if the description of a loop in English distracts from the rest of the text. I think it is worth considering omitting that text like this
```suggestion
/// Vec corresponds to the expression in the `filters` input.
```
(and perhaps instead add an example showing the loop that is described)
--
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
Re: [PR] Further clarification of the supports_filters_pushdown documentation [arrow-datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #9988:
URL: https://github.com/apache/arrow-datafusion/pull/9988#discussion_r1559289424
##########
datafusion/core/src/datasource/provider.rs:
##########
@@ -161,20 +161,83 @@ pub trait TableProvider: Sync + Send {
/// Specify if DataFusion should provide filter expressions to the
/// TableProvider to apply *during* the scan.
///
- /// The return value must have one element for each filter expression passed
- /// in. The value of each element indicates if the TableProvider can apply
- /// that particular filter during the scan.
- ///
/// Some TableProviders can evaluate filters more efficiently than the
/// `Filter` operator in DataFusion, for example by using an index.
///
- /// By default, returns [`Unsupported`] for all filters, meaning no filters
- /// will be provided to [`Self::scan`]. If the TableProvider can implement
- /// filter pushdown, it should return either [`Exact`] or [`Inexact`].
+ /// # Parameters and Return Value
+ ///
+ /// The return `Vec` must have one element for each element of the `filters`
+ /// argument. The value of each element indicates if the TableProvider can
+ /// apply the corresponding filter during the scan. The position in the return
+ /// value corresponds to the expression in the `filters` parameter.
+ ///
+ /// If the length of the resulting `Vec` does not match the `filters` input
+ /// an error will be thrown.
+ ///
+ /// Each element in the resulting `Vec` is one of the following:
+ /// * [`Exact`] or [`Inexact`]: The TableProvider can apply the filter
+ /// during scan
+ /// * [`Unsupported`]: The TableProvider cannot apply the filter during scan
+ ///
+ /// By default, this function returns [`Unsupported`] for all filters,
+ /// meaning no filters will be provided to [`Self::scan`].
///
/// [`Unsupported`]: TableProviderFilterPushDown::Unsupported
/// [`Exact`]: TableProviderFilterPushDown::Exact
/// [`Inexact`]: TableProviderFilterPushDown::Inexact
+ /// # Example
+ ///
+ /// ```rust
+ /// # use std::any::Any; /// #
Review Comment:
In rustdocs you can use `#` to have the line not show up in the example but still be included
##########
datafusion/core/src/datasource/provider.rs:
##########
@@ -161,20 +161,83 @@ pub trait TableProvider: Sync + Send {
/// Specify if DataFusion should provide filter expressions to the
/// TableProvider to apply *during* the scan.
///
- /// The return value must have one element for each filter expression passed
- /// in. The value of each element indicates if the TableProvider can apply
- /// that particular filter during the scan.
- ///
/// Some TableProviders can evaluate filters more efficiently than the
/// `Filter` operator in DataFusion, for example by using an index.
///
- /// By default, returns [`Unsupported`] for all filters, meaning no filters
- /// will be provided to [`Self::scan`]. If the TableProvider can implement
- /// filter pushdown, it should return either [`Exact`] or [`Inexact`].
+ /// # Parameters and Return Value
Review Comment:
I made these their own heading and tried to reduce the redundancy
##########
datafusion/core/src/datasource/provider.rs:
##########
@@ -161,20 +161,83 @@ pub trait TableProvider: Sync + Send {
/// Specify if DataFusion should provide filter expressions to the
/// TableProvider to apply *during* the scan.
///
- /// The return value must have one element for each filter expression passed
- /// in. The value of each element indicates if the TableProvider can apply
- /// that particular filter during the scan.
- ///
/// Some TableProviders can evaluate filters more efficiently than the
/// `Filter` operator in DataFusion, for example by using an index.
///
- /// By default, returns [`Unsupported`] for all filters, meaning no filters
- /// will be provided to [`Self::scan`]. If the TableProvider can implement
- /// filter pushdown, it should return either [`Exact`] or [`Inexact`].
+ /// # Parameters and Return Value
+ ///
+ /// The return `Vec` must have one element for each element of the `filters`
+ /// argument. The value of each element indicates if the TableProvider can
+ /// apply the corresponding filter during the scan. The position in the return
+ /// value corresponds to the expression in the `filters` parameter.
+ ///
+ /// If the length of the resulting `Vec` does not match the `filters` input
+ /// an error will be thrown.
+ ///
+ /// Each element in the resulting `Vec` is one of the following:
+ /// * [`Exact`] or [`Inexact`]: The TableProvider can apply the filter
+ /// during scan
+ /// * [`Unsupported`]: The TableProvider cannot apply the filter during scan
+ ///
+ /// By default, this function returns [`Unsupported`] for all filters,
+ /// meaning no filters will be provided to [`Self::scan`].
///
/// [`Unsupported`]: TableProviderFilterPushDown::Unsupported
/// [`Exact`]: TableProviderFilterPushDown::Exact
/// [`Inexact`]: TableProviderFilterPushDown::Inexact
+ /// # Example
+ ///
+ /// ```rust
+ /// # use std::any::Any; /// #
+ /// # use std::sync::Arc;
+ /// # use arrow_schema::SchemaRef;
+ /// # use async_trait::async_trait;
+ /// # use datafusion::datasource::TableProvider;
+ /// # use datafusion::error::{Result, DataFusionError};
+ /// # use datafusion::execution::context::SessionState;
+ /// # use datafusion_expr::{Expr, TableProviderFilterPushDown, TableType};
+ /// # use datafusion_physical_plan::ExecutionPlan;
+ /// // Define a struct that implements the TableProvider trait
+ /// struct TestDataSource {}
+ ///
+ /// #[async_trait]
+ /// impl TableProvider for TestDataSource {
+ /// # fn as_any(&self) -> &dyn Any { todo!() }
+ /// # fn schema(&self) -> SchemaRef { todo!() }
+ /// # fn table_type(&self) -> TableType { todo!() }
+ /// # async fn scan(&self, s: &SessionState, p: Option<&Vec<usize>>, f: &[Expr], l: Option<usize>) -> Result<Arc<dyn ExecutionPlan>> {
+ /// todo!()
+ /// # }
+ /// // Override the supports_filters_pushdown to evaluate which expressions
+ /// // to accept as pushdown predicates.
+ /// fn supports_filters_pushdown(&self, filters: &[&Expr]) -> Result<Vec<TableProviderFilterPushDown>> {
Review Comment:
I reworked this example to a more functional style (the previous one didn't compile due to some error about moving out of `filters`)
--
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
Re: [PR] Further clarification of the supports_filters_pushdown doc [arrow-datafusion]
Posted by "cisaacson (via GitHub)" <gi...@apache.org>.
cisaacson commented on code in PR #9988:
URL: https://github.com/apache/arrow-datafusion/pull/9988#discussion_r1556329614
##########
datafusion/core/src/datasource/provider.rs:
##########
@@ -161,20 +161,54 @@ pub trait TableProvider: Sync + Send {
/// Specify if DataFusion should provide filter expressions to the
/// TableProvider to apply *during* the scan.
///
- /// The return value must have one element for each filter expression passed
- /// in. The value of each element indicates if the TableProvider can apply
- /// that particular filter during the scan.
- ///
/// Some TableProviders can evaluate filters more efficiently than the
/// `Filter` operator in DataFusion, for example by using an index.
- ///
- /// By default, returns [`Unsupported`] for all filters, meaning no filters
+ ///
+ /// The return value must have one element for each filter expression passed
+ /// in. The value of each element indicates if the TableProvider can apply
+ /// that particular filter during the scan. The position in the return value
+ /// Vec corresponds to the expression in the `filters` input.
+ ///
+ /// Here is an example of how this can be done:
+ ///
+ /// ```
+ /// fn supports_filters_pushdown(
+ /// &self,
+ /// filters: &[&Expr],) -> Result<Vec<TableProviderFilterPushDown>> {
+ ///
+ /// let result_vec: Vec<TableProviderFilterPushDown> = Vec::with_capacity(&filters.len());
+ /// for i in 0..filters.len() {
+ /// // Evaluate a filter
+ /// let filter = filters[i];
+ ///
+ /// // Evaluate a filter to support here
+ /// if filter ... {
+ /// result_vec.push(TableProviderFilterPushDown::Exact);
+ /// } else {
+ /// result_vec.push(TableProviderFilterPushDown::Unsupported);
+ /// }
+ /// }
Review Comment:
Let me know if the latest commit looks OK to you. I didn't see the functional style example until just now, but my example does compile. I bet you may have a better way to do the whole thing, I'm interested in that feedback too. I can look at this again this evening.
--
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
Re: [PR] Further clarification of the supports_filters_pushdown documentation [arrow-datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #9988:
URL: https://github.com/apache/arrow-datafusion/pull/9988#issuecomment-2048040248
Thanks again for all the help and bearing with our back and forth @cisaacson -- I think these docs are getting quite good :bowtie:
--
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
Re: [PR] Further clarification of the supports_filters_pushdown documentation [arrow-datafusion]
Posted by "cisaacson (via GitHub)" <gi...@apache.org>.
cisaacson commented on code in PR #9988:
URL: https://github.com/apache/arrow-datafusion/pull/9988#discussion_r1559349670
##########
datafusion/core/src/datasource/provider.rs:
##########
@@ -161,20 +161,83 @@ pub trait TableProvider: Sync + Send {
/// Specify if DataFusion should provide filter expressions to the
/// TableProvider to apply *during* the scan.
///
- /// The return value must have one element for each filter expression passed
- /// in. The value of each element indicates if the TableProvider can apply
- /// that particular filter during the scan.
- ///
/// Some TableProviders can evaluate filters more efficiently than the
/// `Filter` operator in DataFusion, for example by using an index.
///
- /// By default, returns [`Unsupported`] for all filters, meaning no filters
- /// will be provided to [`Self::scan`]. If the TableProvider can implement
- /// filter pushdown, it should return either [`Exact`] or [`Inexact`].
+ /// # Parameters and Return Value
+ ///
+ /// The return `Vec` must have one element for each element of the `filters`
+ /// argument. The value of each element indicates if the TableProvider can
+ /// apply the corresponding filter during the scan. The position in the return
+ /// value corresponds to the expression in the `filters` parameter.
+ ///
+ /// If the length of the resulting `Vec` does not match the `filters` input
+ /// an error will be thrown.
+ ///
+ /// Each element in the resulting `Vec` is one of the following:
+ /// * [`Exact`] or [`Inexact`]: The TableProvider can apply the filter
+ /// during scan
+ /// * [`Unsupported`]: The TableProvider cannot apply the filter during scan
+ ///
+ /// By default, this function returns [`Unsupported`] for all filters,
+ /// meaning no filters will be provided to [`Self::scan`].
///
/// [`Unsupported`]: TableProviderFilterPushDown::Unsupported
/// [`Exact`]: TableProviderFilterPushDown::Exact
/// [`Inexact`]: TableProviderFilterPushDown::Inexact
+ /// # Example
+ ///
+ /// ```rust
+ /// # use std::any::Any; /// #
+ /// # use std::sync::Arc;
+ /// # use arrow_schema::SchemaRef;
+ /// # use async_trait::async_trait;
+ /// # use datafusion::datasource::TableProvider;
+ /// # use datafusion::error::{Result, DataFusionError};
+ /// # use datafusion::execution::context::SessionState;
+ /// # use datafusion_expr::{Expr, TableProviderFilterPushDown, TableType};
+ /// # use datafusion_physical_plan::ExecutionPlan;
+ /// // Define a struct that implements the TableProvider trait
+ /// struct TestDataSource {}
+ ///
+ /// #[async_trait]
+ /// impl TableProvider for TestDataSource {
+ /// # fn as_any(&self) -> &dyn Any { todo!() }
+ /// # fn schema(&self) -> SchemaRef { todo!() }
+ /// # fn table_type(&self) -> TableType { todo!() }
+ /// # async fn scan(&self, s: &SessionState, p: Option<&Vec<usize>>, f: &[Expr], l: Option<usize>) -> Result<Arc<dyn ExecutionPlan>> {
+ /// todo!()
+ /// # }
+ /// // Override the supports_filters_pushdown to evaluate which expressions
+ /// // to accept as pushdown predicates.
+ /// fn supports_filters_pushdown(&self, filters: &[&Expr]) -> Result<Vec<TableProviderFilterPushDown>> {
Review Comment:
@alamb Very nice, now I can see how to do this the right way, the code and comments look very good. And understanding the `#` in doc comments helps too.
Looks great to me.
--
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
Re: [PR] Further clarification of the supports_filters_pushdown documentation [arrow-datafusion]
Posted by "cisaacson (via GitHub)" <gi...@apache.org>.
cisaacson commented on code in PR #9988:
URL: https://github.com/apache/arrow-datafusion/pull/9988#discussion_r1558714781
##########
datafusion/core/src/datasource/provider.rs:
##########
@@ -161,20 +161,73 @@ pub trait TableProvider: Sync + Send {
/// Specify if DataFusion should provide filter expressions to the
/// TableProvider to apply *during* the scan.
///
+ /// Some TableProviders can evaluate filters more efficiently than the
+ /// `Filter` operator in DataFusion, for example by using an index.
+ ///
/// The return value must have one element for each filter expression passed
/// in. The value of each element indicates if the TableProvider can apply
- /// that particular filter during the scan.
+ /// that particular filter during the scan. The position in the return value
+ /// Vec corresponds to the expression in the `filters` input.
+ ///
+ /// Here is an example of how this can be done:
+ ///
+ /// ```rust
+ /// use datafusion::error::{Result, DataFusionError};
+ /// use datafusion_expr::{Expr, TableProviderFilterPushDown};
+ ///
+ /// // Override the supports_filters_pushdown to evaluate which expressions
+ /// // to accept as pushdown predicates.
+ /// fn supports_filters_pushdown(&self, filters: &[&Expr]) -> Result<Vec<TableProviderFilterPushDown>> {
Review Comment:
Thanks @andygrove , all good catches.
To add the `impl` it would be for `TableProvider` so do I need to create an entire struct, etc. to do that? Any help on how to contribute a minimal doc example would be great. I use VS Code with Rust Analyzer, I also do not see an easy way to copy code into the doc and make it a fn doc comment.
Any help appreciated, then I'll get it cleaned up. I see other areas I can help with the docs too once I have a fully verified example.
I do have all of the fixes done now other than the `impl` must have all methods required. I can do that but it seems like it will clutter the doc example, any easy way to do that? If I need to have them I'll just add them.
--
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
Re: [PR] Further clarification of the supports_filters_pushdown documentation [arrow-datafusion]
Posted by "cisaacson (via GitHub)" <gi...@apache.org>.
cisaacson commented on code in PR #9988:
URL: https://github.com/apache/arrow-datafusion/pull/9988#discussion_r1559361960
##########
datafusion/core/src/datasource/provider.rs:
##########
@@ -161,20 +161,83 @@ pub trait TableProvider: Sync + Send {
/// Specify if DataFusion should provide filter expressions to the
/// TableProvider to apply *during* the scan.
///
- /// The return value must have one element for each filter expression passed
- /// in. The value of each element indicates if the TableProvider can apply
- /// that particular filter during the scan.
- ///
/// Some TableProviders can evaluate filters more efficiently than the
/// `Filter` operator in DataFusion, for example by using an index.
///
- /// By default, returns [`Unsupported`] for all filters, meaning no filters
- /// will be provided to [`Self::scan`]. If the TableProvider can implement
- /// filter pushdown, it should return either [`Exact`] or [`Inexact`].
+ /// # Parameters and Return Value
Review Comment:
Thanks @alamb , looks great now. And thanks for the tips on the doc comments.
--
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
Re: [PR] Further clarification of the supports_filters_pushdown doc [arrow-datafusion]
Posted by "cisaacson (via GitHub)" <gi...@apache.org>.
cisaacson commented on code in PR #9988:
URL: https://github.com/apache/arrow-datafusion/pull/9988#discussion_r1555866848
##########
datafusion/core/src/datasource/provider.rs:
##########
@@ -161,20 +161,34 @@ pub trait TableProvider: Sync + Send {
/// Specify if DataFusion should provide filter expressions to the
/// TableProvider to apply *during* the scan.
///
- /// The return value must have one element for each filter expression passed
- /// in. The value of each element indicates if the TableProvider can apply
- /// that particular filter during the scan.
- ///
/// Some TableProviders can evaluate filters more efficiently than the
/// `Filter` operator in DataFusion, for example by using an index.
+ ///
+ /// The return value must have one element for each filter expression passed
+ /// in. The value of each element indicates if the TableProvider can apply
+ /// that particular filter during the scan. The position in the return value
+ /// Vec corresponds to the expression in the `filters` input. One approach is to
+ /// allocate a result Vec equal to the size of the `filters` length, then
+ /// loop the `filters` array and indicate the proper level of support (or not)
+ /// for each individual filter expression according to the capabilities
+ /// of the TableProvider.
Review Comment:
@alamb Let me know what you think of the example code I added. It does not specify any specific logic, just provides the general idea.
Thanks for the feedback too.
--
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
Re: [PR] Further clarification of the supports_filters_pushdown documentation [arrow-datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #9988:
URL: https://github.com/apache/arrow-datafusion/pull/9988#issuecomment-2047300848
Thanks @cisaacson -- I took the liberty of getting the CI to pass as well as moving a bit of the docs around by pushing some commits to your branch.
I'll leave some comments inline
I think it is looking pretty good now:
--
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
Re: [PR] Further clarification of the supports_filters_pushdown documentation [arrow-datafusion]
Posted by "cisaacson (via GitHub)" <gi...@apache.org>.
cisaacson commented on code in PR #9988:
URL: https://github.com/apache/arrow-datafusion/pull/9988#discussion_r1558714781
##########
datafusion/core/src/datasource/provider.rs:
##########
@@ -161,20 +161,73 @@ pub trait TableProvider: Sync + Send {
/// Specify if DataFusion should provide filter expressions to the
/// TableProvider to apply *during* the scan.
///
+ /// Some TableProviders can evaluate filters more efficiently than the
+ /// `Filter` operator in DataFusion, for example by using an index.
+ ///
/// The return value must have one element for each filter expression passed
/// in. The value of each element indicates if the TableProvider can apply
- /// that particular filter during the scan.
+ /// that particular filter during the scan. The position in the return value
+ /// Vec corresponds to the expression in the `filters` input.
+ ///
+ /// Here is an example of how this can be done:
+ ///
+ /// ```rust
+ /// use datafusion::error::{Result, DataFusionError};
+ /// use datafusion_expr::{Expr, TableProviderFilterPushDown};
+ ///
+ /// // Override the supports_filters_pushdown to evaluate which expressions
+ /// // to accept as pushdown predicates.
+ /// fn supports_filters_pushdown(&self, filters: &[&Expr]) -> Result<Vec<TableProviderFilterPushDown>> {
Review Comment:
Thanks @andygrove , all good catches.
To add the `impl` it would be for `TableProvider` so do I need to create an entire struct, etc. to do that? Any help on how to contribute a minimal doc example would be great. I use VS Code with Rust Analyzer, I also do not see an easy way to copy code into the doc and make it a fn doc comment.
Any help appreciated, then I'll get it cleaned up. I see other areas I can help with the docs too once I have a fully verified example.
--
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
Re: [PR] Further clarification of the supports_filters_pushdown documentation [arrow-datafusion]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #9988:
URL: https://github.com/apache/arrow-datafusion/pull/9988#discussion_r1558416804
##########
datafusion/core/src/datasource/provider.rs:
##########
@@ -161,20 +161,73 @@ pub trait TableProvider: Sync + Send {
/// Specify if DataFusion should provide filter expressions to the
/// TableProvider to apply *during* the scan.
///
+ /// Some TableProviders can evaluate filters more efficiently than the
+ /// `Filter` operator in DataFusion, for example by using an index.
+ ///
/// The return value must have one element for each filter expression passed
/// in. The value of each element indicates if the TableProvider can apply
- /// that particular filter during the scan.
+ /// that particular filter during the scan. The position in the return value
+ /// Vec corresponds to the expression in the `filters` input.
+ ///
+ /// Here is an example of how this can be done:
+ ///
+ /// ```rust
+ /// use datafusion::error::{Result, DataFusionError};
+ /// use datafusion_expr::{Expr, TableProviderFilterPushDown};
+ ///
+ /// // Override the supports_filters_pushdown to evaluate which expressions
+ /// // to accept as pushdown predicates.
+ /// fn supports_filters_pushdown(&self, filters: &[&Expr]) -> Result<Vec<TableProviderFilterPushDown>> {
///
- /// Some TableProviders can evaluate filters more efficiently than the
- /// `Filter` operator in DataFusion, for example by using an index.
+ /// let mut result_vec: Vec<TableProviderFilterPushDown> = Vec::with_capacity(&filters.len());
///
- /// By default, returns [`Unsupported`] for all filters, meaning no filters
+ /// // Process each filter
+ /// for i in 0..filters.len() {
+ /// let expr = *filters[i];
+ ///
+ /// let table_provider_filter_pushdown = match expr {
+ /// // This example only supports a between expr with a single column name.
+ /// Expr::Between(between_expr) => {
+ /// between_expr.expr.try_into_col()
+ /// .map(|column| {
+ /// if column.name = "c1".to_string() {
Review Comment:
This needs to be a comparison and not an assignment
```suggestion
/// if column.name == "c1".to_string() {
```
--
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
Re: [PR] Further clarification of the supports_filters_pushdown documentation [arrow-datafusion]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove merged PR #9988:
URL: https://github.com/apache/arrow-datafusion/pull/9988
--
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
Re: [PR] Further clarification of the supports_filters_pushdown doc [arrow-datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #9988:
URL: https://github.com/apache/arrow-datafusion/pull/9988#discussion_r1556212768
##########
datafusion/core/src/datasource/provider.rs:
##########
@@ -161,20 +161,54 @@ pub trait TableProvider: Sync + Send {
/// Specify if DataFusion should provide filter expressions to the
/// TableProvider to apply *during* the scan.
///
- /// The return value must have one element for each filter expression passed
- /// in. The value of each element indicates if the TableProvider can apply
- /// that particular filter during the scan.
- ///
/// Some TableProviders can evaluate filters more efficiently than the
/// `Filter` operator in DataFusion, for example by using an index.
- ///
- /// By default, returns [`Unsupported`] for all filters, meaning no filters
+ ///
+ /// The return value must have one element for each filter expression passed
+ /// in. The value of each element indicates if the TableProvider can apply
+ /// that particular filter during the scan. The position in the return value
+ /// Vec corresponds to the expression in the `filters` input.
+ ///
+ /// Here is an example of how this can be done:
+ ///
+ /// ```
Review Comment:
In rust any example like this is actually compiled and run (more details are here https://doc.rust-lang.org/rustdoc/write-documentation/documentation-tests.html)
So to get this example to work you'll need to add some `use` etc commands
##########
datafusion/core/src/datasource/provider.rs:
##########
@@ -161,20 +161,54 @@ pub trait TableProvider: Sync + Send {
/// Specify if DataFusion should provide filter expressions to the
/// TableProvider to apply *during* the scan.
///
- /// The return value must have one element for each filter expression passed
- /// in. The value of each element indicates if the TableProvider can apply
- /// that particular filter during the scan.
- ///
/// Some TableProviders can evaluate filters more efficiently than the
/// `Filter` operator in DataFusion, for example by using an index.
- ///
- /// By default, returns [`Unsupported`] for all filters, meaning no filters
+ ///
+ /// The return value must have one element for each filter expression passed
+ /// in. The value of each element indicates if the TableProvider can apply
+ /// that particular filter during the scan. The position in the return value
+ /// Vec corresponds to the expression in the `filters` input.
+ ///
+ /// Here is an example of how this can be done:
+ ///
+ /// ```
+ /// fn supports_filters_pushdown(
+ /// &self,
+ /// filters: &[&Expr],) -> Result<Vec<TableProviderFilterPushDown>> {
+ ///
+ /// let result_vec: Vec<TableProviderFilterPushDown> = Vec::with_capacity(&filters.len());
+ /// for i in 0..filters.len() {
+ /// // Evaluate a filter
+ /// let filter = filters[i];
+ ///
+ /// // Evaluate a filter to support here
+ /// if filter ... {
+ /// result_vec.push(TableProviderFilterPushDown::Exact);
+ /// } else {
+ /// result_vec.push(TableProviderFilterPushDown::Unsupported);
+ /// }
+ /// }
Review Comment:
If you like a more functional style, I think you can write this same logic in this way:
```suggestion
/// let result_vec: Vec<TableProviderFilterPushDown> = filters.iter()
/// .map (|filter|
/// // Evaluate a filter
///
/// // Evaluate a filter to support here
/// if can_support_filter(filter){
/// TableProviderFilterPushDown::Exact
/// } else {
/// TableProviderFilterPushDown::Unsupported;
/// }
/// }).collect();
```
--
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
Re: [PR] Further clarification of the supports_filters_pushdown doc [arrow-datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #9988:
URL: https://github.com/apache/arrow-datafusion/pull/9988#discussion_r1556334496
##########
datafusion/core/src/datasource/provider.rs:
##########
@@ -161,20 +161,71 @@ pub trait TableProvider: Sync + Send {
/// Specify if DataFusion should provide filter expressions to the
/// TableProvider to apply *during* the scan.
///
- /// The return value must have one element for each filter expression passed
- /// in. The value of each element indicates if the TableProvider can apply
- /// that particular filter during the scan.
- ///
/// Some TableProviders can evaluate filters more efficiently than the
/// `Filter` operator in DataFusion, for example by using an index.
- ///
- /// By default, returns [`Unsupported`] for all filters, meaning no filters
+ ///
+ /// The return value must have one element for each filter expression passed
+ /// in. The value of each element indicates if the TableProvider can apply
+ /// that particular filter during the scan. The position in the return value
+ /// Vec corresponds to the expression in the `filters` input.
+ ///
+ /// Here is an example of how this can be done:
+ ///
+ /// ```rust
+ /// use datafusion::error::{Result, DataFusionError};
+ /// use datafusion_expr::{Expr, TableProviderFilterPushDown};
+ ///
+ /// fn supports_filters_pushdown(
+ /// &self,
+ /// filters: &[&Expr],) -> Result<Vec<TableProviderFilterPushDown>> {
+ ///
+ /// let mut result_vec: Vec<TableProviderFilterPushDown> = Vec::with_capacity(&filters.len());
+ ///
+ /// for i in 0..filters.len() {
+ /// let expr = *filters[i];
+ ///
+ /// match expr {
+ /// Expr::Between(between_expr) => {
+ /// match between_expr.expr.try_into_col().ok() {
+ /// Some(column) => {
+ /// let column_name = column.name;
+ /// if column_name = "c1".to_string() {
+ /// result_vec.push(TableProviderFilterPushDown::Exact);
+ /// } else {
+ /// result_vec.push(TableProviderFilterPushDown::Unsupported);
+ /// }
+ /// }
+ /// None => {
+ /// return Err(DataFusionError::Execution(format!("Could not get column. between_expr: {:?}", between_expr)));
+ /// }
Review Comment:
The example might be more general if it didn't error but instead returned `Unsupported` for other exprs
Also, if you want to get your functional Rust programming on, you can use something like this (untested):
```rust
between_expr.expr.try_into_col()
.map(|column| {
if &column.name = "c1" {
TableProviderFilterPushDown::Exact
} else {
TableProviderFilterPushDown::Unsupported
}
})
// if not a column reference, return unsupported
.unwrap_or(TableProviderFilterPushDown::Unsupported)
```
##########
datafusion/core/src/datasource/provider.rs:
##########
@@ -161,20 +161,71 @@ pub trait TableProvider: Sync + Send {
/// Specify if DataFusion should provide filter expressions to the
/// TableProvider to apply *during* the scan.
///
- /// The return value must have one element for each filter expression passed
- /// in. The value of each element indicates if the TableProvider can apply
- /// that particular filter during the scan.
- ///
/// Some TableProviders can evaluate filters more efficiently than the
/// `Filter` operator in DataFusion, for example by using an index.
- ///
- /// By default, returns [`Unsupported`] for all filters, meaning no filters
+ ///
+ /// The return value must have one element for each filter expression passed
+ /// in. The value of each element indicates if the TableProvider can apply
+ /// that particular filter during the scan. The position in the return value
+ /// Vec corresponds to the expression in the `filters` input.
+ ///
+ /// Here is an example of how this can be done:
+ ///
+ /// ```rust
+ /// use datafusion::error::{Result, DataFusionError};
+ /// use datafusion_expr::{Expr, TableProviderFilterPushDown};
+ ///
+ /// fn supports_filters_pushdown(
+ /// &self,
+ /// filters: &[&Expr],) -> Result<Vec<TableProviderFilterPushDown>> {
+ ///
+ /// let mut result_vec: Vec<TableProviderFilterPushDown> = Vec::with_capacity(&filters.len());
+ ///
+ /// for i in 0..filters.len() {
+ /// let expr = *filters[i];
+ ///
+ /// match expr {
Review Comment:
For examples, I think it helps to add a comment explaining what it is doing for new readers
for example, here you might add:
```suggestion
/// // Return `Exact` if we have a way to evaluate the expression exactly. In this
/// // we have a way to evaluate expressions on column c1, but not other columns,
/// match expr {
```
--
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
Re: [PR] Further clarification of the supports_filters_pushdown doc [arrow-datafusion]
Posted by "cisaacson (via GitHub)" <gi...@apache.org>.
cisaacson commented on code in PR #9988:
URL: https://github.com/apache/arrow-datafusion/pull/9988#discussion_r1556329614
##########
datafusion/core/src/datasource/provider.rs:
##########
@@ -161,20 +161,54 @@ pub trait TableProvider: Sync + Send {
/// Specify if DataFusion should provide filter expressions to the
/// TableProvider to apply *during* the scan.
///
- /// The return value must have one element for each filter expression passed
- /// in. The value of each element indicates if the TableProvider can apply
- /// that particular filter during the scan.
- ///
/// Some TableProviders can evaluate filters more efficiently than the
/// `Filter` operator in DataFusion, for example by using an index.
- ///
- /// By default, returns [`Unsupported`] for all filters, meaning no filters
+ ///
+ /// The return value must have one element for each filter expression passed
+ /// in. The value of each element indicates if the TableProvider can apply
+ /// that particular filter during the scan. The position in the return value
+ /// Vec corresponds to the expression in the `filters` input.
+ ///
+ /// Here is an example of how this can be done:
+ ///
+ /// ```
+ /// fn supports_filters_pushdown(
+ /// &self,
+ /// filters: &[&Expr],) -> Result<Vec<TableProviderFilterPushDown>> {
+ ///
+ /// let result_vec: Vec<TableProviderFilterPushDown> = Vec::with_capacity(&filters.len());
+ /// for i in 0..filters.len() {
+ /// // Evaluate a filter
+ /// let filter = filters[i];
+ ///
+ /// // Evaluate a filter to support here
+ /// if filter ... {
+ /// result_vec.push(TableProviderFilterPushDown::Exact);
+ /// } else {
+ /// result_vec.push(TableProviderFilterPushDown::Unsupported);
+ /// }
+ /// }
Review Comment:
Let me know if the latest commit looks OK to you. I didn't see the functional style example until just now, but my example does compile. I bet you may have a better way to do the whole thing, I'm interested in that feedback too. I can look at this again this evening. And I could easily convert it to functional style too. If I put another fn in to evaluate each filter (which is how I would do it) then that would have to be in the example too? That will make it a lot longer so I put just one Expr eval to show the concept.
--
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
Re: [PR] Further clarification of the supports_filters_pushdown documentation [arrow-datafusion]
Posted by "cisaacson (via GitHub)" <gi...@apache.org>.
cisaacson commented on code in PR #9988:
URL: https://github.com/apache/arrow-datafusion/pull/9988#discussion_r1556483089
##########
datafusion/core/src/datasource/provider.rs:
##########
@@ -161,20 +161,71 @@ pub trait TableProvider: Sync + Send {
/// Specify if DataFusion should provide filter expressions to the
/// TableProvider to apply *during* the scan.
///
- /// The return value must have one element for each filter expression passed
- /// in. The value of each element indicates if the TableProvider can apply
- /// that particular filter during the scan.
- ///
/// Some TableProviders can evaluate filters more efficiently than the
/// `Filter` operator in DataFusion, for example by using an index.
- ///
- /// By default, returns [`Unsupported`] for all filters, meaning no filters
+ ///
+ /// The return value must have one element for each filter expression passed
+ /// in. The value of each element indicates if the TableProvider can apply
+ /// that particular filter during the scan. The position in the return value
+ /// Vec corresponds to the expression in the `filters` input.
+ ///
+ /// Here is an example of how this can be done:
+ ///
+ /// ```rust
+ /// use datafusion::error::{Result, DataFusionError};
+ /// use datafusion_expr::{Expr, TableProviderFilterPushDown};
+ ///
+ /// fn supports_filters_pushdown(
+ /// &self,
+ /// filters: &[&Expr],) -> Result<Vec<TableProviderFilterPushDown>> {
+ ///
+ /// let mut result_vec: Vec<TableProviderFilterPushDown> = Vec::with_capacity(&filters.len());
+ ///
+ /// for i in 0..filters.len() {
+ /// let expr = *filters[i];
+ ///
+ /// match expr {
+ /// Expr::Between(between_expr) => {
+ /// match between_expr.expr.try_into_col().ok() {
+ /// Some(column) => {
+ /// let column_name = column.name;
+ /// if column_name = "c1".to_string() {
+ /// result_vec.push(TableProviderFilterPushDown::Exact);
+ /// } else {
+ /// result_vec.push(TableProviderFilterPushDown::Unsupported);
+ /// }
+ /// }
+ /// None => {
+ /// return Err(DataFusionError::Execution(format!("Could not get column. between_expr: {:?}", between_expr)));
+ /// }
Review Comment:
I took out the error, see your point. And I changed it to functional style. Let me know what you think.
--
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
Re: [PR] Further clarification of the supports_filters_pushdown documentation [arrow-datafusion]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #9988:
URL: https://github.com/apache/arrow-datafusion/pull/9988#discussion_r1558415927
##########
datafusion/core/src/datasource/provider.rs:
##########
@@ -161,20 +161,73 @@ pub trait TableProvider: Sync + Send {
/// Specify if DataFusion should provide filter expressions to the
/// TableProvider to apply *during* the scan.
///
+ /// Some TableProviders can evaluate filters more efficiently than the
+ /// `Filter` operator in DataFusion, for example by using an index.
+ ///
/// The return value must have one element for each filter expression passed
/// in. The value of each element indicates if the TableProvider can apply
- /// that particular filter during the scan.
+ /// that particular filter during the scan. The position in the return value
+ /// Vec corresponds to the expression in the `filters` input.
+ ///
+ /// Here is an example of how this can be done:
+ ///
+ /// ```rust
+ /// use datafusion::error::{Result, DataFusionError};
+ /// use datafusion_expr::{Expr, TableProviderFilterPushDown};
+ ///
+ /// // Override the supports_filters_pushdown to evaluate which expressions
+ /// // to accept as pushdown predicates.
+ /// fn supports_filters_pushdown(&self, filters: &[&Expr]) -> Result<Vec<TableProviderFilterPushDown>> {
///
- /// Some TableProviders can evaluate filters more efficiently than the
- /// `Filter` operator in DataFusion, for example by using an index.
+ /// let mut result_vec: Vec<TableProviderFilterPushDown> = Vec::with_capacity(&filters.len());
///
- /// By default, returns [`Unsupported`] for all filters, meaning no filters
+ /// // Process each filter
+ /// for i in 0..filters.len() {
+ /// let expr = *filters[i];
+ ///
+ /// let table_provider_filter_pushdown = match expr {
+ /// // This example only supports a between expr with a single column name.
+ /// Expr::Between(between_expr) => {
+ /// between_expr.expr.try_into_col()
+ /// .map(|column| {
+ /// if column.name = "c1".to_string() {
+ /// TableProviderFilterPushDown::Exact
+ /// } else {
+ /// TableProviderFilterPushDown::Unsupported
+ /// }
+ /// })
+ /// // If there is no column in the expr set the filter to unsupported.
+ /// .unwrap_or(TableProviderFilterPushDown::Unsupported);
Review Comment:
Need to remove the `;` here so that this match arm is returning something and not returning the unit type `()`.
```suggestion
/// .unwrap_or(TableProviderFilterPushDown::Unsupported)
```
--
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
Re: [PR] Further clarification of the supports_filters_pushdown documentation [arrow-datafusion]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #9988:
URL: https://github.com/apache/arrow-datafusion/pull/9988#discussion_r1559714760
##########
datafusion/core/src/datasource/provider.rs:
##########
@@ -161,20 +161,73 @@ pub trait TableProvider: Sync + Send {
/// Specify if DataFusion should provide filter expressions to the
/// TableProvider to apply *during* the scan.
///
+ /// Some TableProviders can evaluate filters more efficiently than the
+ /// `Filter` operator in DataFusion, for example by using an index.
+ ///
/// The return value must have one element for each filter expression passed
/// in. The value of each element indicates if the TableProvider can apply
- /// that particular filter during the scan.
+ /// that particular filter during the scan. The position in the return value
+ /// Vec corresponds to the expression in the `filters` input.
+ ///
+ /// Here is an example of how this can be done:
+ ///
+ /// ```rust
+ /// use datafusion::error::{Result, DataFusionError};
+ /// use datafusion_expr::{Expr, TableProviderFilterPushDown};
+ ///
+ /// // Override the supports_filters_pushdown to evaluate which expressions
+ /// // to accept as pushdown predicates.
+ /// fn supports_filters_pushdown(&self, filters: &[&Expr]) -> Result<Vec<TableProviderFilterPushDown>> {
Review Comment:
@alamb already said this, but you can prefix lines of code with `#` to still have them compile, but not appear in the generated docs.
See https://doc.rust-lang.org/rustdoc/write-documentation/documentation-tests.html#hiding-portions-of-the-example for more info
--
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