fix: apply the parent struct's null mask before hashing its fields

[viirya]

Which issue does this PR close?

Closes #5753.

Rationale for this change

Arrow keeps a StructArray's children validity independent of the parent's, so at a row
where the struct itself is null a child buffer can still hold a value. The struct branch of
create_hashes_internal! recursed straight into struct_array.columns() without consulting
the parent's null buffer, so a null struct hashed whatever happened to sit in the child slot
instead of leaving the seed alone as Spark does (case null => seed).

Two rows with the same logical key then hash differently. What the tests here demonstrate
directly is a wrong answer from hash and xxhash64 in a Spark query. The same hash decides
shuffle partition assignment, so equal keys reaching different partitions follows from it, but
that path is not exercised by these tests.

The List and Map branches in the same match already guard on is_null; only Struct did
not.

What changes are included in this PR?

• utils.rs: use StructArray::flatten, which unions the parent's nulls into each child
  exactly the way GetStructField returns non-null for fields of a NULL struct (missing null-mask propagation) #4432 fixed the same null-mask propagation problem in GetStructField, and
  does it without revalidating the child data buffers — the union only ever adds nulls, so the
  buffers are unchanged. Going through the checked ArrayData builder instead would rescan
  every child buffer on each call, and for a string child that is the whole UTF-8 values
  buffer.
• flatten is called only when the parent actually has a null. It returns early with no null
  buffer, but with a buffer present it builds a fresh Fields with every non-nullable field
  re-marked nullable, which this call site discards. So the case worth skipping is a buffer
  that is present and all-valid, which is what slicing leaves behind. NullBuffer caches its
  null count, so the test itself is O(1).
• Regression tests for both hash algorithms, since the struct branch is shared through the
  macro.

How are these changes tested?

cargo test -p datafusion-comet-spark-expr passes 714 + 5, and CometHashExpressionSuite
passes 40.

Unit tests. For each hash algorithm, a null struct whose child buffer still holds a value
must hash the same as the equivalent struct whose child slot is also null, and the seed must
survive. Both fail without the change — under murmur3 the null row hashes 3319311472 rather
than the 42 seed. A second pair hashes a null struct element inside a list, with valid
elements either side so the chaining is exercised, because that is the per-element route
through hash_list_array! that #5567 made usable as a shuffle key, and the direct-struct test
does not reach it.

End-to-end. Spark tests, using the shape @andygrove identified: a nullable struct
whose child field is REQUIRED is written as optional group c { required int32 a; }, and on
read the child leaf has nowhere to record a null of its own, so its buffer holds a value at
exactly the rows where the struct is null. One case has a scalar child, one has a struct child
so the union has to recurse. Both disagree with Spark on unfixed main.

None of the 18 pre-existing hash tests change, which is the signal that this only moves the
null-struct case that was already wrong.

A third end-to-end case covers the per-element route through hash_list_array!. Note the two
list tests cover different shapes and are not redundant: the unit test makes the list element
itself the null struct, while this one has a valid element wrapping a null struct, which is the
shape a query produces. Using the
struct as the list element directly does not reproduce, because a null element is rebuilt on
the way into the array and the hidden child values go with it. Wrapping it does:
array(named_struct('tag', 1, 'b', c)) yields an element that is itself valid, so it is copied
rather than rebuilt, and the null c inside keeps the values Parquet wrote under it. Covered
with one element and with two, so the chaining between elements is exercised as well; both
disagree with Spark before the change.

Additional context

Found while reviewing #5567, which makes this branch reachable from shuffle partitioning, but
the defect predates it and reproduces on main unchanged.

Follow-ups rather than scope creep here:

• A hash benchmark, raised in review: bench: add a benchmark for the Spark hash kernels #5765.
• GetStructField::project_field implements the same Spark rule with the checked builder:
  Share one helper for pushing a struct's null mask into its children #5768, which lays out the three decisions unifying them would need.

[andygrove]

Good catch, and the reasoning about why only the Struct branch was missing the guard is convincing. Approving, with a few things I would like to see addressed.

A Spark end-to-end test is possible, and I think it belongs here

