[andygrove]

Which issue does this PR close?

Closes #6636.

Part of #6615 and #6385.

Rationale for this change

CometFloatSemanticsSuite crossed Spark's -0.0 and NaN edge values with every operator context and float expression, as 412 runtime tests generated by one test(c.name) call. It only called checkSparkAnswer, so a case that fell back to Spark passed without exercising Comet. #6615 moves such sweeps to SQL file tests.

#6636 expected most of the 412 cases to be known gaps that the SQL framework cannot hold. #6447, #6457, #6475 and #6518 closed those gaps: the one that remains is collect_set before Spark 4.2 (#5312).

What changes are included in this PR?

• The matching cases move to sql-tests/float_semantics/, 400 queries over DOUBLE and FLOAT in seven files.

  File	Queries
  comparison.sql	112: Project, Filter, aggregate argument and FILTER, sort key, Generate, window argument and partition key
  comparison_joins.sql	56: broadcast hash, shuffled hash, sort merge and nested loop join conditions
  comparison_nested.sql	56: array and struct operands
  literal_comparison.sql	42: a column and each edge literal on either side, in Project, Filter and a Parquet scan filter
  keys.sql	40: grouping, join, set, IN, sort and window keys
  expressions.sql	68
  aggregates.sql	24
  aggregates_collect_set_4_2.sql	2: collect_set, MinSparkVersion: 4.2

  Each file builds its tables from the nine edge values and a flag. Spark's Parquet writer canonicalizes NaN, so the sign-bit NaN is made at query time by negating the stored NaN, in a CTE that replaces the suite's temporary views. literal_comparison.sql, keys.sql and expressions.sql turn ConstantFolding on, as the sweep always ran, because a literal such as -CAST('NaN' AS DOUBLE) has to fold for the sign-bit NaN to reach Comet as a constant and for the Parquet scan filter to be pushed down.

• A plain query asserts native execution. 392 of the 400 do. The other 8 are query spark_answer_only, each with the reason above it: mode (opt-in, mode.sql runs it natively), array_distinct and array_union (native only on some Spark patch releases, array_distinct.sql and array_union.sql) and the map lookup (a float key falls back, map_builders_float_key_gate.sql).

• 12 cases are not moved, because existing fixtures hold the same cases over the same edge values: hash, xxhash64 and their array and struct forms (expressions/hash/hash.sql), and the nested ORDER BY and window order keys (windows/nested_float_order_keys.sql).

• CometFloatSemanticsSuite shrinks to the one known gap. A SQL file test has no mode that expects a difference, so collect_set before Spark 4.2 stays a strict Scala test that fails once the answers match, and aggregates_collect_set_4_2.sql covers 4.2 and later. The test removes itself when Evaluate extending collect_set input normalization to Spark 3.4 through 4.1 #5312 lands.

• contributor-guide/floating_point.md points a new expression at the fixtures.

How are these changes tested?

The fixtures are the tests. CometSqlFileTestSuite float_semantics passes on Spark 3.4, 3.5, 4.0, 4.1 and 4.2 (8 tests each; aggregates_collect_set_4_2.sql only runs on 4.2), and CometFloatSemanticsSuite passes on 4.1. The seven files take about a minute together on 4.1.

I checked that the converted sweep still fails when the native helpers are wrong, by mutating native/spark-expr/src/float_semantics/mod.rs and running the fixtures on 4.1:

Mutation	Files that fail
normalize_float stops canonicalizing NaN	expressions, aggregates, comparison_joins, keys
normalize_float stops folding -0.0	expressions, aggregates, keys
compare_floats stops treating two NaNs as equal	expressions, comparison_nested

Join conditions are planned as intended (CometBroadcastHashJoin, CometHashJoin, CometSortMergeJoin, CometBroadcastNestedLoopJoin); the = and <=> cases of the nested loop context become hash joins, as they did in the suite.

The change adds no production code, but it touches every Spark profile's fixtures, so it wants run-all-spark-profiles.

[sunchao]

No introduced P1/P2 issues found within this review.

