Skip to content

fix: make skip_partial_aggregation_probe_ratio_threshold match the docs#22752

Open
haohuaijin wants to merge 5 commits into
apache:mainfrom
haohuaijin:chore-skip-agg
Open

fix: make skip_partial_aggregation_probe_ratio_threshold match the docs#22752
haohuaijin wants to merge 5 commits into
apache:mainfrom
haohuaijin:chore-skip-agg

Conversation

@haohuaijin
Copy link
Copy Markdown
Contributor

Which issue does this PR close?

  • Closes #.

Rationale for this change

The config skip_partial_aggregation_probe_ratio_threshold was documented as triggering skip when the ratio is greater than the threshold, but the code used >=. This meant setting the threshold to 1.0 (to disable the feature) still skipped rows when cardinality was exactly 100%.

What changes are included in this PR?

  • Changed >= to > in the ratio comparison to match the docs.
  • Return None for SkipAggregationProbe when probe_ratio_threshold >= 1.0, effectively disabling the feature since the ratio can never exceed 1.0.

Are these changes tested?

Yes. Added test_skip_aggregation_disabled_at_threshold_one which sets threshold to 1.0 with 100% cardinality input and asserts that no rows are skipped.

Are there any user-facing changes?

Yes. Setting skip_partial_aggregation_probe_ratio_threshold = 1.0 now reliably disables skip aggregation, matching the documented behavior.

@haohuaijin haohuaijin changed the title Chore skip agg fix: make skip_partial_aggregation_probe_ratio_threshold match the docs Jun 4, 2026
Copy link
Copy Markdown
Contributor

@kosiew kosiew left a comment

Choose a reason for hiding this comment

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

@haohuaijin
Thanks for the fix. I do not see any blocking issues, just a couple of small suggestions that could make the regression coverage and config setup a bit clearer.

Comment thread datafusion/physical-plan/src/aggregates/row_hash.rs
Comment thread datafusion/physical-plan/src/aggregates/mod.rs Outdated
@github-actions github-actions Bot added the physical-plan Changes to the physical-plan crate label Jun 5, 2026
@haohuaijin
Copy link
Copy Markdown
Contributor Author

Thanks for you reviews @kosiew , apply suggestion in 0853632

@haohuaijin
Copy link
Copy Markdown
Contributor Author

the failed should not related to my change, let me retrigger the ci to see.

@haohuaijin haohuaijin closed this Jun 5, 2026
@haohuaijin haohuaijin reopened this Jun 5, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

physical-plan Changes to the physical-plan crate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants