Skip to content

fix(common): preserve an exact zero through filter selectivity estimation - #23936

Open
asolimando wants to merge 1 commit into
apache:mainfrom
asolimando:asolimando/filter-preserve-exact-empty
Open

fix(common): preserve an exact zero through filter selectivity estimation#23936
asolimando wants to merge 1 commit into
apache:mainfrom
asolimando:asolimando/filter-preserve-exact-empty

Conversation

@asolimando

@asolimando asolimando commented Jul 28, 2026

Copy link
Copy Markdown
Member

Which issue does this PR close?

Rationale for this change

Precision::with_estimated_selectivity always demoted to inexact, so Exact(0) became Inexact(0). Scaling zero by any selectivity is still exactly zero: filtering a provably empty input cannot produce rows.

Because operators widen exact to inexact, emptiness could not propagate up a plan through statistics at all, it was lost at the first FilterExec. That operator already preserved the exact zero for a contradictory predicate and for column byte sizes, so the same fact came out exact or inexact depending on the path taken.

This is the same reasoning as #23670, which stops FileScanConfig::statistics() demoting an exact zero when a filter is present, applied one level up. The two are independent, but together they let a proof of emptiness survive from the scan through the filter.

What changes are included in this PR?

  • with_estimated_selectivity returns an exact zero unchanged. Inexact(0) is untouched: an estimate of zero is not a proof of zero.
  • scale_byte_size in filter.rs is removed, subsumed by the above.

Are these changes tested?

Yes. A unit test for the method, and a FilterExec test that a satisfiable predicate over an exactly empty input keeps num_rows, total_byte_size (proving that scale_byte_size can be safely removed) and column byte_size exact.

test_filter_statistics_empty_input_equality_ndv_zero asserted Inexact(0) and now asserts Exact(0), which is expected.

Are there any user-facing changes?

No breaking changes. Precision::with_estimated_selectivity is public and keeps its signature, but its documented contract changes: it previously stated it would always return inexact statistics, and it now preserves an exact zero. Callers relying on the old wording will see Exact(0) where they saw Inexact(0). This is an improvement and could be arguably considered a bug-fix, so I consider this contract change justified.

FilterExec over a provably empty input therefore reports exact statistics, visible in EXPLAIN output that shows statistics.


Disclaimer: I used AI to assist in the code generation, I have manually reviewed the output and it matches my intention and understanding.

…tion

`Precision::with_estimated_selectivity` documented itself as always returning
inexact statistics, so `Exact(0)` became `Inexact(0)`. Scaling zero by any
selectivity is still exactly zero: filtering an input that provably has no rows
cannot produce rows, whatever the predicate is.

The consequence is not just a weaker estimate. Because operators widen exact to
inexact as statistics flow up a plan, emptiness could not be propagated upwards
through statistics at all, it was lost at the first `FilterExec`. `FilterExec`
already special-cased this for a contradictory predicate, which returns
`Exact(0)` directly, and for column byte sizes via a local `scale_byte_size`
helper, so the two disagreed about the same fact depending on the path taken.

Preserve `Exact(0)` in `with_estimated_selectivity` itself, which is only called
from `FilterExec`, and drop `scale_byte_size` now that it is subsumed.

`Inexact(0)` is untouched: an estimate of zero is not a proof of zero.
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 96.15385% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.68%. Comparing base (9cad861) to head (79a6ad7).
⚠️ Report is 6 commits behind head on main.

Files with missing lines Patch % Lines
datafusion/physical-plan/src/filter.rs 93.93% 0 Missing and 2 partials ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##             main   #23936    +/-   ##
========================================
  Coverage   80.67%   80.68%            
========================================
  Files        1095     1095            
  Lines      372460   372576   +116     
  Branches   372460   372576   +116     
========================================
+ Hits       300488   300615   +127     
+ Misses      54045    54015    -30     
- Partials    17927    17946    +19     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@tschwarzinger tschwarzinger left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good to me. Thanks for thinking this change further!

@asolimando

Copy link
Copy Markdown
Member Author

cc:

@kosiew kosiew left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@asolimando

Good fix.

Left a small comment suggestion.

/// Caps a row-bounded column statistic (a null count or distinct count) at the
/// filtered row estimate, since a column cannot have more nulls or distinct
/// values than it has rows. Known counts are demoted to inexact because the
/// filtered row count is itself an estimate.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could we update this comment slightly? It currently says the filtered row count is always an estimate, but this PR now allows filtered_num_rows to be Exact(0). Maybe clarify that counts are normally demoted because filter-derived row bounds are estimates, except when the row bound proves there are zero rows.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

common Related to common crate physical-plan Changes to the physical-plan crate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants