Skip to content

fix: expand float fuzz coverage and correct discovered inconsistencies - #24433

Merged
comphead merged 5 commits into
apache:mainfrom
Theodus:test-common-scalar
Aug 20, 2026
Merged

fix: expand float fuzz coverage and correct discovered inconsistencies#24433
comphead merged 5 commits into
apache:mainfrom
Theodus:test-common-scalar

Conversation

@Theodus

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Rationale for this change

Two floating-point edge cases produced inconsistent results:

  • ScalarValue::eq_array used IEEE equality for floats, while ScalarValue equality uses bit representations. An identical NaN therefore compared unequal, while +0.0 and -0.0 compared equal.
  • Grouped floating-point MIN and MAX used partial ordering. With NaNs or signed zeros, results could depend on input order, batching, or partitioning and disagree with the non-grouped accumulators.

Both issues were exposed by generating special floating-point values in fuzz data and adding fuzz coverage for ScalarValue array conversions.

What changes are included in this PR?

  • Generate +0.0, -0.0, positive and negative infinities, and NaNs with varied signs and payloads in random Float32 and Float64 test data.
  • Add fuzz coverage checking consistency and round trips among ScalarValue::try_from_array, eq_array, to_array, and iter_to_array across supported array types and sliced arrays.
  • Make Float16, Float32, and Float64 ScalarValue::eq_array comparisons use the same bitwise equality as ScalarValue::PartialEq.
  • Make grouped Float16, Float32, and Float64 MIN/MAX use total ordering and total-order extrema as their initial accumulator values.

Are these changes tested?

Yes. The existing scalar_eq_array test now covers NaNs and signed zeros. New deterministic grouped-accumulator tests cover positive and negative NaNs, signed zeros, and the total-order sentinel values. The scalar conversion and aggregation fuzz tests cover the same paths across randomized arrays and execution configurations.

Are there any user-facing changes?

Yes. ScalarValue::eq_array now uses bitwise float equality, and grouped floating-point MIN and MAX now return deterministic results consistent with DataFusion's existing total-order semantics. There are no API signature changes.

@Jefffrey

Copy link
Copy Markdown
Contributor

cc @comphead since you worked on something similar before

@codecov-commenter

codecov-commenter commented Aug 18, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 76.25000% with 19 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.24%. Comparing base (bb038a6) to head (567e7aa).
⚠️ Report is 2 commits behind head on main.

Files with missing linesPatch %Lines
test-utils/src/array_gen/random_data.rs0.00%14 Missing ⚠️
datafusion/common/src/scalar/mod.rs25.00%1 Missing and 2 partials ⚠️
datafusion/functions-aggregate/src/min_max.rs96.77%0 Missing and 2 partials ⚠️
Additional details and impacted files
@@ Coverage Diff @@## main #24433 +/- ##
========================================
Coverage 81.23% 81.24% ========================================
Files 1112 1112 Lines 390635 390982 +347 Branches 390635 390982 +347 ========================================
+ Hits 317350 317650 +300 - Misses 54650 54687 +37 - Partials 18635 18645 +10 

☔ 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.

@jayzhan211jayzhan211 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.

LGTM Thanks @Theodus

@comphead

Copy link
Copy Markdown
Contributor

sorry missed that, checking it rn

@compheadcomphead 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.

Thanks @Theodus the PR is pretty consistent

@comphead
comphead added this pull request to the merge queueAug 20, 2026
@github-merge-queue
github-merge-queueBot removed this pull request from the merge queue due to no response for status checks Aug 20, 2026
@comphead
comphead added this pull request to the merge queueAug 20, 2026
Merged via the queue into apache:main with commit 15eaa9fAug 20, 2026
37 checks passed
@Theodus
Theodus deleted the test-common-scalar branch August 20, 2026 19:41
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

commonRelated to common cratecoreCore DataFusion cratefunctionsChanges to functions implementation

Projects

None yet

5 participants

@Theodus@Jefffrey@codecov-commenter@comphead@jayzhan211