andygrove commented on code in PR #6827:
URL: https://github.com/apache/datafusion-comet/pull/6827#discussion_r4237878377
##########
native/spark-expr/src/conditional_funcs/case_when.rs:
##########
@@ -60,8 +59,17 @@ pub fn create_case_when(
.transpose()?
.unwrap_or(DataType::Null);
- let Some(coerce_type) = get_coerce_type_for_case_expression(&then_types,
Some(&else_type))
- else {
+ // Spark merges THEN types from left to right, with ELSE last, retaining
the first THEN's
Review Comment:
`CometCoalesce` lowers `coalesce` and `nvl` to this same `CaseWhen` proto,
with the last argument as the ELSE, so this fold also decides the field names
of their results. On `main` the query below makes Comet return `{"X":102}`
where Spark returns `{"x":102}`. It passes with this change. Nothing in the PR
pins it, so a later change to `CometCoalesce` could regress it quietly.
Could you add it to a small `coalesce_field_names.sql` that reuses the setup
from `case_when_field_names.sql`? The last argument needs a different case from
the first, or the query also passes on `main`.
```sql
query expect_native(coalesce)
SELECT to_json(coalesce(
CASE WHEN p THEN named_struct('x', i) END,
CASE WHEN q THEN named_struct('X', i + 100) END,
named_struct('X', 7)))
FROM test_case_field_names
```
##########
spark/src/test/resources/sql-tests/expressions/conditional/case_when_field_names.sql:
##########
@@ -0,0 +1,52 @@
+-- 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 preserves the first THEN's struct field names when branches differ
only in case.
+-- Native to_json exposes Arrow field names that the row comparison would
otherwise ignore.
+-- https://github.com/apache/datafusion-comet/issues/6482
+-- Config: spark.sql.caseSensitive=false
+-- Config: spark.comet.expression.StructsToJson.allowIncompatible=true
+
+statement
+CREATE TABLE test_case_field_names(p boolean, q boolean, i int, d double)
USING parquet
+
+statement
+INSERT INTO test_case_field_names VALUES
+ (true, false, 1, 1.5), (false, true, 2, 2.5),
+ (NULL, false, 3, 3.5), (false, NULL, NULL, NULL)
+
+-- The result keeps the THEN name for both selected branches, in either name
order.
+query expect_native(casewhen)
+SELECT
+ to_json(CASE WHEN p THEN named_struct('x', i) ELSE named_struct('X', i) END),
+ to_json(CASE WHEN p THEN named_struct('X', i) ELSE named_struct('x', i) END)
+FROM test_case_field_names
+
+-- Later THEN branches and an omitted ELSE must also preserve the first THEN's
name.
+query expect_native(casewhen)
+SELECT
+ to_json(CASE WHEN p THEN named_struct('x', i)
+ WHEN q THEN named_struct('X', i + 10) ELSE named_struct('X', i + 20) END),
+ to_json(CASE WHEN p THEN named_struct('x', i)
+ WHEN q THEN named_struct('X', i + 10) END)
+FROM test_case_field_names
+
+-- Fields align by position. Matching these names would incorrectly combine
INT with DOUBLE.
+query expect_native(casewhen)
+SELECT to_json(CASE WHEN p THEN named_struct('x', i, 'X', CAST(5.5 AS DOUBLE))
+ ELSE named_struct('X', 0, 'x', d) END)
+FROM test_case_field_names
Review Comment:
These queries use flat structs, and the first THEN is always a struct. Two
shapes that reach other parts of the new fold fail on `main` and pass here.
Could you add them?
The first puts case-variant names at two levels, which exercises the
recursion in `merge_branch_types`. Please keep both branch orders. The second
starts the fold from a typed NULL.
```sql
query expect_native(casewhen)
SELECT
to_json(CASE WHEN p THEN named_struct('a', named_struct('x', i)) ELSE
named_struct('A', named_struct('X', i)) END),
to_json(CASE WHEN p THEN named_struct('A', named_struct('X', i)) ELSE
named_struct('a', named_struct('x', i)) END)
FROM test_case_field_names
query expect_native(casewhen)
SELECT to_json(CASE WHEN p THEN CAST(NULL AS STRUCT<x: INT>) WHEN q THEN
named_struct('x', i) ELSE named_struct('X', i) END)
FROM test_case_field_names
```
##########
native/spark-expr/src/conditional_funcs/case_when/tests.rs:
##########
@@ -0,0 +1,791 @@
+// Licensed to the Apache Software Foundation (ASF) under one
Review Comment:
Was there a reason to move the tests into this file as part of the fix? The
move makes the diff about 1,650 changed lines and hides what changed in the
tests, which is two extended tests, one timestamp expectation and a helper
rename.
It also turns open PRs that edit the inline `mod tests` into conflicts. I
merged this branch with #6716, #6763 and #6135 locally and all three now
conflict in `case_when.rs`. With the tests left in place, #6716 and #6135 merge
cleanly, and #6763 keeps only the conflict it already has with `main`.
Could the move go in a separate PR after those land?
--
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]
