apache / apache/datafusion

supports_filters_pushdown is invoked more than once on a single Custom Data Source

Open
#13,994 9 comments 0 reactions 0 assignees View on GitHub
bug
Dominant language
Rust
Stars
9.3k
Forks
2.4k
Avg merge
3d 7h
Merged PRs (30d)
344

Description

### Describe the bug

The `supports_filters_pushdown` fn is invoked more than once on the same Custom Data Source for a query. Further, it is called with different `filters` list, so it is not consistent. This occurs when the Custom Data Source has a query where multiple `filters` could qualify for pushdown, but only one is desired.

I simple query example is:

```sql
SELECT a.c1
FROM a
WHERE a.c2 > 5
AND a.c3 = 'test_string'
```

Of these 2 `filter`s the intent is to pick the better `filter` (and only one), which in this case would be `a.c3 = 'test_string'` as it would use a better index in the Custom Data Source.

Sometimes the `supports_filters_pushdown` is sending both `filters`, then it is called again with just a single `filter`, and it can be the wrong choice as the `supports_filters_pushdown` fn does not have both `filters` to choose from the second time. I have seen it vary unpredictably as to when it sends both `filter`s and when it sends just one (in other words the order is inconsistent).

Then the `scan` fn is always called only once (correct) but it can be called with the wrong filter.

### To Reproduce

To fully reproduce this I would need to create an example Custom Data Source. If it cannot be figured out otherwise I'll invest the time and do that. But in my case it will be a fair amount of work.

### Expected behavior

The `supports_filters_pushdown` should be called exactly once (I believe that is correct), and it should provide all of the available `filter`s for the Custom Data Source table. Then `scan` should faithfully provide the `filter` selected and that one only based on the result `vec` of the `TableProviderFilterPushDown` values that were returned in `supports_filters_pushdown`. Otherwise a Custom Data Source cannot control pushdown `filter`s, a key benefit of DataFusion.

### Additional context

I do not see how my Custom Data Source code could cause this, since the `supports_filters_pushdown` fn is invoked by DataFusion, not by user code.

I was able to trigger a `panic` on the second call of `supports_filters_pushdown`, here is the stacktrace. Hopefully this can shed light on this, to see if it is indeed expected behavior or a bug.

Here is a stack trace I generated from when the supports_filters_pushdown fn was called a second time on the same data source. This query has 2 filters on the same table, the first call sent 1 filter, the second sent both filters. Other times I have seen these reversed, so I suspect a threading issue. Let me know what you think and if I should file a bug with just the stacktrace.

This is v42, you just released v44, if you want I can try that version.

This definitely would appear to be a bug, you can see it is all DataFusion code that is in the trace.

