Skip to content

fix: honor strict floating-point mode for array extrema - #5403

Open
sunchao wants to merge 1 commit into
apache:mainfrom
sunchao:dev/chao/codex/comet-strict-fp-array-extrema
Open

fix: honor strict floating-point mode for array extrema#5403
sunchao wants to merge 1 commit into
apache:mainfrom
sunchao:dev/chao/codex/comet-strict-fp-array-extrema

Conversation

@sunchao

@sunchao sunchao commented Aug 21, 2026

Copy link
Copy Markdown
Member

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=true should protect queries from floating-point differences between Spark and Comet. array_min and array_max currently 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 value Spark array_min Native array_min Spark array_max Native array_max
[+0.0, -0.0] +0.0 -0.0 +0.0 +0.0
[-0.0, +0.0] -0.0 -0.0 -0.0 +0.0

Spark considers the two zeros equal and keeps the first equal element. The native implementation orders -0.0 before +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/DOUBLE values, not bare SQL decimal literals. The regressions construct negative zero with float('-0.0') or double('-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 allowIncompatible opt-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 --check passed. The dictionary matrix varies parquet.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:

./mvnw test -Pspark-4.1 -Dtest=none -Dscalastyle.skip=true -Dsuites="org.apache.comet.CometSqlFileTestSuite array_min"
./mvnw test -Pspark-4.1 -Dtest=none -Dscalastyle.skip=true -Dsuites="org.apache.comet.CometSqlFileTestSuite array_max"
./mvnw test -Pspark-4.1 -Dtest=none -Dscalastyle.skip=true -Dsuites="org.apache.comet.CometSqlFileTestSuite array_extrema_strict_fp"

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.

@sunchao
sunchao requested review from andygrove and comphead August 21, 2026 15:32
(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)

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.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants