comphead commented on code in PR #6760:
URL: https://github.com/apache/datafusion-comet/pull/6760#discussion_r4231745018
##########
native/spark-expr/src/conditional_funcs/case_when.rs:
##########
@@ -162,6 +163,38 @@ fn coerce_branch(
))
}
+/// Casts the arguments of a Spark `ComplexTypeMergingExpression` such as
`greatest` or `least` to
+/// their common type, which keeps the first argument's field names, as
Spark's result type does.
+///
+/// Spark has already given the arguments the same SQL type up to nullability,
and up to the case
+/// of struct field names when the analysis is case-insensitive. It compares
structs field by field
+/// by position. DataFusion's struct coercion and Arrow's struct cast both
match fields by name
+/// when two structs hold the same set of names, which pairs different
positions when the names
+/// differ only in case, so the arguments are reconciled here positionally
instead. The arguments
+/// are returned unchanged when they have no positional common type.
+pub fn coerce_to_common_type(
Review Comment:
#6428 at its latest head (`ba8f2a5cd4`) renames `if_common_type` and
`coerce_branch` to `positional_common_type`, which takes a
`PositionalTypeCoercion` mode, and a public `cast_to_common_type`. Its new `IN`
arm in the planner also folds a common type over the operands and casts each
one, as this function does. The two PRs conflict in `planner.rs`,
`case_when.rs` and `mod.rs`, so whichever lands second has to adapt. Could that
one leave a single helper for `IN`, `greatest` and `least`? `MetadataOnly`
looks like the right mode here, since #6428 documents it as the comparison mode
where Catalyst has already coerced the leaf types.
##########
native/spark-expr/src/conditional_funcs/case_when.rs:
##########
@@ -1577,4 +1610,95 @@ mod tests {
Some(null)
));
}
+
+ /// `greatest` and `least` arguments whose struct fields hold the same
names in another order,
+ /// which Spark accepts when the names differ only in case. They are cast
to the first
+ /// argument's type by position, with each field nullable if any
argument's is. Matching the
+ /// fields by name would put the second argument's `x` value in the first
position.
+ #[test]
+ fn coerce_to_common_type_is_positional() {
Review Comment:
`if_reconciles_case_variant_fields_positionally` and
`if_reconciles_struct_field_nullability_and_names` already pin `if_common_type`
and `coerce_branch` on swapped case-variant names and merged nullability. The
new SQL file covers this function end to end for structs and arrays of structs,
including the three-argument fold that this test does not reach, and the
description says all eight queries fail without the fix. Could this test be
dropped, since it calls the same helpers with two arguments?
##########
spark/src/test/resources/sql-tests/expressions/math/greatest_least_struct_field_case.sql:
##########
@@ -0,0 +1,83 @@
+-- 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.
+
+-- With case-insensitive analysis, Spark accepts greatest and least arguments
whose struct field
+-- names differ only in case, and compares the structs field by field by
position. The result
+-- takes the first argument's field names. Here the second argument holds the
same names in the
+-- other order, so matching the fields by name instead would pair x with x and
give a different
+-- row for id = 0 and id = 2.
+
+-- 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]
