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.
• Extend the existing inline Rust 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, nested structs, a typed-NULL first THEN, multiple WHEN branches, omitted ELSE, and positional INT/DOUBLE struct fields. A separate COALESCE fixture checks the same field-name rule through its CASE WHEN lowering. 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 of the review follow-ups is pending CI. A maintainer needs to add run-spark-4.1-tests for Spark SQL coverage before merging; 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.

[pingzh]

@andygrove updated, could you please check again? 🙏

[sunchao]

Re-reviewed fb8f70595202fe352fb89d14c2f8204a1581770b against actual merge base 00a4b422f0ed6560ef76dce004b94c3697613108, including source integration with main at 79dd56f3660e4016e290b05635670f1f1033cca2. No introduced or materially worsened P1/P2 finding was established.

The follow-up restores the Rust tests inline and adds nested-struct, typed-NULL-first-THEN and COALESCE coverage; its production code matches the previously reviewed head. Independently checked first-surviving-THEN field-name precedence, positional nested types and nullability, and casts before mixed or single-branch batch returns. The review requests are addressed in source. Dependency checks use the locked Arrow 59.3.0 source and exact upstream DataFusion 55.1.0/55.2.0 tags; their relevant CASE/type-coercion files match.

Validation was immutable source inspection and Git-object/hash verification. No tests, builds, runtime probes or benchmarks were run. Spark comparison covered the inspected 4.1.3 and 3.5.9 sources. Commit-status metadata reports pending with zero entries, which does not establish passing checks; CI jobs/logs/artifacts and the repository-requested Spark SQL runtime validation were not verified. No additional performance or design issue met the P1/P2 reporting threshold.

[andygrove]

Thanks for turning these around so quickly. All three are addressed.

I merged this branch into current main and ran both fixtures. They pass on Spark 3.4, 4.1 and 4.2. With main's case_when.rs, every query in them fails, including the new COALESCE one, which returns {"X":102} where Spark returns {"x":102}. Moving the tests back inline also clears the case_when.rs conflicts with #6716 and #6763.

I added the run-spark-4.1-tests label so the Spark SQL suite also runs here, ahead of the merge queue's own run.