supports_filter_pushdown() receives broken-up filters

[avantgardnerio]

Is your feature request related to a problem or challenge? Please describe what you are trying to do.

I have a TableProvider that supports multi-column indexes and could theoretically support filter pushdown, however when running the query:

SELECT s_quantity, s_data, s_dist_01, s_dist_02, s_dist_03, s_dist_04, s_dist_05, s_dist_06, s_dist_07, s_dist_08, s_dist_09, s_dist_10 FROM stock WHERE s_i_id = $1 AND s_w_id = $2 FOR UPDATE

on the table

create table stock (
s_i_id int not null, 
s_w_id smallint not null, 
...
s_data varchar(50),
PRIMARY KEY(s_w_id, s_i_id) ) ;

I observe the following in my logs:

ArrowTableProvider::supports_filter_pushdown() proj=stock.s_i_id = Int32(3604)
ArrowTableProvider::supports_filter_pushdown() proj=stock.s_w_id = Int32(1)
ArrowTableProvider::supports_filter_pushdown() proj=stock.s_i_id = Int32(3604)
ArrowTableProvider::supports_filter_pushdown() proj=stock.s_w_id = Int32(1)

    fn supports_filter_pushdown(&self, filter: &Expr) -> Result<TableProviderFilterPushDown, DataFusionError> {
        // TODO: parse expression and return "true" if it's a simple range on an index we have
        println!("ArrowTableProvider::supports_filter_pushdown() filter={filter:?}");
        Ok(TableProviderFilterPushDown::Unsupported)
    }

So I would have to do complicated logic (append each portion of the predicate, always return "true", then "and" them back together during scan()) to properly apply this filter push down in my code.

Describe the solution you'd like

DataFusion would provide raw Exprs of the entire filter it is trying to push down, or - as a compromise, try first with the whole filter, then if the TableProvider returns Unsupported try again with each part as suggested by @andygrove .

Describe alternatives you've considered

• Try to reconstruct the original filter in my TableProvider asynchronously
• Give up and don't use indexes

[Dandandan]

Just for my understanding, what kind of TableProvider are you implementing?
If it's a relational database, I guess it would be best to just AND the expressions together and push the full filter down to the database and let it find out the index by itself? What would be the reason to push down certain parts?

[avantgardnerio]

what kind of TableProvider are you implementing? .. If it's a relational database

yes, it is

push the full filter down to the database

Replace "database" with TableProvider and I agree with you there. (We're writing the database, not calling out to an external one)

What would be the reason to push down certain parts?

Only that this is how it happens now. I assume others are attached to the present behavior? It seems way more simple to have the TableProvider split the conjunction (if it wants) than have it re-assemble the original filter from a bunch of separate calls.

[avantgardnerio]

Honestly, I think the fix might just be removing this line: https://github.com/apache/arrow-datafusion/blob/2a5ef432aef45793b60c2847552d6d597589d67d/datafusion/optimizer/src/push_down_filter.rs#L706

@Dandandan @andygrove , thoughts?

[Dandandan]

I think the idea of splitting the filter is that a tableprovider might support certain expressions/functions but not others. This way it can be communicated, so filter push down can push down only certain sub-expressions. If you would pass the complete expression, it only could communicate this for the entire filter.

[avantgardnerio]

@Dandandan what do you think about Andy's suggestion to:

1. try with the whole filter - the TableProvider is free to say "no"
2. if "no" split the conjunction and try with each part

?

[avantgardnerio]

@alamb I guess the alternative is going full-bore on what we had talked about and having TableProviders expose something like:

pub fn get_indexes(&self) -> Vec<Vec<(Column, SortOrder)>> {}

And taking care of this in the planner / optimizer.

[avantgardnerio]

An alternative approach might be something like:

    fn supports_filter_pushdown(&self, filter: &Expr) -> Result<Expr, DataFusionError> {
        // reduce the input expression down to only the parts of the expression this TableProvider supports
    }

[alamb]

@avantgardnerio I wonder if your TableProvider could return TableProviderFilterPushDown::Inexact for all the filters

• DataFusion will not rely on pushed down filters for correctness (there will still be a Filter after the scan)
• DataFusion will provide all filters to the TableProvider::scan
• The TableProvider can then figure out what, if any, indexes can be used given the list of predicates it gets.
• Any predicate that can't be used in the index is simply ignored

https://docs.rs/datafusion/18.0.0/datafusion/datasource/datasource/enum.TableProviderFilterPushDown.html#variant.Inexact

Not only does this seem (?) to handle the usecase in this ticket's description, it would also allow you to handle predicates where you only have indexes on part of the predicate. For example, instead of

 WHERE s_i_id = $1 AND s_w_id = $2

What if you had a predicate like

 WHERE s_i_id = $1 AND s_w_id = $2 AND s_i_age > 21

Or something? You still probably want to use the multi-column index on (s_i_id, s_w_id)

[avantgardnerio]

return TableProviderFilterPushDown::Inexact for all the filters

I think this would work, but not perform optimally - the TableProvider would actually match exactly, but DataFusion would still have a (useless) Filter in the plan.

I think I have a solution that I believe would work for all the cases. Simply change the existing signature of:

fn supports_filter_pushdown( &self, filters: &Expr) -> Result<TableProviderFilterPushDown>

to:

fn supports_filter_pushdown( &self, filters: &[Expr]) -> Result<Vec<TableProviderFilterPushDown>>

Then my TableProvider could see both s_i_id and s_w_id at the same time, find the appropriate index, and return Exact for both of those but NotSupported for s_i_age.

It's barely a change from the existing signature, and we can even wrap it in an adapter method with a default impl that will keep existing TableProviders working by calling it iteratively, mark the old one @Deprecated and gracefully change it out over the next few releases.

Thoughts @alamb ?

[Dandandan]

I like that latest suggestion

[avantgardnerio]

I like that latest suggestion

done: #5367

[andygrove]

The API change seems reasonable to me too