sunchao commented on code in PR #6761:
URL: https://github.com/apache/datafusion-comet/pull/6761#discussion_r4235489931
##########
native/spark-expr/src/math_funcs/greatest_least.rs:
##########
@@ -51,11 +51,11 @@ impl SparkGreatestLeast {
}
}
- /// Whether arguments of this type need Spark's float ordering.
DataFusion's `greatest` and
- /// `least` already match Spark for every other type, since equal values
are identical there.
+ /// Whether arguments of this type need Spark's ordering, of floats or of
nulls inside nested
+ /// values. DataFusion's `greatest` and `least` already match Spark for
every other type, since
+ /// equal values are identical there.
pub fn handles(data_type: &DataType) -> bool {
- matches!(data_type, DataType::Float32 | DataType::Float64)
- || (data_type.is_nested() && has_float_leaf(data_type))
+ matches!(data_type, DataType::Float32 | DataType::Float64) ||
data_type.is_nested()
Review Comment:
[P2] Could the expanded routing preserve constant reduction for float-free
nested arguments? For an `ARRAY<INT>` column with multiple constant-array
arguments, DataFusion’s `execute_conditional` previously reduced the constants
to one before broadcasting. `SparkGreatestLeast::invoke_with_args` instead
broadcasts every constant and performs a full comparison and `zip` for each. An
8,192-row batch with eight 16-element constant arrays allocated approximately
1.68 MB through base routing versus 12.94 MB for head `greatest` and 10.18 MB
for head `least`, with identical results. Spark retains these separate
constants after optimization, so ordinary queries reach this path. This
introduces repeated batch processing and allocation even for already-correct
`greatest` calls. Please retain constant reduction using Spark’s nested
ordering for float-free inputs, or preserve the existing route where its
semantics already match.
Evidence: Reproduction input: one ListArray<Int32> containing 8,192 rows,
each row repeating `row_index % 21` sixteen times, followed by eight
ScalarValue::List arguments repeating constants 0 through 7 sixteen times each.
Compare DataFusion GreatestFunc/LeastFunc, selected by base routing, with
exact-head SparkGreatestLeast. Output arrays match. An instrumented allocator
measured cumulative allocated bytes per invocation: greatest 1,675,836 ->
12,936,608, least 1,675,836 -> 10,184,096. With two constants, both increase
from 1,674,716 to 2,890,136 bytes. Harness and compile arguments are saved
under `/tmp/review6761-ad544-session-621i9yw7/`. Run `perf-tests
perf::nested_constant_cost --nocapture --test-threads=1`. Spark 3.5.9 confirmed
that `SELECT greatest(array_repeat(CAST(id % 21 AS INT),16),
array_repeat(0,16), array_repeat(1,16), array_repeat(2,16), array_repeat(3,16),
array_repeat(4,16), array_repeat(5,16), array_repeat(6,16), array_repeat(7,16))
FROM range(8192)` retains on
e column-dependent child and eight literal children. The same holds for
`least`. Timing from cached debug dependencies is not used as a
release-performance claim.
--
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]
