Skip to content

fix: match Spark percentile ordering for negative NaNs - #6829

Merged
sunchao merged 2 commits into
apache:mainfrom
rich7420:fix/6519-approx-percentile-nan-order
Oct 11, 2026
Merged

sunchao merged 2 commits into
apache:mainfrom
rich7420:fix/6519-approx-percentile-nan-order

Conversation

@rich7420

@rich7420 rich7420 commented Oct 9, 2026 •

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Closes #6519.

Rationale for this change

Native percentile_approx sorts negative-sign NaNs before finite values, while Spark places all NaNs last. After repeated buffer flushes, this can produce incorrect percentiles, including a finite maximum instead of NaN.

What changes are included in this PR?

  • Move leading negative NaNs to the end after sorting, without allocating another buffer. Preserve NaN bits and signed-zero ordering.
  • Add Rust regressions for NaN signs and payloads, repeated buffer flushes, serialized summary merges, infinities and signed zero.
  • Add SQL coverage comparing native results with Spark for Float/Double inputs, scalar/array outputs and grouped aggregation. Generate negative NaNs after the Parquet scan so the test preserves the trigger.

How are these changes tested?

  • Before the latest rebase: 15 quantile-module tests passed; negative-control tests reproduced the bug with the original kernel.
  • Earlier integration validation: 169 native aggregate tests passed, plus 20 SQL queries per profile on Spark 3.5.9 and 4.1.3. The new SQL fixture failed with the original kernel.
  • Published head 237473a5ee844f33e6fc9fbcafa4a30c0bbe3d3c: upstream CI completed with 26 successful checks and 15 skipped checks, including successful Required Checks. This PR is ready for review. The local results above belong to the earlier revisions named there.
  • Spark SQL and nondefault Comet Spark profiles were skipped in the default PR tier. Before queueing, please apply run-all-spark-profiles and run-spark-4.1-tests and check their results. Fork workflow dispatch returned “Actions has been disabled for this repository,” so it does not provide current-head broader CI evidence.

@github-actions github-actions Bot added bug Something isn't working area:aggregation Hash aggregates, aggregate expressions labels Oct 9, 2026
@rich7420
rich7420 marked this pull request as ready for review October 10, 2026 06:14

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

Reviewed the entire two-file diff from bfacc43a50aba13da4fa8a828db52309541191b4 to 237473a5ee844f33e6fc9fbcafa4a30c0bbe3d3c. Confirmed the PR is not a draft. Read AGENTS.md and applied review-comet-pr and review-comet-expression-pr. The discussion snapshot and live comment checks contained no existing review concerns.

No introduced P1/P2 issues found within this review.

Summary

  • Prior state and problem: total_cmp places negative-sign NaNs before finite values. This diverged from Spark’s ordering and corrupted approximate-percentile results across buffer flushes.
  • Design approach: Retain the existing sort, then move its leading negative-NaN block to the end with an in-place rotation.
  • Correctness: The prefix predicate is monotonic after total_cmp, and rotation preserves finite ordering and signed zeros. Checked Spark’s QuantileSummaries, ApproximatePercentile, and Scala ordering sources. All 15 module tests passed. Additional comparisons against Spark covered flush boundaries, NaN signs and payloads, all-NaN buffers, late NaNs, and partial merges without finding a regression.
  • Compatibility analysis: Relevant ordering semantics agree across Spark 3.4.3, 3.5.9, 4.0.4, 4.1.3, and the 4.2 branch. Input support, null handling, output types, serialization, and fallback routing remain unchanged.
  • Key design decisions: Preserve NaN bits and -0.0 ordering while retaining Spark’s 50,000-value flush boundary, rank arithmetic, and merge behavior. NaN payload identity is explicitly outside the result-equivalence guarantee.
  • Implementation sketch: A first-element guard avoids unnecessary prefix searches. partition_point finds the negative-NaN prefix and rotate_left relocates it. Rust regressions cover sorting and serialized merges. The SQL fixture creates negative NaNs after the Parquet scan and compares native scalar, array, Float, Double, and grouped results with Spark.
  • Performance: Unaffected buffers gain a constant-time guard. Affected buffers add a logarithmic prefix search and linear in-place rotation over at most 50,000 values, without another heap buffer. No evidence-backed P1/P2 performance issue was identified. No benchmark was run.
  • Design: The correction is localized to the ordering mismatch and leaves the established quantile algorithm intact. This is a straightforward, bounded fix.
  • Abstraction & complexity: No new API or abstraction is introduced. The short post-sort adjustment has a clear invariant and avoids adding NaN handling to every comparator call.
  • Behavioral changes worth calling out: The reported 300,000-row negative-NaN case now produces [1.0, 75001.0, 150000.0, 225013.0, NaN]. This is also an intended correction relative to branch-1.1 at e9efd9f764ee0a59b7898ff028d6985d4a7a28e1, whose quantile and accumulator files match the PR base.
  • Suggested improvements: No additional code change met the P1/P2 reporting bar. Pending CI and the repository-required Spark SQL coverage remain validation work before queueing.

