viirya commented on PR #5754:
URL:
https://github.com/apache/datafusion-comet/pull/5754#issuecomment-5574397884
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