The PR only has Rust unit tests that hand-build the array, so it is not obvious that a real query can produce that shape. I went looking and it can, which makes this a user-visible wrong-answer bug rather than hardening.

None of the obvious shapes reproduce. A struct written with when(col("id") > 1, struct(...)), hash(nested1.nested2), hash(array(nested1)), a CASE WHEN producing a struct, and the spark.comet.convert.parquet.enabled path all agree with Spark on unfixed main. That is why the existing hash - struct and hash - nested struct tests in CometHashExpressionSuite never caught this.

What does reproduce is a nullable struct whose child field is non-nullable. Spark writes optional group c { required int32 a; }, and on read the child leaf has nowhere to record a null of its own, so the child buffer holds a value at exactly the rows where the struct is null. That is the case the comment in get_struct_field.rs is already describing when it mentions parquet files where a logically-null struct column still has a populated child buffer.

test("hash - null struct with a required child field") {
  // `c` is nullable but its child is REQUIRED, so Spark writes
  // `optional group c { required int32 a; }`. On read the child buffer holds a value at the
  // rows where the struct itself is null, which is the case the parent null mask has to cover.
  withTempPath { dir =>
    val schema = StructType(
      Seq(
        StructField(
          "c",
          StructType(Seq(StructField("a", IntegerType, nullable = false))),
          nullable = true)))
    withSQLConf(CometConf.COMET_ENABLED.key -> "false") {
      val rows = Seq(Row(Row(1)), Row(null), Row(Row(3)), Row(null))
      spark
        .createDataFrame(spark.sparkContext.parallelize(rows), schema)
        .coalesce(1)
        .write
        .parquet(dir.toString)
    }
    spark.read.parquet(dir.toString).createOrReplaceTempView("t")
    checkSparkAnswerAndOperator("SELECT c, hash(c), xxhash64(c) FROM t")
  }
}

On main this fails, with the two null rows hashing to non-seed values:

!== Spark Answer - 4 ==            == Comet Answer - 4 ==
 [-1823081949,6258084186791473711]  [-1823081949,6258084186791473711]
![-559580957,-6698625589789238999]  [-1823081949,6258084186791473711]
![42,42]                            [-559580957,-6698625589789238999]
![42,42]                            [933211791,3614696996920510707]

With the change applied it passes. That also confirms the shuffle partitioning claim in the description rather than leaving it as an inference.

Would it also be worth adding a case where the null struct's child is itself a struct, so the test shows the union recursing more than one level, and a list<struct> case, since that is the per-element path and the shape #5567 made reachable as a shuffle key?

Overhead

I benchmarked create_murmur3_hashes on an 8192-row struct<a: int32, b: utf8>, release build, alternating between the two versions to control for drift. Noise is around plus or minus 2 percent.

case	before	after (flatten)	after with a null_count() > 0 guard
parent nulls None	34.3 us	34.6 us	34.1 us
parent null buffer, 0 nulls	34.3 us	34.7 us	34.1 us
parent 10% nulls	34.1 us	37.9 us (+12%)	38.7 us
list<struct>, 1024 lists x 8 elements	843 us	866 us (+2.7%)	851 us

The no-null-buffer path is unchanged, which is what I hoped for, since flatten early-returns and clones the same Vec the old code did. The 12 percent on a struct that actually has nulls is mostly not flatten's allocations, it is that the children now carry a null mask so the child hashing loops take the null-checked branch instead of the null_count() == 0 fast path. That is inherent to the fix and I would not try to optimize it away.

The one piece that is pure waste is that flatten() also builds a fresh Fields, re-marking non-nullable fields as nullable, and this call site discards it with let (_, columns). That is a Vec plus an Arc<[FieldRef]> allocation on every call, on a path that runs once per list element. Guarding on the null count skips it, and the union, whenever the buffer is present but all valid, which is what a slice leaves you with:

let columns: Vec<ArrayRef> = match struct_array.nulls() {
    Some(n) if n.null_count() > 0 => struct_array.flatten().1,
    _ => struct_array.columns().to_vec(),
};

NullBuffer caches its null count so the check is free, and it keeps the no-null path provably identical to what is there today.