Reviewed the full ten-file PR diff at d619e5e673ddba6fca05491f0e2dcae557c83656 against base 498956132ced6f4e45453841aeae4e56e8e98e61. Verified that the apparent release-tooling differences belong to a newer base-only commit. The PR remains non-draft. The snapshot and refreshed GitHub discussion contain no reviews or comments.

Routed skills: review-comet-pr and review-comet-expression-pr. Read AGENTS.md and the relevant contributor guides.

Summary

• Prior state and problem: The 412 generated Scala cases used checkSparkAnswer, allowing fallback to Spark to satisfy tests intended to exercise Comet.
• Design approach: Move 400 cases into eight SQL fixtures, reuse existing coverage for twelve cases, and retain the pre-4.2 collect_set mismatch tests in Scala.
• Correctness: A mechanical comparison confirmed that all 400 migrated queries preserve the old templates after alias normalization. Setup data and signed-NaN reconstruction are preserved. Spark source confirms SQL floating-point ordering and the collect_set normalization change in SPARK-57298.
• Compatibility analysis: Checked relevant Spark sources for 3.4.3, 3.5.9, 4.0.4, 4.1.3 and 4.2.0. The MinSparkVersion: 4.2 fixture complements the older-version Scala tests. Version-dependent array operations retain answer-only checks. No production behavior change is introduced relative to the latest release branch, branch-1.1.
• Key design decisions: Enabling ConstantFolding preserves folded signed-NaN literals and scan-filter tests. Eight documented answer-only queries accommodate existing fallback or opt-in behavior.
• Implementation sketch: Fixtures create nine edge values per type and, where needed, 81 ordered pairs. CTEs reconstruct signed NaNs after Parquet storage. The remaining Scala tests fail when the known collect_set mismatch disappears.
• Performance: Production execution paths are unchanged. Fixtures repeat small setup datasets to remain independent. No evidence-backed P1/P2 overhead issue was identified, and no performance benchmark was run.
• Design: Reusing the SQL test framework strengthens execution coverage while keeping the known-mismatch assertion in the framework that supports it.
• Abstraction & complexity: The change removes the generated-case model and adds no test-framework abstraction. Explicit SQL and self-contained setup make individual failures traceable.
• Behavioral changes worth calling out: 392 queries now require Comet operators as well as matching answers. Existing hash and nested-order fixtures retain the deliberately omitted coverage. The inherited collect_set gap remains explicitly tracked.
• Suggested improvements: No additional change meeting the P1/P2 reporting bar was identified.

Validation: The exact-head SQL parser accepted all eight fixtures, including cleanup tables, constant-folding directives and version gating. Spark 4.1.3 executed all 398 applicable queries and 38 setup statements successfully. Additional checks verified signed-zero and signed-NaN bits and pre-4.2 collect_set behavior. The working tree remains unchanged.

Exact-head CI: 20 checks succeeded, 39 were skipped, and three remained running: two native-library builds and Rust tests. Java lint passed across all five Spark profiles. No failures were reported. run-all-spark-profiles is present, but Comet suite results were not yet available. Spark SQL and Iceberg jobs were skipped.

Validation limits: The local runtime run used Spark alone. No Comet/native build or multi-profile Comet execution was performed locally, so native parity remains dependent on the pending CI results.

[rich7420]

Could a known mismatch go in a strict CometFloatSemanticsSuite test instead of ignore, so the test fails once it's fixed?

Suggested change

	query for `DOUBLE` and one for `FLOAT`. If one does not match Spark yet, mark its queries
	`ignore(<issue>)`. A SQL file test cannot expect a difference, so a gap that should fail once it
	is fixed goes in `CometFloatSemanticsSuite`, whose test for `collect_set` before Spark 4.2 fails
	when the answers start to match.
	query for `DOUBLE` and one for `FLOAT`. If one does not match Spark yet, add a strict test in
	`CometFloatSemanticsSuite` that links the issue and fails once the answers match, like its
	`collect_set` test before Spark 4.2.

[rich7420]

Could we keep a FLOAT array hash case? hash.sql only hashes array(-d), so hash(array(-f)) and xxhash64(array(-f)) seem to lose coverage.