fix: preserve CASE WHEN struct field names from the first THEN branch

[pingzh]

Which issue does this PR close?

Closes #6482.

Rationale for this change

With case-insensitive analysis, Spark accepts CASE WHEN q THEN named_struct('x', i) ELSE named_struct('X', i) END and names the result field x. Native CASE WHEN chose the ELSE branch's name instead, making native to_json emit {"X":7} instead of {"x":7}.

What changes are included in this PR?

• Merge branch types from the first THEN through the remaining THEN branches, with ELSE last. Reuse IF's positional struct merge as merge_branch_types, retaining field names and combining nested nullability without matching fields by name.
• Keep casts for differing branch types. The synthetic timezone regression now expects the first THEN's label when present, consistent with IF; normal Comet timestamp inputs remain UTC.
• Move the existing Rust unit tests into case_when/tests.rs and extend the existing struct checks to cover CASE WHEN as well as IF.

How are these changes tested?

• Add native SQL regressions for both field-name casing orders, multiple WHEN branches, omitted ELSE, and positional INT/DOUBLE struct fields. Native to_json exposes names that ordinary row comparison ignores.
• Rust checks cover complete Arrow types, nullability, values, and mixed/all-THEN/all-ELSE batches.
• Two independent source reviews completed. Rust formatting and git diff --check passed; no pre-commit hook is installed.
• Local tests were not run. Runtime validation is pending CI. A maintainer needs to add run-spark-4.1-tests for Spark SQL coverage before this leaves draft; GitHub denied the label request because pingzh lacks label permissions.

[ajsquared]

Reviewed the current changes and feedback; no independently confirmed unresolved P1 remains.

[pingzh]

cc @andygrove 🙏

[sunchao]

Summary

• Prior state and problem: Native CASE used an ELSE-first common-type calculation. That could select the wrong struct field names, and name-based matching could combine different field positions. With case-insensitive analysis, Spark accepts CASE WHEN p THEN named_struct('x', i) ELSE named_struct('X', i) END and derives the result name from the first THEN. Native to_json makes this schema difference visible.
• Design approach: Merge the first THEN type with subsequent THEN types in order, then merge ELSE. Reuse IF's positional merge for structs, lists and maps, preserving left-side names and combining nested nullability. Keep branch casts when the full Arrow types differ.
• Correctness / compatibility analysis: No introduced P1/P2 finding established from the source review of 04f328ccbae5888baf7f5a36ec037eca954ce6de, against actual merge base 00a4b422f0ed6560ef76dce004b94c3697613108 and selected main at 6960a48ae8d302ed6b836f3213367ebecebb9515. Checked pinned Spark 4.1.3 and 3.5.9 conditional source, Spark's positional complex-type merging, and DataFusion 55.1.0 source contracts. Traced case-variant names, heterogeneous field positions, nested struct/list/map nullability, omitted ELSE, typed NULLs and unreachable branches. The selected target's guarded Scala-UDF IF dispatch retains the native conversion path, and its protobuf delta changes only a datetime comment. Independent validation was source inspection and Git-object/UTF-8/hash verification. No tests, builds, runtime probes or benchmarks were run. Supplied head CI status metadata is pending with zero status entries, which does not establish passing checks. No CI logs or artifacts were inspected.
• Key design decisions: Spark has already resolved the logical branch types, so native reconciliation must preserve positions while making Arrow schemas agree. Casting every differing branch prevents the all-THEN and all-ELSE paths from returning an array with another branch's field names or nullability. The extracted IF helper preserves its existing behavior. Schema merging still includes branches that remain in the expression even when no row selects them. This does not change when their values are evaluated.
• Implementation sketch: The production change is confined to CASE's type fold and the shared helper's name. Evaluation and cast kernels are unchanged. Existing Rust tests move to case_when/tests.rs, with the struct checks extended to CASE as well as IF. The SQL fixture uses column predicates, native CASE assertions and JSON output to expose names that ordinary row comparison can miss. The helper adds recursive type construction during planning, proportional to the branch count and nested schema size. Equal branch types still avoid runtime cast wrappers. No measured performance claim is made, and no P1/P2 performance or abstraction issue was established.
• Behavioral changes worth calling out: Later THEN and ELSE values now carry the first surviving THEN's field names, including when all rows choose a later branch. Positional INT/DOUBLE fields keep their positions and intended types. Missing ELSE still produces NULL for unmatched rows. The synthetic mixed-timezone-label test now prefers a present first-THEN label, while the ordinary serialized timestamp representation remains UTC. Nested containers were traced through source but are not added as dedicated execution cases in this PR, and none of the tests was executed in this review.
• Suggested improvements: None at the requested P1/P2 threshold.