```
supports_filters_pushdown was called twice!
0: rust_begin_unwind
at /rustc/25ef9e3d85d934b27d9dada2f9dd52b1dc63bb04/library/std/src/panicking.rs:647:5
1: core::panicking::panic_fmt
at /rustc/25ef9e3d85d934b27d9dada2f9dd52b1dc63bb04/library/core/src/panicking.rs:72:14
2: ::supports_filters_pushdown
at ./target/debug/build/rt-query-098625d83c072beb/out/fbs_datasource_generated.rs:3438:13
3: ::supports_filters_pushdown
at /Users/coryisaacson/git/rtip-datafusion/datafusion/core/src/datasource/default_table_source.rs:70:9
4: ::rewrite
at /Users/coryisaacson/git/rtip-datafusion/datafusion/optimizer/src/push_down_filter.rs:876:31
5: datafusion_optimizer::optimizer::optimize_plan_node
at /Users/coryisaacson/git/rtip-datafusion/datafusion/optimizer/src/optimizer.rs:338:16
6: ::f_down
at /Users/coryisaacson/git/rtip-datafusion/datafusion/optimizer/src/optimizer.rs:316:13
7: datafusion_common::tree_node::TreeNode::rewrite
at /Users/coryisaacson/git/rtip-datafusion/datafusion/common/src/tree_node.rs:179:37
8: datafusion_common::tree_node::TreeNode::rewrite::{{closure}}::{{closure}}
at /Users/coryisaacson/git/rtip-datafusion/datafusion/common/src/tree_node.rs:179:64
9: datafusion_expr::logical_plan::tree_node::rewrite_arc
at /Users/coryisaacson/git/rtip-datafusion/datafusion/expr/src/logical_plan/tree_node.rs:387:5
10: datafusion_expr::logical_plan::tree_node::::map_children
at /Users/coryisaacson/git/rtip-datafusion/datafusion/expr/src/logical_plan/tree_node.rs:83:19
11: datafusion_common::tree_node::TreeNode::rewrite::{{closure}}
at /Users/coryisaacson/git/rtip-datafusion/datafusion/common/src/tree_node.rs:28:37
12: datafusion_common::tree_node::Transformed::transform_children
at /Users/coryisaacson/git/rtip-datafusion/datafusion/common/src/tree_node.rs:735:24
13: datafusion_common::tree_node::TreeNode::rewrite
at /Users/coryisaacson/git/rtip-datafusion/datafusion/common/src/tree_node.rs:179:9
14: datafusion_optimizer::optimizer::Optimizer::optimize
at /Users/coryisaacson/git/rtip-datafusion/datafusion/optimizer/src/optimizer.rs:388:42
15: datafusion::execution::session_state::SessionState::optimize
at /Users/coryisaacson/git/rtip-datafusion/datafusion/core/src/execution/session_state.rs:716:13
16: datafusion::execution::session_state::SessionState::create_physical_plan::{{closure}}
at /Users/coryisaacson/git/rtip-datafusion/datafusion/core/src/execution/session_state.rs:734:28
17: datafusion::dataframe::DataFrame::create_physical_plan::{{closure}}
at /Users/coryisaacson/git/rtip-datafusion/datafusion/core/src/dataframe/mod.rs:215:61
18: rt_query_common::query_control::QueryControl::create::{{closure}}
at ./rt-query-common/src/query_control.rs:159:55
19: ::get_flight_info_statement::{{closure}}
at ./rt-ng-db-service/src/ng_db_service_impl.rs:357:61
20: as core::future::future::Future>::poll
at /rustc/25ef9e3d85d934b27d9dada2f9dd52b1dc63bb04/library/core/src/future/future.rs:124:9
21: arrow_flight::sql::server::::get_flight_info::{{closure}}
at /Users/coryisaacson/.cargo/registry/src/index.crates.io-6f17d22bba15001f/arrow-flight-53.0.0/src/sql/server.rs:610:64
22: as core::future::future::Future>::poll
at /rustc/25ef9e3d85d934b27d9dada2f9dd52b1dc63bb04/library/core/src/future/future.rs:124:9
23: < as tower_service::Service>>::call::GetFlightInfoSvc as tonic::server::service::UnaryService>::call::{{closure}}
at /Users/coryisaacson/.cargo/registry/src/index.crates.io-6f17d22bba15001f/arrow-flight-53.0.0/src/arrow.flight.protocol.rs:1242:88
24: as core::future::future::Future>::poll
at /rustc/25ef9e3d85d934b27d9dada2f9dd52b1dc63bb04/library/core/src/future/future.rs:124:9
25: tonic::server::grpc::Grpc::unary::{{closure}}
at /Users/coryisaacson/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tonic-0.12.2/src/server/grpc.rs:251:14
26: as tower_service::Service>>::call::{{closure}}
at /Users/coryisaacson/.cargo/registry/src/index.crates.io-6f17d22bba15001f/arrow-flight-53.0.0/src/arrow.flight.protocol.rs:1264:59
27: as core::future::future::Future>::poll
at /rustc/25ef9e3d85d934b27d9dada2f9dd52b1dc63bb04/library/core/src/future/future.rs:124:9
28: ::try_poll
at /Users/coryisaacson/.cargo/registry/src/index.crates.io-6f17d22bba15001f/futures-core-0.3.30/src/future.rs:82:9
29: as core::future::future::Future>::poll
at /Users/coryisaacson/.cargo/registry/src/index.crates.io-6f17d22bba15001f/futures-util-0.3.30/src/future/try_future/into_future.rs:34:9
30: as core::future::future::Future>::poll
at /Users/coryisaacson/.cargo/registry/src/index.crates.io-6f17d22bba15001f/futures-util-0.3.30/src/future/future/map.rs:55:37
31: as core::future::future::Future>::poll
at /Users/coryisaacson/.cargo/registry/src/index.crates.io-6f17d22bba15001f/futures-util-0.3.30/src/lib.rs:91:13
32: as core::future::future::Future>::poll
at /Users/coryisaacson/.cargo/registry/src/index.crates.io-6f17d22bba15001f/futures-util-0.3.30/src/lib.rs:91:13
33: as core::future::future::Future>::poll
at /Users/coryisaacson/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tower-0.4.13/src/macros.rs:38:17
34: as core::future::future::Future>::poll
at /rustc/25ef9e3d85d934b27d9dada2f9dd52b1dc63bb04/library/core/src/future/future.rs:124:9
35: as core::future::future::Future>::poll
at /Users/coryisaacson/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tower-0.4.13/src/util/oneshot.rs:97:38
36: as core::future::future::Future>::poll
at /Users/coryisaacson/.cargo/registry/src/index.crates.io-6f17d22bba15001f/axum-0.7.5/src/routing/route.rs:162:61
37: ::poll
at /Users/coryisaacson/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tonic-0.12.2/src/service/router.rs:163:22
38: as core::future::future::Future>::poll
at /Users/coryisaacson/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tonic-0.12.2/src/transport/service/grpc_timeout.rs:83:38
39: as core::future::future::Future>::poll
at /Users/coryisaacson/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tower-0.4.13/src/util/either.rs:71:57
40: as core::future::future::Future>::poll
at /Users/coryisaacson/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tonic-0.12.2/src/transport/server/service/recover_error.rs:60:20
41: as core::future::future::Future>::poll
at /Users/coryisaacson/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tonic-0.12.2/src/transport/server/mod.rs:942:50
42: as core::future::future::Future>::poll
at /rustc/25ef9e3d85d934b27d9dada2f9dd52b1dc63bb04/library/core/src/future/future.rs:124:9
43: as core::future::future::Future>::poll
at /Users/coryisaacson/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tower-0.4.13/src/util/oneshot.rs:97:38
44: as core::future::future::Future>::poll
at /Users/coryisaacson/.cargo/registry/src/index.crates.io-6f17d22bba15001f/hyper-util-0.1.6/src/service.rs:60:9
45: hyper::proto::h2::server::H2Stream::poll2
at /Users/coryisaacson/.cargo/registry/src/index.crates.io-6f17d22bba15001f/hyper-1.4.1/src/proto/h2/server.rs:447:37
46: as core::future::future::Future>::poll
at /Users/coryisaacson/.cargo/registry/src/index.crates.io-6f17d22bba15001f/hyper-1.4.1/src/proto/h2/server.rs:537:9
47: tokio::runtime::task::core::Core::poll::{{closure}}
at /Users/coryisaacson/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.39.2/src/runtime/task/core.rs:328:17
48: tokio::loom::std::unsafe_cell::UnsafeCell::with_mut
at /Users/coryisaacson/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.39.2/src/loom/std/unsafe_cell.rs:16:9
49: tokio::runtime::task::core::Core::poll
at /Users/coryisaacson/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.39.2/src/runtime/task/core.rs:317:13
50: tokio::runtime::task::harness::poll_future::{{closure}}
at /Users/coryisaacson/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.39.2/src/runtime/task/harness.rs:485:19
51: as core::ops::function::FnOnce<()>>::call_once
at /rustc/25ef9e3d85d934b27d9dada2f9dd52b1dc63bb04/library/core/src/panic/unwind_safe.rs:272:9
52: std::panicking::try::do_call
at /rustc/25ef9e3d85d934b27d9dada2f9dd52b1dc63bb04/library/std/src/panicking.rs:554:40
```

Contributor guide

Open the contributing guide

Research direction

Start in datafusion/optimizer/src/push_down_filter.rs, where the stack trace shows supports_filters_pushdown is called, and trace through DefaultTableSource and the TableProvider filter-pushdown path. Reproduce the SQL case with a Custom Data Source that has two filters, then determine whether the callback receives inconsistent filter sets. Done means the selected pushdown filter is consistently passed to scan and regression coverage verifies the behavior.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust, sql
Domain
databases
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Needs clarification
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.