Skip to content

Fix parquet filter_pushdown: respect parquet filter pushdown config in scan #16646

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
Jul 2, 2025
Merged

Fix parquet filter_pushdown: respect parquet filter pushdown config in scan #16646

merged 2 commits into from
Jul 2, 2025

Conversation

adriangb
Copy link
Contributor

@adriangb adriangb commented Jul 1, 2025 •
edited by alamb
Loading

@github-actions github-actions bot added the datasource Changes to the datasource crate label Jul 1, 2025
@adriangb
Copy link
Contributor Author

adriangb commented Jul 1, 2025

I'm trying to understand why this isn't caught by datafusion/sqllogictest/test_files/parquet_filter_pushdown.slt. Might have to wait until tomorrow.

@github-actions github-actions bot added the sqllogictest SQL Logic Tests (.slt) label Jul 1, 2025
@adriangb
Copy link
Contributor Author

adriangb commented Jul 1, 2025

I can confirm this new test fails without the change:

❯ cargo test -p datafusion-sqllogictest --test sqllogictests -- parquet_filter_pushdown
   Compiling datafusion-datasource-parquet v48.0.0 (/Users/adriangb/GitHub/datafusion-clone/datafusion/datasource-parquet)
   Compiling datafusion v48.0.0 (/Users/adriangb/GitHub/datafusion-clone/datafusion/core)
   Compiling datafusion-substrait v48.0.0 (/Users/adriangb/GitHub/datafusion-clone/datafusion/substrait)
   Compiling datafusion-sqllogictest v48.0.0 (/Users/adriangb/GitHub/datafusion-clone/datafusion/sqllogictest)
    Finished `test` profile [unoptimized + debuginfo] target(s) in 4.69s
     Running bin/sqllogictests.rs (target/debug/deps/sqllogictests-5b94bf3a17202ad3)
Completed 1 test files in 0 seconds                                                                                                                                                                                                                                      External error: 1 errors in file /Users/adriangb/GitHub/datafusion-clone/datafusion/sqllogictest/test_files/parquet_filter_pushdown.slt

1. query result mismatch:
[SQL] select a from t where b > 2 ORDER BY a;
[Diff] (-expected|+actual)
+   bar
    baz
    foo
+   foo
    NULL
    NULL
    NULL
at /Users/adriangb/GitHub/datafusion-clone/datafusion/sqllogictest/test_files/parquet_filter_pushdown.slt:120



Error: Execution("1 failures")
error: test failed, to rerun pass `-p datafusion-sqllogictest --test sqllogictests`

@adriangb adriangb mentioned this pull request Jul 1, 2025
Closed
alamb
alamb reviewed Jul 1, 2025
View reviewed changes
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @adriangb 🙏

I don't understand how this fixes wrong results. I am out of time today but will check on this first thing tomorrow

datafusion/datasource-parquet/src/source.rs
/// Defaults to false
///
/// [`Expr`]: datafusion_expr::Expr
/// Defaults to false.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How did this result in wrong results?

Is it because the planner assumed filters would be pushed down but they weren't?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well this bit is just a comment 😛

But this produced wrong results because we'd return Supported(filter) up to the parents -> FilterExec removed from the plan but ParquetOpener.pushdown_filters = false -> pruning did not actually happen during the scan.

adriangb
adriangb commented Jul 1, 2025
View reviewed changes
datafusion/datasource-parquet/src/source.rs
None => conjunction(allowed_filters.iter().cloned()),
};
source.predicate = Some(predicate);
source = source.with_pushdown_filters(pushdown_filters);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alamb this is the relevant LOC that fixes things

ianthetechie
ianthetechie approved these changes Jul 2, 2025
View reviewed changes
Copy link
Contributor

@ianthetechie ianthetechie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can confirm as well that it works when pointing at this branch using my original code. Thanks!

datafusion/datasource-parquet/src/source.rs
// If both are disabled, we will not push down the filters.
// By default they are both disabled.
// Regardless of pushdown, we will update the predicate to include the filters
// because even if scan pushdown is disabled we can still use the filters for stats pruning.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had wondered about this; thanks for updating this comment!

alamb
alamb approved these changes Jul 2, 2025
View reviewed changes
Copy link
Contributor

@alamb alamb left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @adriangb and @ianthetechie

With his and several other issues, I think we should make a 48.0.1 release:

I will begin work on that later today

@alamb alamb changed the title respect parquet filter pushdown config in scan Fix parquet filter_pushdown: respect parquet filter pushdown config in scan Jul 2, 2025
@alamb alamb mentioned this pull request Jul 2, 2025
Closed
@alamb alamb merged commit f03a8fd into apache:main Jul 2, 2025
29 of 30 checks passed
@alamb
Copy link
Contributor

alamb commented Jul 2, 2025

Thank you @adriangb -- I plan to backport this to the branch-48and include it in a patch release

alamb pushed a commit to alamb/datafusion that referenced this pull request Jul 2, 2025
@adriangb @alamb
Fix parquet filter_pushdown: respect parquet filter pushdown config i…
cad0820 
…n scan (apache#16646)

* respect parquet filter pushdown config in scan

* Add test
@alamb alamb mentioned this pull request Jul 2, 2025
Merged
@alamb
Copy link
Contributor

alamb commented Jul 2, 2025

Backport PR:

alamb added a commit that referenced this pull request Jul 3, 2025
@alamb @adriangb
Fix parquet filter_pushdown: respect parquet filter pushdown config i…
7b31676 
…n scan (#16646) (#16656)

* respect parquet filter pushdown config in scan

* Add test

Co-authored-by: Adrian Garcia Badaracco <1755071+adriangb@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alamb alamb alamb approved these changes

+1 more reviewer

@ianthetechie ianthetechie ianthetechie approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
datasource Changes to the datasource crate sqllogictest SQL Logic Tests (.slt)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Wrong results when pushdown_filters is enabled (starting in 48.0.0)
3 participants
@adriangb @alamb @ianthetechie