fix: honor strict floating-point mode for array extrema - #5403
Open
sunchao wants to merge 1 commit into
Open
Conversation
comphead
reviewed
Aug 21, 2026
| (array(0.0, double('-0.0'), 1.0)) | ||
|
|
||
| query ignore(array_min signed-zero: Spark +0.0, Comet -0.0) | ||
| query ignore(https://github.com/apache/datafusion-comet/issues/5401) |
Contributor
There was a problem hiding this comment.
is it any chance to use query expect_fallback so once this fixed we would know the test should be addressed as well, instead of ignoring it
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Partially addresses #5401 by fixing strict-mode evaluation. The issue remains open for native signed-zero parity when strict mode is off.
Why are the changes needed?
Enabling
spark.comet.exec.strictFloatingPoint=trueshould protect queries from floating-point differences between Spark and Comet.array_minandarray_maxcurrently miss that protection: they are classified as compatible regardless of this setting, so strict mode still executes native comparisons that can return a different zero sign from Spark.Consider these two floating-point array values read from a table:
array_minarray_minarray_maxarray_max[+0.0, -0.0]+0.0-0.0+0.0+0.0[-0.0, +0.0]-0.0-0.0-0.0+0.0Spark considers the two zeros equal and keeps the first equal element. The native implementation orders
-0.0before+0.0, so it always picks negative zero for the minimum and positive zero for the maximum. The values compare equal numerically, but their signs remain observable, for example when converted to strings. A user requesting strict compatibility should not get a different result merely because Comet selected the native implementation.The table describes actual
FLOAT/DOUBLEvalues, not bare SQL decimal literals. The regressions construct negative zero withfloat('-0.0')ordouble('-0.0')and read it from Parquet columns. The literal corrections in #5393 exposed this pre-existing compatibility gap; they did not introduce it.What changes were proposed in this PR?
This PR brings floating-point array extrema into Comet's existing compatibility-routing mechanism. In strict mode, Comet evaluates these expressions with Spark's own generated code through the existing JVM codegen dispatcher. The expression can therefore remain inside the Comet pipeline while using Spark's comparison rules. If the dispatcher is disabled or cannot handle the expression, the operator falls back to Spark.
The decision follows the type of the returned element, including floats and doubles nested inside arrays or structs. This prevents a nested result from bypassing strict mode, while leaving non-floating extrema on their existing native path. Non-strict execution is unchanged, and an explicit per-expression
allowIncompatibleopt-in still selects the native implementation.The scope is to make strict mode honor Spark's behavior. DataFusion and Arrow comparison semantics are unchanged, so the default-mode native difference remains tracked in #5401. The compatibility documentation is updated to explain that boundary, including the fact that JVM evaluation is conditional rather than the default for every array-extrema query.
How was this PR tested?
All 10 directly changed SQL-file/configuration cases passed on each of Spark 3.4.3 and 4.1.3, using Comet with the unchanged native library built first under JDK 17. The cases exercise the dispatcher, forced Spark fallback, and explicit native opt-in, with both zero orders and floating-point widths. They also cover column and literal inputs, nested arrays, and null/empty inputs. A fresh Spark 3.4.3 replay during review confirmed the same results.
To verify that the new assertions catch the original bug, the strict min/max fixtures were also run against the unpatched Spark 3.4.3 Comet runtime. All four min/max dictionary-setting combinations failed on the expected zero-sign mismatches, with native execution visible in the plans. The patched runtime passes those same fixtures.
The Spark implementations were source-checked across supported versions. Documentation generation, Scala compilation with Scalastyle enabled, Spotless, Prettier, and
git diff --checkpassed. The dictionary matrix variesparquet.enable.dictionary; it does not establish that the small fixtures contain dictionary-encoded pages. Local validation did not include the full test suite or a native Comet run on Spark 4.2.Focused test commands
Each selector was run separately:
Repeated with
-Pspark-3.4, using a clean reactor build when switching profiles. Local runs used dependency-cache and native-library-directory overrides. These substring selectors also include neighboring fixtures; the count above includes only the changed fixtures and their configuration variants.