[andygrove]

cc @andygrove 🙏

I am reviewing now

[sunchao]

Reviewed the full three-file diff at 04f328ccbae5888baf7f5a36ec037eca954ce6de against 00a4b422f0ed6560ef76dce004b94c3697613108. The PR is not a draft. Read the existing reviews and discussion before forming conclusions. No unresolved existing P1/P2 concern was substantiated.

Routed skills: review-comet-pr and review-comet-expression-pr, with the expression, testing, timezone, development and CI guides.

Summary

• Prior state and problem: Native CASE used ELSE-first DataFusion coercion. This could choose ELSE’s struct field spelling and match fields by name rather than position, producing incorrect native to_json output or unwanted field-type widening.
• Design approach: Fold the first THEN type through subsequent THEN types, then ELSE. Reuse IF’s recursive positional merge as merge_branch_types, preserving left-side field names and combining nested nullability.
• Correctness: Checked Spark’s CaseWhen, ComplexTypeMergingExpression and type-coercion source. Traced omitted ELSE, typed NULLs, multiple branches, nested structs/lists/maps and batches selecting only one branch. Existing casts reconcile complete Arrow types before evaluation. No introduced P1/P2 correctness issue was identified.
• Compatibility analysis: Verified the relevant ordering and positional-merge semantics across Spark 3.4.3, 3.5.9, 4.0.4, 4.1.3 and 4.2.0. No shim, protobuf, configuration or support-level change is needed. Normal timestamp inputs retain the existing UTC/NTZ representation rules.
• Key design decisions: Derive the common schema from every branch while retaining existing short-circuit evaluation. Keep casts whenever complete Arrow types differ, including field names and nullability, so selecting a later branch cannot return a conflicting schema.
• Implementation sketch: Production changes are confined to CASE’s type fold and the shared helper rename. IF’s merge logic is unchanged. Existing Rust tests move into case_when/tests.rs, with CASE added to struct regressions. The SQL fixture exposes naming differences through to_json and asserts native CASE execution.
• Performance: Recursive schema reconciliation happens during expression construction. Equal-type branches still avoid cast wrappers, and evaluation kernels are unchanged. No benchmarks were run and no evidence-supported P1/P2 performance regression was identified.
• Design: Correcting schema reconciliation gives downstream consumers consistent field names and types. The change stays focused on the reported defect without introducing a separate execution path.
• Abstraction & complexity: Sharing the existing IF helper avoids duplicate merging rules. Its recursive struct/list/map handling matches the nested schema problem, and the test extraction does not add production complexity.
• Behavioral changes worth calling out: Later THEN and ELSE values now use the first THEN’s field spelling. Positional INT/DOUBLE fields retain their intended types. CASE-backed COALESCE also receives first-child schema precedence. Compared with branch-1.1 at e9efd9f764ee0a59b7898ff028d6985d4a7a28e1, this is an intended Spark-compatibility correction. The synthetic mixed-timezone test now prefers a present first-THEN label.
• Suggested improvements: The JSON and complete-Arrow-type assertions directly exercise the defect. No additional code change meeting the P1/P2 reporting bar was identified.

Exact-head CI: run 37975490027 passed Required Checks and the executed Linux checks. Its merge commit’s tree exactly matches the reviewed head. Logs confirm 2,206 Rust tests passed and 2,210 expression tests passed, including the new SQL fixture and extended Rust regressions. The expression run also recorded one canceled and 12 ignored tests.

Local validation: all 16 focused CASE/IF Rust tests passed. git diff --check passed and the checkout remains unchanged. No local JVM suites or benchmarks were run. Spark’s own SQL suites were skipped in CI, and nondefault Spark runtime coverage remains unverified. The repository’s requested Spark SQL verdict is therefore still outstanding.

No introduced P1/P2 issues found within this review.

[andygrove]

Thanks for the fix. The fold now matches Spark's ComplexTypeMergingExpression.dataType, and reusing IF's positional merge keeps one set of rules for both.

I ran this locally. The new SQL fixture fails on main (Comet returns {"X":1} where Spark returns {"x":1}) and passes here on Spark 4.1 and 3.5. The conditional_funcs Rust tests pass. I also compared the old and new common type across about 40 Arrow types and found no reachable difference besides the field names.

I have three asks, each in an inline comment: a COALESCE query, two more shapes for the fixture, and a question about moving the tests. The Spark SQL suites have not run on this PR yet.

[andygrove]

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.

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

[andygrove]

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.

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

[andygrove]

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?