Repository navigation
Conversation
154dc9b to
e7e492a
Compare
sunchao
left a comment
There was a problem hiding this comment.
Summary
- Prior state and problem: Native nested types could disagree with Catalyst’s nullability, causing valid struct comparisons to fail.
- Design approach: Carry Catalyst field-nullability flags into
named_structand reconcile compatible nested types by position. - Correctness / compatibility analysis: Reviewed both commits and all six changed files against
bd1d4d113cd4524b44927d59f10adc5791997a47. Found one P2 issue: the new comparison fixture is invalid on supported Spark 3.x versions. - Key design decisions: Reuse Spark-compatible casts, preserve positional values, and skip unnecessary casts for identical types. No demonstrated performance regression was found; benchmarks were not run.
- Implementation sketch: Extend protobuf/Scala serialization, validate constructor metadata, add planner reconciliation helpers, and exercise construction and conditional results.
- Behavioral changes worth calling out: Nested comparison operands and conditional branches receive consistent types, including uniform
IFbatches. - Suggested improvements: Move the differently named struct comparison into a separate fixture with
-- MinSparkVersion: 4.0, retaining older-version coverage for the other queries.
Reviewed SHA: fc228046151444b18809b427d2355c4efebfc8f1. Routed skills: review-comet-pr and review-comet-expression-pr. Existing reviews, comments, and threads were empty.
Exact-head CI: 35 successful checks and 15 skipped. Rust tests, Linux Comet suites, and all nine Spark 4.1 SQL shards passed. Other Spark SQL versions and macOS were skipped.
Validation: Four constructor tests and a focused native positional-comparison/IF probe passed locally. Running all six fixture queries on Spark 3.5.9 reproduced the fifth query’s analysis failure; the other five passed. Spark sources confirm the version boundary. Full Comet JVM and multi-version suites were not rerun locally. Disposable project tests were removed.
fc22804 to
8580cb0
Compare
|
Additional validation on current head |
sunchao
left a comment
There was a problem hiding this comment.
Summary
- Prior state and problem: Native struct nullability and name-based coercion could make valid Spark expressions fail or compare the wrong positions.
- Design approach: Serialize Catalyst nullability and align compatible nested types positionally using Spark-compatible casts.
- Correctness / compatibility analysis: Found one introduced P2 regression in Spark 4.x comparisons. The previously reported Spark 3.x fixture issue is resolved.
- Key design decisions: Preserve physical child types, union nested nullability, and retain existing IF/CASE evaluators. No substantiated performance regression was found. Benchmarks were not run.
- Implementation sketch: Extend protobuf and Scala serialization, validate constructor metadata, add planner reconciliation, and cover construction and conditional results.
- Behavioral changes worth calling out: Compared with
branch-1.1, the intended changes preserve declared nullability and positional values. The comparison failure below is unintended. Relevant release constructor and reconciliation code match the supplied base. - Suggested improvements: Fix the name-insensitive early-return guard and add a comparison regression with equal nullability on both operands. Request changes for this P2.
Reviewed full SHA: 8580cb00400d839e39389ee241bd0bd076aec0bf, against base ac9ae94d057013095fdf1b70e2d4bc5b24a58d90. Reviewed all three PR commits and seven PR-changed files. Apparent floating-point removals in the direct tree comparison belong to base-only commit #6457, not this PR.
Routed skills: review-comet-pr and review-comet-expression-pr. Read existing reviews, comments, and threads.
Exact-head CI: 53 successful checks, 15 skipped, no failures. Linux Comet suites passed across all supported Spark profiles, alongside Rust tests and all nine Spark 4.1 SQL shards. Other Spark SQL versions and macOS were skipped.
Validation: Four constructor tests passed. An isolated native probe reproduced the regression against the base and verified the guard correction. Spark 4.1.3 Catalyst analysis and evaluation confirmed the expected result. Full local JVM/SQL execution was limited by missing runtime dependencies. Disposable project tests were removed, and the checkout is clean.
sunchao
left a comment
There was a problem hiding this comment.
Summary
- Prior state and problem: Native struct nullability and name-based coercion could reject valid Spark expressions or compare the wrong field positions.
- Design approach: Serialize Catalyst’s field nullability and align compatible nested comparison operands and CASE branches positionally.
- Correctness / compatibility analysis: No introduced P1/P2 issues found within this review. Both previously reported P2 issues are resolved. Checked relevant Spark sources across supported 3.4–4.2 versions.
- Key design decisions: Preserve physical child types, combine nullability by position, use exact type equality, and retain existing conditional evaluators. No substantiated performance regression found. Benchmarks were not run.
- Implementation sketch: Extend protobuf/Scala serialization, validate constructor metadata, add planner reconciliation helpers, and cover nested construction and comparisons.
- Behavioral changes worth calling out: Compared with
branch-1.1, the intended changes preserve Catalyst nullability and positional values. Differently named comparison fixtures are correctly gated to Spark 4.x. - Suggested improvements: None meeting the P1/P2 reporting bar.
Reviewed full SHA: 4652ca226499bb0879c48d78fd87353ce94bd47e, against supplied base ac9ae94d057013095fdf1b70e2d4bc5b24a58d90. Reviewed all four commits and seven files in the full PR diff. Apparent floating-point removals in the direct tree comparison belong to base-only commit #6457.
Routed skills: review-comet-pr and review-comet-expression-pr. Read existing reviews, issue comments, inline comments, and threads. The PR remains open and non-draft.
Exact-head CI: 39 successful checks, 13 skipped, 11 still running, no reported failures. Rust CI passed 2,043 tests, including the new planner and constructor tests. Three expression profiles and eight Spark 4.1 SQL shards remain pending, so CI has no final verdict yet.
Validation: Four constructor tests and three focused native probes passed locally. Probe helpers were verified against exact-head source. Five ungated queries passed on Spark 3.5.9. Spark 4.1.3 Catalyst checks passed for positional struct/array comparisons and IF/CASE nullability. Full local Comet JVM/Spark SQL suites were not run. Spark-only checks do not validate JNI integration. Disposable project tests were removed and the checkout is clean.
|
reviewing now |
sunchao
left a comment
There was a problem hiding this comment.
Summary
- Prior state and problem: Native struct nullability could disagree with Catalyst, making computed structs incompatible with typed nulls. Name-based coercion could also reject or reorder positional fields.
- Design approach: Serialize Catalyst’s field-nullability flags and reconcile compatible nested comparison operands and
CASEbranches by position. - Correctness: Checked Spark’s constructor, comparison, conditional evaluation and nullability-merging sources across supported 3.4–4.2 versions. Focused validation preserved field positions, null structs and conditional result types. No introduced P1/P2 issues found within this review.
- Compatibility analysis: Spark 3.x requires matching comparison field names, while Spark 4.x permits structural comparison. The separate positional-comparison fixture correctly uses
MinSparkVersion: 4.0. Scala, protobuf and Rust agree on one nullability flag per struct value. - Key design decisions: Preserve physical child types, union nested nullability, retain left-side field names and use exact type equality before skipping alignment. Both previously reported P2 issues are resolved.
- Implementation sketch: Extend
CreateNamedStructmetadata, validate constructor lengths, retain metadata during expression rewriting, add positional reconciliation helpers and exercise construction, comparisons and conditionals. - Performance: Type reconciliation adds planning work and casts only differing types. Metadata alignment reuses underlying value buffers. No evidence-backed performance regression was identified. Benchmarks were not run.
- Design: The fix follows Catalyst’s type decisions and reuses existing Spark-compatible casts and conditional evaluators. It does not introduce another evaluation engine or change branch-selection rules.
- Abstraction & complexity: The helpers separate common-type calculation from casting. The
CASEwrapper limits special handling to compatible nested layouts and preserves general coercion otherwise. No complexity concern met the P1/P2 bar. - Behavioral changes worth calling out: Computed structs retain declared nullability, and nested alignment preserves ordinal values. Compared with
branch-1.1ate9efd9f764ee0a59b7898ff028d6985d4a7a28e1, these are intended fixes. The head also inherits main’s newer conditional evaluator. - Suggested improvements: None meeting the P1/P2 reporting bar. No substantiated existing blocker remains unresolved.
Reviewed full SHA: 4652ca226499bb0879c48d78fd87353ce94bd47e, against supplied base ac9ae94d057013095fdf1b70e2d4bc5b24a58d90. Reviewed all four PR commits and seven files in the full merge-base-relative PR diff. Apparent floating-point removals in the direct tree comparison belong to base-only commit #6457. The PR remains open and non-draft.
Routed skills: review-comet-pr and review-comet-expression-pr. Read repository guidance and existing reviews, issue comments, inline comments and review threads.
Exact-head CI: 52 successful checks, 15 skipped, no failures or pending checks. Linux Comet suites passed across Spark 3.4–4.2, all nine Spark 4.1 SQL shards passed, and Rust logs confirm 2,043 passing tests. macOS, other versions’ full Spark SQL suites, Iceberg suites and benchmarks were skipped.
Validation: Four constructor tests and three disposable native probes passed locally. Probe helpers were verified against exact-head source. All five common fixture queries passed on Spark 3.5.9, and Spark 4.1.3 Catalyst checks passed for positional struct/array comparisons and conditional nullability. An initial stale native-cache build failure was resolved by rebuilding local dependencies. Full Comet JVM/JNI integration suites were not rerun locally. Disposable tests were removed and the checkout is clean.
|
Oh this is nice PR |
andygrove
left a comment
There was a problem hiding this comment.
This fixes a silent wrong result that ships in 1.0 and 1.1. On main, named_struct('x', CAST(id AS DOUBLE), 'y', 1D) = named_struct('y', 1D, 'x', CAST(id AS DOUBLE)) returns true for every row on Spark 4.x, because the old reconcile path casts with arrow's CastExpr, which matches struct fields by name. With this PR merged into current main (including #6447) the new fixtures, the struct and conditional SQL fixtures, and a set of comparison, CASE, IF, array, union and aggregate probes all match Spark on 4.1. A few comments inline, mostly about sharing the positional merge with the IF path from #6458.
One more that I couldn't leave inline because the file isn't in the diff. The doc on expandComplexLiteral in literals.scala says the CreateNamedStruct proto "carries no type, so Spark's declared field nullability cannot survive the wire either way". With field_nullable that no longer holds for the struct's own fields. It's one of the two reasons given for declining arrays of structs, so could you update it here so the decline isn't kept for a reason that's gone?
|
|
||
| /// Merge compatible nested types by ordinal, retaining the left field names and physical | ||
| /// leaf types. Return None when a metadata-only Spark cast cannot safely align the layouts. | ||
| fn positional_nullability_union(left: &DataType, right: &DataType) -> Option<DataType> { |
There was a problem hiding this comment.
This is the same positional merge that #6458 added for IF as if_common_type and coerce_branch in spark-expr/src/conditional_funcs/case_when.rs, and the two copies already disagree. This one refuses dictionaries because Comet's cast rejects a dictionary target, but if_common_type passes them to type_union_coercion, so an IF over the same structs would still hit that cast error. They also differ on the map sort flag (equal here, AND there), only if_common_type widens differing leaf types through type_union_coercion, and the cast here has no timezone while coerce_branch passes "UTC" and explains that Comet's cast needs one for timestamps. Could we keep one helper in case_when.rs and use it for IF, for CASE inside create_case_when, and for reconcile_nested_comparison_types? Then create_case_expr could go away, so CASE isn't coerced in two layers, and the comment that refers to "main's" evaluator goes with it.
There was a problem hiding this comment.
Consolidated this in 6d8a0a9. positional_common_type and cast_to_common_type now live in case_when.rs and serve IF, CASE, comparisons, and IN. The planner's create_case_expr wrapper is gone. Conditionals retain leaf-type promotion and combine map sort flags, while comparisons and IN only align compatible metadata. Both use the same dictionary-target checks and UTC cast options.
Final validation after merging current main in ba8f2a5: 20 native conditional tests and four constructor tests pass; all 29 conditional/struct SQL-file cases pass on Spark 4.0.4. Focused Clippy, Rust formatting, Spotless, and Scalastyle pass. Local Spark 3.5/4.1 tests are dependency-DNS blocked; hosted CI has started.
|
|
||
| -- Equal nullability must not bypass field-name alignment. Native Range makes every field | ||
| -- non-nullable; id=1 compares equal, while id=2 detects an accidental name-based reorder. | ||
| query expect_native(equalto) |
There was a problem hiding this comment.
The reconcile change covers all eight comparison operators, but this file only exercises =. Since #6447, = and <> on float fields go through NestedPredicate, while <, <=, >, >= and <=> compare normalized operands in a BinaryExpr, so they take a different path after the cast. Could we add a < and a <=> query with the same swapped names? On main today named_struct('x', CAST(id AS DOUBLE), 'y', 1D) < named_struct('y', 1D, 'x', CAST(id AS DOUBLE)) returns false for id=0 where Spark returns true, and with this PR it matches.
There was a problem hiding this comment.
I can confirm both on the merge base 98662215d with Spark 4.1. named_struct('x', CAST(id AS DOUBLE), 'y', 1D) < named_struct('y', 1D, 'x', CAST(id AS DOUBLE)) and the same query with <=> both return a wrong row on main. With this PR both match Spark, so adding them to the fixture would lock that in.
There was a problem hiding this comment.
Added < and <=> alongside = in 6d8a0a9, with explicit native-expression assertions. The computed-struct query includes id=0 to catch the wrong ordering result you identified. The fixture also covers Parquet columns, so it exercises the BinaryExpr path as well as nested floating-point equality.
Both isolated queries return wrong results with the merge-base native library and match Spark with this revision.
Final validation after merging current main in ba8f2a5: 20 native conditional tests and four constructor tests pass; all 29 conditional/struct SQL-file cases pass on Spark 4.0.4. Focused Clippy, Rust formatting, Spotless, and Scalastyle pass. Local Spark 3.5/4.1 tests are dependency-DNS blocked; hosted CI has started.
|
might be related to apache/iceberg-rust#3255 |
|
I checked, and this doesn't overlap with #6725. This PR changes native expression planning ( |
viirya
left a comment
There was a problem hiding this comment.
Thanks for this. The positional alignment fixes real wrong results. I ran the new fixtures plus extra comparison and CASE probes against both the merge base and this head on Spark 4.1. = with swapped field names errors on main, < and <=> return wrong rows on main, and a CASE over map values with case-variant struct names panics on main. All of them match Spark with this PR. A few comments inline, mostly about the field_nullable half and test coverage.
| structBuilder.addAllValues(valExprs.map(_.get).asJava) | ||
| structBuilder.addAllNames(expr.names.map(_.toString).asJava) | ||
| // Native expressions can conservatively report nullable even when Catalyst proves otherwise. | ||
| structBuilder.addAllFieldNullable(expr.valExprs.map(v => Boolean.box(v.nullable)).asJava) |
There was a problem hiding this comment.
Could you share a query that fails on main without the field_nullable change? All five queries in create_named_struct_nullability.sql pass natively on the merge base 98662215d, including the array(named_struct(...), NULL) example from the description. CometCreateArray already casts every element to deepNullable(...) (#5452), so make_array never sees the mismatch.
I also ran this PR with CreateNamedStruct::fields reverted to expr.nullable(schema). Both new fixtures and a set of comparison, CASE, IF, array and aggregate probes still matched Spark. Only test_create_struct_preserves_catalyst_nullability_in_array failed, and it calls array_array directly without that cast.
The change does add a failure mode. A native child that emits a null where Catalyst proves non-null now fails the task in StructArray::try_new with Found unmasked nulls for non-nullable StructArray field. Unless there is a query that needs it, could this half be dropped or moved to its own PR with a test that fails without it? If it stays, the expandComplexLiteral doc @andygrove mentioned needs updating too.
There was a problem hiding this comment.
You're right about the original example: CometCreateArray already normalizes it, and the direct array_array test did not establish a SQL regression. I replaced those fixtures with this path, which bypasses CreateArray:
SELECT array_union(
array_repeat(named_struct('x', CAST(id AS INT)), 2),
array_repeat(named_struct('x', IF(id = 0, 0, 1)), 2))
FROM range(3)On Spark 4.0.4 with ordinary constant folding and native Range, the PR matches Spark. Changing only CreateNamedStruct::fields back to expr.nullable(schema) fails in array_union because the first struct field is nullable and the second is not. The revised SQL covers this and its nested-struct variant. 6d8a0a9 retains field_nullable, makes the unit test check the constructor contract directly, corrects the PR rationale, and updates the literal-expansion docs.
Final validation after merging current main in ba8f2a5: 20 native conditional tests and four constructor tests pass; all 29 conditional/struct SQL-file cases pass on Spark 4.0.4. Focused Clippy, Rust formatting, Spotless, and Scalastyle pass. Local Spark 3.5/4.1 tests are dependency-DNS blocked; hosted CI has started.
|
|
||
| -- CASE unions nullability by ordinal, even when case-insensitive names swap places. | ||
| query | ||
| SELECT CASE WHEN id % 2 = 0 |
There was a problem hiding this comment.
The two CASE queries here also pass on main, so they don't guard the new create_case_expr alignment. This query panics on main with Found unmasked nulls for non-nullable StructArray field "x" and matches Spark with this PR. It is valid on Spark 3.x as well with spark.sql.caseSensitive=false:
SELECT CASE WHEN id % 2 = 0
THEN map(1, named_struct('x', CAST(id AS DOUBLE), 'X', CAST(NULL AS DOUBLE)))
ELSE map(1, named_struct('X', CAST(NULL AS DOUBLE), 'x', CAST(id AS DOUBLE)))
END FROM range(4)Could you add it, or something like it, alongside the existing ones?
There was a problem hiding this comment.
Added your map-valued CASE reproducer in 6d8a0a9, plus a variant with an untyped NULL branch. These replace the two weaker struct-only CASE queries. They remain ungated for Spark 3.x and run with case sensitivity disabled.
The isolated CASE query fails Arrow validation with the merge-base native library and matches Spark with this revision.
Final validation after merging current main in ba8f2a5: 20 native conditional tests and four constructor tests pass; all 29 conditional/struct SQL-file cases pass on Spark 4.0.4. Focused Clippy, Rust formatting, Spotless, and Scalastyle pass. Local Spark 3.5/4.1 tests are dependency-DNS blocked; hosted CI has started.
| } | ||
| } | ||
|
|
||
| /// DataFusion's nested comparison kernel (`apply_cmp_for_nested`) requires both operands to |
There was a problem hiding this comment.
The doc says apply_cmp_for_nested requires identical types including nested field names and nullability. In DataFusion 55 it checks equals_datatype, which ignores both. The constraints that actually bite are NestedPredicate's validate_types for =/<>, which compares names through datatype_is_logically_equal, and the name-matching arrow CastExpr in the old fallback. On main, < and <=> returned wrong rows only because that cast reordered the fields. Could the comment describe those two? Otherwise the next reader will "fix" the wrong thing.
There was a problem hiding this comment.
Corrected the comment in 6d8a0a9 to distinguish the name-sensitive nested equality validator from DataFusion's comparison kernel and to explain the old Arrow cast's positional reordering. One detail from checking the pinned Arrow implementation: equals_datatype ignores struct field names but does compare nested nullability. The new wording preserves that distinction.
| /// them without changing field positions. Exact equality is required here: Arrow's | ||
| /// `equals_datatype` ignores nested field names, even when nullability already matches. | ||
| /// Non-comparison ops and non-nested or already-matching types are left untouched. | ||
| pub fn reconcile_nested_comparison_types( |
There was a problem hiding this comment.
IN has the same shape and is not covered here. Spark leaves named_struct('x', id) IN (named_struct('y', id)) uncast, because findTypeForComplex only unifies struct names that match under the resolver, and In.checkInputDataTypes accepts it in 3.5.9 and 4.0.2 through equalsStructurally(..., ignoreNullability = true), comparing the fields by position. Comet sends it to spark_in_list without this reconcile step. From reading the code I expect it to fail, in validate_types when a leaf is a float and otherwise in DataFusion's InListExpr type check. Both use datatype_is_logically_equal, which compares struct field names. I have not run it. Should this PR route IN through the same alignment, or should a follow-up issue track it?
There was a problem hiding this comment.
Confirmed the failure on both the merge-base native library and the previous PR head and included the fix in 6d8a0a9. The planner now computes one common positional type across the IN value and every candidate before calling spark_in_list. A new SQL fixture covers integer and floating-point paths, multiple candidates, NOT IN, nested structs/lists, and NULL candidates. It is ungated because Spark 3.x also accepts differently named structs in IN.
Final validation after merging current main in ba8f2a5: 20 native conditional tests and four constructor tests pass; all 29 conditional/struct SQL-file cases pass on Spark 4.0.4. Focused Clippy, Rust formatting, Spotless, and Scalastyle pass. Local Spark 3.5/4.1 tests are dependency-DNS blocked; hosted CI has started.
| if matches!( | ||
| lt, | ||
| DataType::Struct(_) | DataType::List(_) | DataType::Map(_, _) | ||
| ) { | ||
| return (left, right); | ||
| } |
There was a problem hiding this comment.
With this early return, every Struct, List and Map pair is either aligned above or left alone, so the Field::try_merge and CastExpr code below only serves LargeList and FixedSizeList. Comet maps Spark arrays to List only (serde.rs), and the list-building kernels I checked (flatten, shuffle, array_insert) keep their input's offset width, so I could not find a path from a Spark array to either type. If there is none, the tail and the two matching arms of nested can go, leaving one reconcile path. If there is one, the tail's CastExpr matches struct fields by name, which is the behavior this PR replaces, so it would need the positional merge too.
There was a problem hiding this comment.
Removed the old Field::try_merge/CastExpr tail and the LargeList/FixedSizeList admission arms in 6d8a0a9. Spark's supported nested representations now go through the shared positional helper, with incompatible physical layouts left unchanged. There is no remaining name-based fallback in this comparison reconciliation.
| use datafusion_comet_spark_expr::EvalMode; | ||
|
|
||
| #[test] | ||
| fn test_if_reconciles_nested_branch_nullability() { |
There was a problem hiding this comment.
This test only goes through ExprStruct::If and create_if_expr, which this PR does not change, and it feeds bound columns directly, so none of this PR's code is on its path. #6458 already covers these shapes: if_reconciles_struct_field_nullability_and_names and if_reconciles_case_variant_fields_positionally in case_when.rs, and conditional/if_nested_nullability.sql, whose INSERT batches give uniform THEN, uniform ELSE, NULL and mixed predicates over struct, array, map and nested containers, including the swapped x/X case through to_json. I expect this test passes on the PR's base too, but I have not run it. Could it be dropped (about 130 lines)? The last query in create_named_struct_nullability.sql (three IFs over a typed NULL branch) is on the same unchanged path. Any missing shape would fit in the existing fixture.
There was a problem hiding this comment.
Removed the duplicate planner IF test and the three-IF SQL query in 6d8a0a9. The existing IF tests remain the coverage for those shapes. The revised fixture focuses on constructor array_union and map-valued CASE regressions, and the shared-helper tests cover the coercion distinctions introduced by the consolidation.
| -- Config: spark.comet.exec.range.enabled=true | ||
| -- Config: spark.comet.sparkToColumnar.enabled=true | ||
| -- Config: spark.comet.sparkToColumnar.supportedOperatorList=Range | ||
| -- Config: spark.sql.caseSensitive=false |
There was a problem hiding this comment.
exec.range.enabled=true already makes CometRangeExec the leaf here. CometExecRule prefers a leaf's enabled Comet operator and keeps the Spark-to-Arrow conversion only as the fallback for a range the operator declines, so the two sparkToColumnar lines do nothing for these ranges. No query in this file uses case-variant names, so caseSensitive=false does nothing either. Dropping all three would also make the file fail loudly if native Range ever declined these ranges, instead of silently running the equal-nullability premise on a different leaf. In create_named_struct_nullability.sql, supportedOperatorList=Range repeats the default (Range,InMemoryTableScan,RDDScan,OneRowRelation).
There was a problem hiding this comment.
Removed the redundant Spark-to-columnar settings and case-sensitivity setting from the positional comparison fixture in 6d8a0a9. Both struct fixtures now request native Range directly. The constructor/CASE fixture retains only caseSensitive=false in addition to native Range because its CASE regression deliberately swaps x/X names.
| -- Spark 4.x compares struct fields by position, irrespective of field names. | ||
| query | ||
| SELECT named_struct('x', CAST(id AS DOUBLE), 'y', CAST(NULL AS DOUBLE)) = | ||
| named_struct('y', CAST(NULL AS DOUBLE), 'x', CAST(id AS DOUBLE)) | ||
| FROM range(8) |
There was a problem hiding this comment.
Every operand in this file comes from range or a literal, so no compared struct is NULL, no leaf is NaN or -0.0, and nothing is read from a Parquet table. A table with a struct<x: double, y: double> and b struct<y: double, x: double> holding a NULL struct, a NULL leaf, NaN and -0.0 would cover the equal-nullability, swapped-name case from the last commit on scan-produced types, and nested = on float leaves has its own path that this would reach. The < and <=> queries requested on this file could run over the same rows.
There was a problem hiding this comment.
Added a Parquet table in 6d8a0a9 with swapped struct names, distinct field values, NULL structs, NULL leaves, NaN, and -0.0 versus +0.0. A native query checks =, <, and <=> against Spark for all nine rows.
The first run exposed the existing nested signed-zero gap #6157. I subsequently merged current main (e76c35f), including its independent fix #6447, and restored the signed-zero row to all three comparisons. Nothing is excluded from the final fixture.
Final validation at ba8f2a5: all 29 conditional/struct SQL-file cases pass on Spark 4.0.4, including this full Parquet fixture. The 20 native conditional and four constructor tests, focused Clippy, Rust formatting, Spotless, and Scalastyle pass. Local Spark 3.5/4.1 tests remain dependency-DNS blocked; hosted CI has started.
Which issue does this PR close?
No associated issue.
Rationale for this change
Spark compares compatible struct fields by position. Arrow also includes field names and nested nullability in its types, so Comet must reconcile that metadata without moving the values. Letting a name-based cast do the reconciliation can change the answer or make a valid query fail.
For example, Spark 4.x accepts this comparison:
Spark returns
true,false, andfalse. Atid = 0, it compares[0.0, 1.0]with[1.0, 0.0]. The old name-based cast instead rearranges the right-hand values to[0.0, 1.0], so Comet incorrectly returnsfalse. Other layouts fail because their nested Arrow types still have different names or nullability.The illustration shows the same positional alignment principle with null fields:
The right-hand values stay in their original positions. Only the metadata used by the comparison is aligned.
Membership needs the same treatment.
named_struct('x', id) IN (named_struct('y', id))should returntruefor every row, but its native path previously rejected the differently named types. Spark 3.x already accepts differently named structs forIN, so the membership regression covers all supported Spark versions. The differently named binary comparison above requires Spark 4.x.Conditional branches can also differ only in nested metadata. A
CASEchoosing between maps whose values are structs with corresponding namesx, XandX, xmust retain each branch's field positions. A name-based merge can put a null under a nonnullable field and fail Arrow validation. This matters even when every row chooses the same branch: the returned array must match the conditional's declared type.There is a separate constructor problem when native expressions infer more nullability than Catalyst. This query reaches it with ordinary constant folding enabled:
Spark returns arrays containing
{x: 0},{x: 1}, and{x: 2}, {x: 1}for the three rows. It declares bothxfields nonnullable. Native cast inference marks the first nullable, while nativeIFcorrectly marks the second nonnullable.array_repeatpreserves those field types, andarray_unionrejects the mismatch. Preserving Catalyst's field declarations makes the query succeed and match Spark. The existing normalization inarray(...)does not apply to arrays produced byarray_repeat.What changes were proposed in this PR?
A shared positional type-alignment routine now serves
IF,CASE, nested comparisons, andIN. It retains one set of field names and combines nullability at corresponding positions, recursively through structs, lists, and maps. Comet's Spark-compatible casts apply the common type while preserving value order. Exact type equality determines whether a cast can be skipped.Conditional alignment retains the existing coercion of differing leaf types, such as compatible timestamp representations. Comparison and membership alignment changes compatible nested metadata without introducing leaf-type conversions. The existing conditional evaluator continues to control which branches execute.
CreateNamedStructnow receives Catalyst's field nullability alongside its names and values. Its declared type and evaluated array use the same flags, including after expression rewriting, while preserving the children's physical types. Constructor validation rejects inconsistent metadata.The literal-expansion documentation is also corrected: native struct constructors support scalar broadcasting and preserve field nullability, while folded arrays of structs still require a separate expansion path.
How are these changes tested?
SQL regressions compare results with Spark and require native execution for positional comparisons and membership, conditional struct/map results, and the
array_repeat/array_unionconstructor mismatch. Native tests check the shared alignment rules and that constructor field declarations survive evaluation and expression rewriting.The Parquet fixture checks NULL structs and leaves, NaN, signed zero, and distinct field values across equality, ordering, and null-safe equality. The branch includes current main's independent floating-point normalization fix (#6447), so all nine stored rows exercise all three comparison paths.
After merging current main (
e76c35f1e), local validation passes 20 native conditional tests, four native constructor tests, and all 29 conditional/struct SQL-file cases on Spark 4.0.4. These include the new fixtures and existing IF, CASE, lazy-evaluation, IN, and IN-set coverage. Focused native Clippy checks (including tests, with warnings denied), Rust formatting, Spotless, Scalastyle, andgit diff --checkalso pass.For a before/after control, I built the native library at merge base
98662215dand ran five isolated queries against the same Spark 4.0.4 test harness. Ordering and null-safe equality returned wrong results; IN, map-valued CASE, and the constructor/array_union example raised errors. All five match Spark with the revised native library.Local Spark 3.5 and 4.1 runs stop before tests because dependency repositories fail DNS resolution (AWS SDK 1.12.262 and Jackson BOM 2.21.2 respectively). The PR retains the all-profile and Spark 4.1 CI labels; hosted results for the new revision remain pending.