Uh oh!
There was an error while loading. Please reload this page.
Enable parquet filter pushdown by default - #19477
Conversation
alamb
commented
Dec 24, 2025
run benchmarks |
alamb-ghbot
commented
Dec 24, 2025
🤖 |
alamb-ghbot
commented
Dec 24, 2025
🤖: Benchmark completed Details |
adriangb
commented
Dec 25, 2025
What a 🎄 🎁 ! I see mostly speedups, vastly outweighing the slowdowns, but there are still slowdowns... any idea what those cases have in common? Any way I can help brainstorm solutions? |
alamb
commented
Dec 27, 2025
Thanks @adriangb -- I am not working full time the next few days but I am hoping to work on this particular item as it is something I desparately want to be done with (and I find it fun!) I spent some time analyzing where the time is going. I haven't written up my notes yet, but I will do so (probably tomorrow as I have a long train ride). I found a few places to improve, but nothing that will obviously get back all the time (yet). I'll kep at it |
# Which issue does this PR close? - RElated to apache/datafusion#19477 # Rationale for this change While profiling apache/datafusion#19477 I noticed some additional clones we could avoid <img width="1504" height="927" alt="Screenshot 2025-12-26 at 12 03 00 PM" src="https://github.com/user-attachments/assets/958f76e2-53d0-4008-b224-d9275984cd1a" /> I doubt this will be a huge deal but it does remove some allocations int he parquet read path # What changes are included in this PR? Use `into_data` rather than `to_data` # Are these changes tested? <!-- We typically require tests for all PRs in order to: 1. Prevent the code from being accidentally broken by subsequent changes 2. Serve as another way to document the expected behavior of the code If tests are not included in your PR, please explain why (for example, are they covered by existing tests)? --> # Are there any user-facing changes? <!-- If there are user-facing changes then we may require documentation to be updated before approving the PR. If there are any breaking changes to public APIs, please call them out. -->
alamb
commented
Dec 29, 2025
I also wrote up some notes about current status and plan here: |
Dandandan
commented
Jan 3, 2026
run benchmark tpch |
alamb-ghbot
commented
Jan 3, 2026
🤖 |
Dandandan
commented
Jan 3, 2026
run benchmark tpcds |
alamb-ghbot
commented
Jan 3, 2026
🤖: Benchmark completed Details |
alamb-ghbot
commented
Jan 3, 2026
🤖 |
alamb-ghbot
commented
Jan 3, 2026
Benchmark script failed with exit code 1. Last 10 lines of output: Click to expand |
Dandandan
commented
Jan 3, 2026
This doesn't look great.... |
Dandandan
commented
Jan 3, 2026
run benchmark tpch |
alamb-ghbot
commented
Jan 3, 2026
🤖 |
alamb-ghbot
commented
Jan 3, 2026
🤖: Benchmark completed Details |
# Which issue does this PR close? <!-- We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. --> - related to apache/datafusion#19477 # Rationale for this change While working on apache/datafusion#19477, and profiling ClickBench q7, I noticed that the RowSelectors was being cloned to resolve the strategy -- for a large number of selections this is expensive and shows up in the traces <img width="1724" height="1074" alt="Screenshot 2025-12-28 at 4 49 49 PM" src="https://github.com/user-attachments/assets/72c6fd22-9377-48ef-ba80-6bc03b177cf7" /> ```shell samply record -- ./datafusion-cli-alamb_enable_pushdown -f q.sql > /dev/null 2>& ``` We should change the code to avoid cloning the RowSelectors when resolving the strategy. # Changes Don't clone / allocate while resolving the strategy. I don't expect this to have a massive impact, but it did show up in the profile FYI @hhhizzz -- perhaps you could review this PR # Are these changes tested? Yes by CI # Are there any user-facing changes? small performance improvement
# Which issue does this PR close? - related to apache/datafusion#19477 # Rationale for this change I am tracking down allocations in various parts of the parquet reader (to remove them) and I ran across this in the cached reader. # What changes are included in this PR? Use `Arc::clone` to make it clear there is no deep cloning going on . I don't expect this will have any impact on actual performance # Are these changes tested? By CI # Are there any user-facing changes? <!-- If there are user-facing changes then we may require documentation to be updated before approving the PR. If there are any breaking changes to public APIs, please call them out. -->
alamb-ghbot
commented
Feb 3, 2026
🤖: Benchmark completed Details |
alamb-ghbot
commented
Feb 3, 2026
🤖 |
alamb-ghbot
commented
Feb 3, 2026
🤖: Benchmark completed Details |
Changed default value of enable_aggregate_dynamic_filter_pushdown from false to true.
Dandandan
commented
Feb 3, 2026
run benchmark tpch |
alamb-ghbot
commented
Feb 3, 2026
🤖 |
Dandandan
commented
Feb 3, 2026
run benchmark tpch |
alamb-ghbot
commented
Feb 3, 2026
🤖: Benchmark completed Details |
alamb-ghbot
commented
Feb 3, 2026
🤖 |
alamb-ghbot
commented
Feb 3, 2026
🤖: Benchmark completed Details |
Dandandan
commented
Feb 3, 2026
Run benchmarks |
alamb-ghbot
commented
Feb 3, 2026
🤖 |
alamb-ghbot
commented
Feb 3, 2026
🤖: Benchmark completed Details |
alamb
commented
Feb 12, 2026
(note there is no longer any regression with the tpch benchmarks 🎉 ) |
| /// When set to true, the optimizer will attempt to push down Join dynamic filters | ||
| /// into the file scan phase. | ||
| pub enable_join_dynamic_filter_pushdown: bool, default = true | ||
| pub enable_join_dynamic_filter_pushdown: bool, default = false |
There was a problem hiding this comment.
I say ship it! We can work on re-enabling this later 😄
Dandandan
commented
Feb 12, 2026
Huh, there still seems to be, no? :
|
alamb
commented
Feb 12, 2026
🤔 there wans't on some previous run 🤔 |
alamb
commented
Feb 12, 2026
run benchmark tpch_mem |
alamb-ghbot
commented
Feb 12, 2026
🤖 |
alamb-ghbot
commented
Feb 12, 2026
🤖: Benchmark completed Details |
Dandandan
commented
Feb 13, 2026
run benchmark tpch |
alamb-ghbot
commented
Feb 13, 2026
🤖 |
tpch_mem doesn't have filter pushdown ;) |
alamb-ghbot
commented
Feb 13, 2026
🤖: Benchmark completed Details |
Thank you for your contribution. Unfortunately, this pull request is stale because it has been open 60 days with no activity. Please remove the stale label or comment or this will be closed in 7 days. |
Which issue does this PR close?
filter_pushdown) by default #3463Rationale for this change
I am testing where we currently are with performance
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?