A benchmark for hashing

There is no hash benchmark anywhere in native/spark-expr/benches despite the 70-odd benches already there, and #5567 just made this the native shuffle partitioning path. Could a hash.rs bench covering struct, list<struct> and map keys be added, or filed as a follow-up issue and linked from this PR?

Two smaller things

GetStructField::project_field in struct_funcs/get_struct_field.rs does the same parent-to-child null union, with the checked build()? rather than the unchecked path this comment argues for. Since both implement the same Spark rule, is it worth pulling it into one shared helper so they cannot drift and so the checked versus unchecked decision lives in one place?

The description says the fix pushes the nulls in with NullBuffer::union, but the code went with struct_array.flatten(). Since the description becomes the commit message, could you update it to match?

[viirya]

Thanks — the Parquet shape was the missing piece, and it turned this from hardening into a demonstrable wrong answer. All four points addressed.

End-to-end tests. Added two, using your optional group c { required int32 a; } shape: one with a scalar child and one where the child is itself a struct, so the union has to recurse. Both disagree with Spark on unfixed main and pass with the change.

I also tried the array<struct<..>> case you asked about and have not included it, because it agrees with Spark either way, so it would not be a regression test for this fix. Worth recording why, since my first explanation was wrong: I assumed Spark had made the element's child nullable, based on the read schema showing a: integer (nullable = true). Checking the footer instead, the file really does contain the layout —

optional group c (LIST) {
  repeated group list {
    optional group element {
      required int32 a;
    }
  }
}

— and ParquetSchemaConverter applies containsNull to element only, then recurses on each child's own nullability, so there is no rule forcing children nullable. The read schema relaxing a says nothing about the file or the Arrow buffers. So this is "does not reproduce on the path I tested" rather than "cannot happen"; something in the read path appears to normalise the child nulls there, which I did not chase further. The commit message says this rather than claiming unreachability.

To cover the per-element path anyway, there is now a unit test that hashes a null struct element inside a list, with valid elements either side so the chaining is exercised too, for both hash algorithms. My original test hashed a struct directly and so never went through hash_list_array! — that was a real gap.

Overhead. Applied your null_count() > 0 guard. I confirmed the reasoning in the arrow source: flatten returns early when there is no null buffer, but with a buffer present it builds a fresh Fields before checking anything, and this call site discards it — so the case worth skipping is a buffer that is present and all-valid, which is what slicing leaves behind. NullBuffer caches its null count, so the test is O(1). I agree the 12% on a struct that genuinely has nulls is inherent, since the children now carry a mask and the child loops take the null-checked branch; not worth optimising away.

Description. Updated. I also narrowed a claim while I was there: it said the bug "means they can land in different partitions and break grouping and joins", which was an inference. It now says the tests demonstrate a wrong answer from hash/xxhash64 directly, and that the partitioning consequence follows but is not exercised here.

Hash benchmark. Filed as #5765 rather than added here, so it lands against main and its numbers can be reproduced independently of the change that motivated them. It covers int32, utf8, struct, array<int32>, array<struct<..>> and map<utf8, int32>. The two list shapes sit next to each other because they take different paths, and the gap is stark — 141 µs versus 9660 µs for the same element count, which is the shape behind the hash.nested.enabled default in #5567. Only murmur3 is covered: create_xxhash64_hashes is pub(crate), and widening visibility just for a benchmark seemed the wrong trade.

Shared helper. I would rather do this as a follow-up than here, if you are happy with that. The duplication predates this PR, both versions are correct today, and unifying them involves a design choice I would not want to bury in a bug fix: project_field extracts one field with the checked builder, this one flattens all of them via the unchecked path. Unifying on unchecked means arguing the safety case for project_field too; unifying on checked reintroduces the per-element revalidation this PR is avoiding. There is also the question of whether project_field should take the null_count() > 0 guard. I will open an issue with those options once this merges, and link it here.

[viirya]

Correction to my previous comment: the array<struct<..>> case does reproduce, and there is now an end-to-end test for it.

