sunchao commented on code in PR #6475:
URL: https://github.com/apache/datafusion-comet/pull/6475#discussion_r4156038252
##########
spark/src/main/scala/org/apache/comet/serde/CometSortOrder.scala:
##########
@@ -20,40 +20,20 @@
package org.apache.comet.serde
import org.apache.spark.sql.catalyst.expressions.{Ascending, Attribute,
Descending, NullsFirst, NullsLast, SortOrder}
-import org.apache.spark.sql.types.{DoubleType, FloatType}
-import org.apache.comet.CometConf
import org.apache.comet.serde.QueryPlanSerde.exprToProtoInternal
+/**
+ * The key of a Sort, TopK, Window, WindowGroupLimit or range partitioning.
+ *
+ * Every key type is compatible, in strict floating-point mode too. Arrow
orders floats by IEEE
Review Comment:
[P2] Retain strict-mode protection for nullable nested keys until their null
semantics match Spark. Removing `getSupportLevel` now admits these keys with
`spark.comet.exec.strictFloatingPoint=true` and
`spark.comet.expression.SortOrder.allowIncompatible=false`. For Parquet `t(id
INT, d DOUBLE)` containing `(1,1.0),(2,NULL),(3,3.0)`, with one shuffle
partition, `ORDER BY array(d) ASC NULLS LAST, id` produces IDs `[1,3,2]`
instead of Spark’s `[2,1,3]`. `SUM(id) OVER (ORDER BY array(d))` produces `6`
for every row instead of `(id,running) = [(1,3),(2,2),(3,6)]`. These silently
incorrect results were previously avoided by the strict-mode fallback. The
underlying bugs are tracked in #6476/#6477, but exposing previously protected
floating-point queries is introduced here. Preserve a guard for affected shapes
or fix those comparisons before declaring every key compatible.
Evidence: A freshly compiled probe included this exact checkout’s
`normalize.rs` and used locked Arrow 59.3.0/DataFusion 55.1.0. After
normalization, array and struct sorting returned `[1,3,2]` for ASC NULLS LAST,
and the array RANGE window returned sums `[6,6,6]`. Spark 3.5.9 reference
queries returned `[2,1,3]` and sums `[3,2,6]` by ID. Base
`CometSortOrder.getSupportLevel` returns `Incompatible` for these nested DOUBLE
keys in strict mode, and `CometWindowExec` requires successful sort-order
serialization.
##########
spark/src/test/resources/sql-tests/windows/nested_float_order_keys.sql:
##########
@@ -0,0 +1,108 @@
+-- Licensed to the Apache Software Foundation (ASF) under one
+-- or more contributor license agreements. See the NOTICE file
+-- distributed with this work for additional information
+-- regarding copyright ownership. The ASF licenses this file
+-- to you under the Apache License, Version 2.0 (the
+-- "License"); you may not use this file except in compliance
+-- with the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing,
+-- software distributed under the License is distributed on an
+-- "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+-- KIND, either express or implied. See the License for the
+-- specific language governing permissions and limitations
+-- under the License.
+
+-- Sort and window keys that nest floats in arrays and structs follow Spark's
ordering at every
+-- depth: -0.0 equals 0.0, every NaN equals every other NaN, and NaN sorts
above every other
+-- value. Only the comparison keys are normalized, so returned values keep
their bits.
+--
+-- The keys use IF(s, -d, d): rows 1 and 2 hold -0.0 and 0.0, and rows 4 and 5
a canonical NaN
+-- and a NaN with the sign bit set, the NaN that arithmetic produces on
x86-64, which Arrow's
+-- total order sorts below -Infinity. Each ORDER BY ends with a unique
tiebreaker, so peers come
+-- out in the tiebreaker's order.
+
+-- Strict floating-point mode no longer needs to fall back for these keys. The
test harness
+-- admits incompatible sort orders by default, so turn that off to check the
shipped policy.
+-- ConfigMatrix: spark.comet.exec.strictFloatingPoint=false,true
+-- Config: spark.comet.expression.SortOrder.allowIncompatible=false
+
+statement
+CREATE TABLE nested_float_keys(id INT, g INT, d DOUBLE, f FLOAT, s BOOLEAN)
USING parquet
+
+statement
+INSERT INTO nested_float_keys VALUES
+ (1, 1, 0.0D, float('0.0'), true),
+ (2, 1, 0.0D, float('0.0'), false),
+ (3, 1, 1.0D, float('1.0'), false),
+ (4, 2, double('NaN'), float('NaN'), false),
+ (5, 2, double('NaN'), float('NaN'), true),
+ (6, 2, double('Infinity'), float('Infinity'), false),
+ (7, 2, -1.0D, float('-1.0'), false),
+ (8, 1, NULL, NULL, false),
+ (9, 2, double('-Infinity'), float('-Infinity'), false)
+
+-- Sort. Row 8's keys hold a null element, which Spark orders below every
other value whatever
+-- the key's null order. Arrow ties the order of nested nulls to NULLS FIRST
or LAST, so these
+-- queries keep the default null order, where the two agree (#6476).
+query
+SELECT id FROM nested_float_keys ORDER BY array(IF(s, -d, d)), id DESC
+
+query
+SELECT id FROM nested_float_keys ORDER BY array(IF(s, -f, f)) DESC, id
+
+query
+SELECT id FROM nested_float_keys ORDER BY named_struct('x', IF(s, -d, d))
DESC, id DESC
+
+query
+SELECT id FROM nested_float_keys ORDER BY named_struct('x', IF(s, -f, f)), id
+
+-- Nested more deeply, and more than one nested key
+query
+SELECT id FROM nested_float_keys
+ORDER BY array(named_struct('x', IF(s, -d, d))), named_struct('a', array(IF(s,
-f, f))) DESC, id
+
+-- Returning the keys as well. CometExpressionSuite checks that their bits
come back unchanged.
+query
+SELECT id, array(IF(s, -d, d)) AS k, named_struct('x', IF(s, -f, f)) AS t
+FROM nested_float_keys ORDER BY k, id DESC
+
+-- TopK
+query
+SELECT id FROM nested_float_keys ORDER BY array(IF(s, -d, d)) DESC, id LIMIT 4
+
+query
+SELECT id FROM nested_float_keys ORDER BY named_struct('x', IF(s, -f, f)), id
DESC LIMIT 4
+
+-- Window order keys: peers share a rank, and the default RANGE frame of a
running sum spans
+-- all of them. The running sums leave out row 8: DataFusion finds a RANGE
frame's end by ordering
+-- a null element above every value, while the sort puts it first, so the
frame of every row
+-- after it would run to the end of the partition, with or without floats
(#6477).
+query
+SELECT id,
+ RANK() OVER (ORDER BY array(IF(s, -d, d))) AS r,
+ DENSE_RANK() OVER (PARTITION BY g ORDER BY named_struct('x', IF(s, -f, f))
DESC) AS dr
+FROM nested_float_keys
+
+-- Comet declines to sort on a lone struct column, which Arrow cannot sort, so
every struct key
+-- comes with a second key or a partition.
+query
+SELECT id,
+ RANK() OVER (PARTITION BY g ORDER BY named_struct('x', IF(s, -d, d))) AS r,
+ SUM(id) OVER (ORDER BY array(IF(s, -d, d))) AS running,
+ SUM(id) OVER (PARTITION BY g ORDER BY array(IF(s, -f, f)) DESC) AS by_group
+FROM nested_float_keys WHERE id <> 8
+
+-- A rank limit keeps every peer of the last rank it admits
+query
Review Comment:
[P2] Reconcile the JVM rank-limit guard with these native-execution
assertions. This plain `query` requires an entirely native plan, but Spark’s
default rank-limit optimization inserts `WindowGroupLimit`, whose converter
still unconditionally rejects nested floating-point order keys for `RANK` and
`DENSE_RANK`. Consequently, the new fixture fails under both strict-mode
settings and blocks required CI. The Rust peer test bypasses this converter, so
its success does not establish end-to-end native support. Update the guard and
its associated fallback coverage to reflect the newly normalized keys before
requiring native execution here.
Evidence: Exact-head run
https://github.com/apache/datafusion-comet/actions/runs/36813076101/job/110214256198
reports both `nested_float_order_keys.sql` configurations failing at line 100
with `Expected only Comet native operators, but found Project`. Both partial
and final `WindowGroupLimit` nodes report `RANK and DENSE_RANK compare nested
floating-point values exactly, so they fall back`. The responsible guard
remains in `CometWindowGroupLimitExec.scala:108-127`. Focused suite command:
`./mvnw test -Dtest=none -Dsuites="org.apache.comet.CometSqlFileTestSuite
nested_float_order_keys"` after the documented native build.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
