Skip to content

fix(common): preserve min/max when merging all-null statistics - #26993

Closed
Subodh-17 wants to merge 1 commit into
apache:mainfrom
Subodh-17:fix/stats-merge-all-null-min-max
Closed

fix(common): preserve min/max when merging all-null statistics#26993
Subodh-17 wants to merge 1 commit into
apache:mainfrom
Subodh-17:fix/stats-merge-all-null-min-max

Conversation

@Subodh-17

@Subodh-17 Subodh-17 commented Nov 1, 2026

Copy link
Copy Markdown

Important

Fork CI setup: For pull requests from a fork to main, enable workflows in your fork's Actions tab before you push.
See the fork CI setup guide.

Which issue does this PR close?

Closes #26748

Rationale for this change

When merging partition statistics via Statistics::try_merge_iter_with_ndv_fallback (for example, across multiple parquet files or parallel branches of a UnionExec), an all-null partition (num_rows == null_count) or empty partition (num_rows == 0) has no non-null values that can define a minimum or maximum (Precision::Absent).

Under existing logic, merging Precision::Absent from an all-null partition with known exact bounds (Precision::Exact(min), Precision::Exact(max)) causes precision_min and precision_max to poison the merged result to Precision::Absent. In relational semantics, MIN and MAX aggregates ignore nulls. Erasing valid bounds when an empty or all-null partition is encountered causes downstream query optimizers to lose column range pruning opportunities.

What changes are included in this PR?

  1. Helper Function is_known_all_null: Detects when a partition is provably empty (num_rows == Exact(0)) or provably all-null (num_rows == Exact(N) and null_count == Exact(N)). Inexact estimates or absent row/null counts are not treated as all-null.
  2. Order-Independent Bound Initialization:
    • In Statistics::try_merge_iter_with_ndv_fallback, tracks has_non_null_rows: Vec<bool> across columns.
    • If the first partition is all-null or empty, resets its min/max to Precision::Absent and marks has_non_null_rows false for that column.
    • The first partition with non-null rows initializes min_value and max_value.
    • Subsequent all-null/empty partitions bypass the min/max update, preserving established bounds regardless of partition ordering.
  3. Sound Conservatism: If null_count is unknown (Precision::Absent) while num_rows > 0, non-null values cannot be disproven, so standard conservative degradation to Precision::Absent is preserved.

What is the testing strategy for this PR?

Added comprehensive unit test test_try_merge_iter_ignores_all_null_and_empty_inputs_for_min_max in datafusion/common/src/stats.rs covering:

  • Non-null partition followed by all-null partition
  • All-null partition followed by non-null partition
  • Empty partition combined with non-null partition (both orderings)
  • Multi-partition sequence with interleaved all-null partitions
  • Unknown null count with absent min/max (verified conservative poisoning to Absent)
  • All-null-only inputs (verified bounds remain Absent)
  • Empty-only inputs (verified bounds remain Absent)
  • Exact-zero value preservation (integers and floating point 0.0/-0.0)

Validation results:

  • cargo fmt --check -p datafusion-common: PASS (clean formatting)
  • cargo test -p datafusion-common --lib stats: PASS (78 passed; 0 failed)
  • cargo clippy -p datafusion-common -- -D warnings: PASS (0 warnings)
  • cargo test -p datafusion-physical-plan --lib union: PASS (28 passed; 0 failed)

Are there any user-facing changes?

No breaking changes to public APIs. Fixes a statistics degradation bug where table min/max statistics were unnecessarily collapsed to Precision::Absent.

@Subodh-17
fix(common): preserve min/max when merging all-null statistics
bd7c444 
Closes apache#26748

When merging partition statistics in Statistics::try_merge_iter_with_ndv_fallback, inputs that are provably all-null (num_rows == null_count) or empty (num_rows == 0) have Precision::Absent for min_value and max_value. Merging these with existing exact bounds previously degraded the overall table or partition min/max to Precision::Absent.

This change:

1. Ignores min/max updates from provably all-null or empty partition inputs.

2. Preserves known bounds established by non-null partitions regardless of partition ordering.

3. Preserves conservative degradation to Precision::Absent if null counts are absent when rows > 0.

4. Adds regression tests covering ordering invariance, empty partitions, multi-partitions, unknown null counts, and all-null-only inputs.
@github-actions github-actions Bot added the common Related to common crate label Nov 1, 2026
asolimando
asolimando approved these changes Nov 2, 2026
View reviewed changes

@asolimando asolimando left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM on min/max, skipping all-null inputs and preserving values from the first real partition makes sense, and you also keep the demotion to Absent when counts are unknown. Tests make sense too.

I noticed that distinct_count suffers from the same problem, can you file a follow-up issue for that?

@alamb

alamb commented Nov 2, 2026

Copy link
Copy Markdown
Contributor

Thank you for the contribution. The change may well be correct, but I am closing this PR under our AI policy.

Maintainer review time is the scarcest resource in this project. This account has opened more than 70 PRs across many repositories in the last month, which looks like GitHub "merge-maxxing" (collecting merged PRs) rather than work on a project the author uses. That suggests the changes aren't coming from someone who uses DataFusion, and that no human will be around to discuss review feedback and maintain the code. Reviewing PRs like this costs us more time than it saves.

If you do use DataFusion and this fix matters to you, please pick the one change you care most about. Then open a focused PR with a short description in your own words, one you have reviewed and understand, and we'd be happy to help with review.

@alamb alamb closed this Nov 2, 2026
@alamb

alamb commented Nov 2, 2026

Copy link
Copy Markdown
Contributor

Sorry that you have already reviewd this @asolimando

@txwyy123 txwyy123 mentioned this pull request Nov 13, 2026
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
@asolimando asolimando asolimando approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

common Related to common crate

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Skip all-null inputs when merging min/max in Statistics::try_merge_iter

3 participants

@Subodh-17 @alamb @asolimando