Repository navigation
test: cover positional struct reconciliation in native CASE WHEN and COALESCE - #7149
kazuyukitanimura wants to merge 1 commit into
Conversation
sunchao
left a comment
There was a problem hiding this comment.
Reviewed the full diff from da2ff08134ba6ceeccade058aea09e7c21d9472b to 6e865aef7f7b76388e6ece0cfdb2c25a160d2572: one commit, three files, 315 added lines. The PR is not a draft. The snapshot and live GitHub discussion contain no reviews, conversation comments, or review threads.
Routed skills: .ai/skills/review-comet-pr/SKILL.md and .ai/skills/review-comet-expression-pr/SKILL.md.
Summary
- Prior state and problem: The positional reconciliation fix already exists in the base. Coverage remained incomplete for reversed branch order,
COALESCE, and nested containers. The regression being guarded against pairs struct fields by name and can widen anINTtoDOUBLE. - Design approach: Add complementary Rust type/value assertions and Spark-versus-Comet SQL comparisons. The Rust test checks Arrow representation directly, while
to_jsonmakes field names and numeric representation observable in SQL results. - Correctness: Checked Spark’s
findTypeForComplex,ComplexTypeMergingExpression,CaseWhen,Coalesce, and conditional-expression tests. Spark pairs struct fields positionally, retains the first branch’s names, and merges nullability. The new expectations match those rules. The existingevaluate_conditionalhelper also checks result types for mixed batches and individual row slices. No correctness issue meeting the P1/P2 bar was identified. - Compatibility analysis: Compared the relevant Spark sources at 3.4.3, 3.5.9, 4.0.4, 4.1.3, and 4.2.0. Positional complex-type reconciliation, left-to-right type merging, and the relevant branch-selection behavior agree across these versions. The fixtures explicitly use case-insensitive resolution and retain the existing opt-in for native
StructsToJson. - Key design decisions:
expect_native(casewhen)andexpect_native(coalesce)verify the intended execution mechanism. Extracting structs from arrays and maps beforeto_jsonkeeps the assertions within its supported input shape. Reversing branches verifies that output names follow argument position. - Implementation sketch: A test-local
Layerenum andnesthelper construct six nesting shapes. Both branch orders run throughcreate_if_exprandcreate_case_when, including theIS NOT NULLshape used byCOALESCE. Ten added SQL queries cover multiple branches, nulls, arrays, maps, and deeper nesting. - Performance: There is no production-code change or added query-runtime overhead. Exact-head CI reports approximately 0.96 seconds for the complete
coalesce_field_names.sqlfixture and 1.19 seconds forcase_when_field_names.sql. These are whole-fixture timings, not incremental benchmarks. - Design: The two testing layers address different failure modes without changing execution architecture. Direct Arrow assertions protect schema details that ordinary row comparisons can miss, and SQL comparisons check integration with Spark planning.
- Abstraction & complexity: The small, test-local nesting helper avoids repeating Arrow construction for every shape. It introduces no production abstraction or dependency. No complexity concern meets the reporting bar.
- Behavioral changes worth calling out: Compared with
branch-1.1at89c729a9697467e8f8d89e3eadf1797a75e2328c, the relevant CASE/COALESCE path has already moved from DataFusion’s common-type calculation to positional reconciliation in the supplied base. That intended correction predates this PR. This PR adds regression coverage without another runtime, configuration, or support-level change. - Suggested improvements: No additional change request meets the P1/P2 reporting bar.
Exact-head CI: 26 successful checks and 18 skipped checks, with no failures. Logs confirm the new Rust test and both modified SQL fixtures passed. The Spark 4.1/JDK 17 expression job completed 2,263 tests successfully. Required Checks, the native build, and the other enabled Linux test groups also passed. See the CI run.
Local validation: cargo test --locked --offline -p datafusion-comet-spark-expr --lib conditional_funcs passed all 30 tests. cargo fmt --check and the base-relative whitespace check passed. JVM tests were not rerun locally. SQL execution evidence is from exact-head Spark 4.1 CI. Other supported Spark versions were source-checked only, and the upstream Spark SQL, Iceberg, and macOS jobs were skipped.
No introduced P1/P2 issues found within this review. No substantiated existing blockers remain.
Which issue does this PR close?
Closes #6532.
Rationale for this change
#6532 reported that native
CASE WHENandCOALESCEreconcile struct fields by name, where Spark reconciles them by position and keeps the first branch's field names. With case-insensitive resolution, case-distinct names swapped by position could make the native common type pair anINTfield with aDOUBLEfield and widen theINTtoDOUBLE. Nativeto_jsonthen showed{"x":7.0,"X":5.5}instead of{"x":7,"X":5.5}.The behavior fix has already landed. #6827 changed
create_case_whento use the positionalmerge_branch_typeshelper that #6458 added forIF.COALESCEis planned asCASE WHEN x IS NOT NULL THEN x … ELSE last, so it goes through the same path. What remained from the issue was the regression coverage it asks for:CASE WHENwas coveredfor one branch order only, and
COALESCEand nested array and map cases were not covered.What changes are included in this PR?
Tests only, with no change to production code.
case_when.rs: new unit testconditional_branches_reconcile_nested_case_variant_fields_positionally.array<struct>,map<string, struct>, and three deeper nestings (a struct holding a list, a list of maps, and a map holding a struct).create_if_expr,create_case_when, and theCASE WHEN x IS NOT NULLshape thatCOALESCEbecomes.INTis not widened toDOUBLE.case_when_field_names.sqlandcoalesce_field_names.sql: new SQL file tests, one table each with batches where every row takes THEN, every row takes ELSE, and rows are mixed.WHENbranches / severalCOALESCEarguments.[0]/['k']because nativeto_jsondoes not accept arrays or maps.How are these changes tested?
cargo test -p datafusion-comet-spark-expr --lib conditional_funcspasses (30 of 30). With the positional struct merge inmerge_branch_typesdisabled, the new test and the existing positional test both fail, so the test does catch the bug.cargo fmt --checkis clean.