Validation: Compiled the exact-head quantile module directly with rustc --test: 15 passed. In a disposable copy, removing the rotation made both new NaN flush/merge regressions fail, reproducing incorrect finite percentiles and a finite maximum. A separate 23-case corpus matched Spark 3.5.9 source and Spark 4.1.3 classes, preserving finite and signed-zero bits while treating NaN payloads as unspecified.

Exact-head CI: Run 38030166915 currently has 18 successful checks, 7 in progress, and 14 skipped, with no reported failures. Native build and lint succeeded. Rust tests, four Spark 4.1 Comet test groups, and TPC-H/TPC-DS verification remain in progress. Spark SQL suites for every supported profile were skipped.

Validation limits: No full Cargo/JNI build, local Comet SQL fixture execution, or full Spark SQL suite was run. This checkout has no native build artifacts. The isolated tests establish kernel behavior, not full integration coverage. Earlier results in the PR description were not counted as exact-head validation. Project files remain unchanged.

@sunchao
sunchao added this pull request to the merge queue Oct 11, 2026
@sunchao

sunchao commented Oct 11, 2026

Copy link
Copy Markdown
Member

Merged, thanks @rich7420 !

Merged via the queue into apache:main with commit 52a3a33 Oct 11, 2026
42 checks passed
@rich7420

Copy link
Copy Markdown
Contributor Author

@sunchao and @andygrove thanks for the review!

@andygrove andygrove added the backport-1.1 Candidate for backporting to 1.1 release branch label Oct 11, 2026
andygrove added a commit that referenced this pull request Oct 12, 2026
(cherry picked from commit 52a3a33)

Co-authored-by: KUAN-HAO HUANG <101171023+rich7420@users.noreply.github.com>
dwsmith1983 pushed a commit to dwsmith1983/datafusion-comet that referenced this pull request Oct 12, 2026
…orders NaN as Spark does (apache#6866)

* refactor: stop normalizing float divisors now that percentile_approx orders NaN as Spark does

CometExecRule wrapped the divisor of a FLOAT or DOUBLE Divide or Remainder in a Project or Filter in NormalizeNaNAndZero. Comparisons don't need it, since the native comparison builder follows Spark's ordering, including the check for a zero divisor. It stayed only because it also canonicalized a NaN quotient: 1.0D / (-d) of a NaN d is a NaN with the sign bit set, and percentile_approx ordered that NaN below every other value (apache#6519). apache#6829 fixed percentile_approx, so the rule can go.

nan_divisor.sql keeps its percentile_approx query, which now checks the sign-bit NaN quotient directly, along with the hash and extrema queries for the earlier consumers.

* test: regenerate TPC-DS plans without the divisor normalization

Without NormalizeNaNAndZero around float divisors, the 11 approved plans that hold a float division in a Project or Filter count one native expression fewer, and q78 no longer lists knownfloatingpointnormalized and normalizenanandzero among its dispatched expressions. Regenerated on Spark 3.4, 3.5, 4.0, 4.1 and 4.2; 4.1 and 4.2 need no override.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:aggregation Hash aggregates, aggregate expressions backport-1.1 Candidate for backporting to 1.1 release branch bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

percentile_approx returns different percentiles from Spark when its input holds a NaN with the sign bit set

3 participants