comphead commented on code in PR #6761:
URL: https://github.com/apache/datafusion-comet/pull/6761#discussion_r4231744669
##########
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:
Could this PR also update the `greatest` and `least` entries in
`docs/source/contributor-guide/expression-audits/math_funcs.md`? Lines 152 and
161 say that only arrays and structs with a floating-point leaf use
`SparkGreatestLeast` and that other types use DataFusion's `GreatestFunc` and
`LeastFunc`. That no longer holds for nested types without a float leaf, and
the `least` entry should also say it orders a null inside an array or struct
first, as Spark does. #6457 added those two lines together with the float
routing.
##########
native/spark-expr/src/math_funcs/greatest_least.rs:
##########
@@ -408,8 +408,44 @@ mod tests {
Ok(())
}
+ /// Spark orders a null inside a struct before every other value, in
`least` as in `greatest`.
#[test]
- fn handles_floats_and_nested_floats_only() {
+ fn nested_nulls_sort_first() -> Result<()> {
Review Comment:
`nested_nulls_sort_first` calls `invoke_with_args` directly, so it never
goes through `handles`, and `pick` and `spark_comparator` are the same as on
the base. From reading the code I expect it to pass without this change too. I
have not run it. Its two structs are the `id = 3` row of the first query in
`greatest_least_nested_nulls.sql`, which does fail without the fix. Could we
drop it and keep `handles_floats_and_nested_types_only` as the unit-level guard
for the routing?
##########
spark/src/test/resources/sql-tests/expressions/math/greatest_least_nested_nulls.sql:
##########
@@ -0,0 +1,43 @@
+-- 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.
+
+-- Spark ranks a null inside a struct or array before every other value, in
least as in greatest.
+-- For id = 3 the second argument's first field is null, so least picks it and
greatest does not.
+
+-- Config: spark.comet.exec.range.enabled=true
+-- Config: spark.comet.sparkToColumnar.enabled=true
+-- Config: spark.comet.sparkToColumnar.supportedOperatorList=Range
Review Comment:
With `spark.comet.exec.range.enabled=true`, `CometExecRule` turns
`range(...)` into `CometRangeExec` and only falls back to the Spark-to-Arrow
conversion when that operator declines a range (around line 504 of
`CometExecRule.scala`). So the two `sparkToColumnar` lines have no effect here,
and naming `Range` in `spark.comet.sparkToColumnar.supportedOperatorList` is
deprecated per its doc in `CometConf.scala`. Could we drop both lines? The file
would then fail if native `Range` ever stopped taking these ranges, rather than
quietly running on a converted leaf.
--
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]