What I had missed is that it depends on how the struct reaches the array. Using it as the element directly, array(c), does not reproduce — a null element is rebuilt on the way in and the hidden child values go with it, which is why my first attempt was green either way. Wrapping it does: array(named_struct('tag', 1, 'b', c)) yields an element that is itself valid, so it is copied rather than rebuilt, and the null c inside keeps the values Parquet wrote under it.

hash(array(named_struct('tag', 1, 'b', c)))

On unfixed main that gives 245521047, 292098604, 1066699649, 1066699649 where Spark gives -559580957, -559580957, 245521047, 1066699649; with the change they agree. Covered with one element and with two, so the chaining between elements is exercised as well, and the plan is native in both. So the per-element path #5567 made reachable now has direct end-to-end coverage rather than only the unit test I mentioned before. I had concluded too early that the shape was unreachable through a query.

One number in that comment has also moved: array<struct<..>> in #5765 now measures 12609 µs rather than 9660 µs, because I added a skewed shape to that benchmark and remeasured. The 141 µs for array<int32> is unchanged, so the gap is wider than I quoted, not narrower.

[sunchao]

Correctness

This fixes a real mismatch in the shared struct hashing path. Previously, a null struct could hash values still present in its child buffers. The change combines the parent mask with each child's mask before recursion, so those rows preserve the incoming hash seed. Valid fields continue to hash in field order, including when a struct is nested inside another struct or a list element.

I compared this with the maintained Spark 3.5 and 4.0 implementations. Both interpreted hashing and generated code skip null values, and the recursive field and array chaining agrees with this change. The hash and xxhash64 return types, seed handling, supported-type checks and fallback behavior are unchanged. This null rule has no ANSI-mode branch. Empty structs still leave the seed unchanged. Arrow 59.3.0's flatten preserves data buffers and correctly combines sliced parent and child masks. I found no new P1/P2 issue in the reviewed change.

The four new Rust regressions cover both algorithms, direct null structs and null struct elements between valid list elements. The Scala tests add the required-child Parquet shape, recursive masking and a valid list element wrapping a null struct, with one and two elements. Their helper compares Spark answers and requires native operators. Programmatic schema construction is justified here because the required child is essential to exposing the defect.

At the 2026-09-08 04:28 UTC snapshot, CI had 56 successful checks, 7 skipped and 8 running, with no recorded failure. The Rust job passed 1,192 tests, including all four new regressions, with 4 skipped. Its checkout was merge 88a375a, whose parents are the assigned base 92ad99e and head 4ad9411, and whose tree equals the reviewed head. Eight broader SQL/Iceberg jobs remained running. I have independently inspected only the Rust test log, so I am not taking new JVM execution credit from the updated status counts. I have not independently rerun the JVM tests or exercised shuffle partition assignment. Source comparison is limited to the maintained Spark 3.5 and 4.0 branches, with no compatibility claim for unavailable maintained versions.

Performance

The null-count guard preserves the existing no-null path, including slices carrying an all-valid mask. It avoids allocating discarded field metadata or combining masks in that case, and the count lookup is constant time. Structs with nulls need the additional mask work to produce correct hashes. Reusing Arrow's unchecked rebuild avoids validating unchanged child buffers on every recursive call. The nested-list allocation cost remains relevant, but this is a correctness fix to existing expressions. The focused benchmark work is tracked in #5765. I have not independently reproduced the discussion's timing figures.

Design

Putting the fix in create_hashes_internal! gives both hash algorithms the same null handling and lets existing recursion propagate it through deeper structs. Using Arrow's existing operation keeps the safety argument with the implementation that owns the array representation. The PR addresses the earlier review's regression shapes and all-valid-mask guard. The proposed unification with GetStructField is tracked in #5768, where the single-field versus all-fields API and checked versus unchecked rebuilding can be considered together.

Abstraction & complexity

The change adds no new abstraction or configuration. It uses a small conditional inside the existing shared implementation and preserves its callers. The unit tests and Parquet tests cover different representations of the null-struct problem, so their separate setup has a concrete purpose. I found no additional simplification that needs to be made before merging this fix.

[viirya]

Thanks @andygrove @sunchao !