You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "mrhard9090 (via GitHub)" <gi...@apache.org> on 2026/09/26 09:57:53 UTC
[PR] fix: xxhash64 of a NULL struct ignores the struct fields [datafusion]
mrhard9090 opened a new pull request, #25774:
URL: https://github.com/apache/datafusion/pull/25774
## Which issue does this PR close?
- Closes #25505.
## Rationale for this change
Spark's `xxhash64` treats a NULL value by leaving the running hash unchanged, so `xxhash64` of a NULL struct should be the seed, `42`. When a struct is NULL but its child arrays still hold values, for example after `nullif(s, s)`, DataFusion hashed those child values anyway:
```sql
SELECT s IS NULL, xxhash64(s), xxhash64(arrow_cast(NULL, 'Struct("a": Int64, "b": Utf8)'))
FROM (SELECT nullif(s, s) AS s FROM (SELECT named_struct('a', 1, 'b', 'x') AS s));
-- true 6158280145716684778 42
```
The same NULL gets two different hashes depending on how it was produced.
## What changes are included in this PR?
In the `DataType::Struct` branch of `create_hashes_internal!`, when the struct array has NULLs, the hashes are saved before the fields are hashed, and the saved value is put back for every NULL row. Without NULLs the branch works as before and copies nothing.
## What is the testing strategy for this PR?
- New unit test `test_xxhash64_null_struct_ignores_fields`: a struct whose second row is NULL but whose fields still hold values. The NULL row keeps the seed and the valid row is hashed.
- `spark/hash/xxhash64.slt` gets the query from the issue, which now returns `true 42 42`. Without the change it returns `6158280145716684778` for the masked NULL, so the test fails.
- `cargo test -p datafusion-spark --lib xxhash64` passes, and so do the `xxhash64` sqllogictest, `cargo fmt --all` and `cargo clippy -p datafusion-spark --no-deps --all-targets --all-features -- -D warnings`.
## Are there any user-facing changes?
`xxhash64` of a NULL struct now returns the input hash (`42` for a single argument), matching Spark. There are no API changes.
This PR was written with help from Claude Opus 5.5. I reviewed the change and ran the tests above myself.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: xxhash64 of a NULL struct ignores the struct fields [datafusion]
Posted by "neilconway (via GitHub)" <gi...@apache.org>.
neilconway commented on PR #25774:
URL: https://github.com/apache/datafusion/pull/25774#issuecomment-5894037883
@mrhard9090 If you get a chance to add that additional test, I can land this shortly
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: xxhash64 of a NULL struct ignores the struct fields [datafusion]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #25774:
URL: https://github.com/apache/datafusion/pull/25774#issuecomment-5856488440
## [Codecov](https://app.codecov.io/gh/apache/datafusion/pull/25774?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 82.49%. Comparing base ([`6a792c6`](https://app.codecov.io/gh/apache/datafusion/commit/6a792c671376c6c91f778530220358eb70a91271?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`73d8364`](https://app.codecov.io/gh/apache/datafusion/commit/73d83644047e717fc54979216d80ee641161362d?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 10 commits behind head on main.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #25774 +/- ##
==========================================
- Coverage 82.49% 82.49% -0.01%
==========================================
Files 1140 1140
Lines 438555 438572 +17
Branches 438555 438572 +17
==========================================
+ Hits 361803 361809 +6
- Misses 54880 54883 +3
- Partials 21872 21880 +8
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/datafusion/pull/25774?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: xxhash64 of a NULL struct ignores the struct fields [datafusion]
Posted by "neilconway (via GitHub)" <gi...@apache.org>.
neilconway commented on code in PR #25774:
URL: https://github.com/apache/datafusion/pull/25774#discussion_r4115564906
##########
datafusion/sqllogictest/test_files/spark/hash/xxhash64.slt:
##########
@@ -195,3 +195,12 @@ SELECT id, xxhash64(id, name) FROM xxhash_test ORDER BY id;
statement ok
DROP TABLE xxhash_test;
+
+# A NULL struct hashes like any other NULL, whatever its fields still hold
+query BII
+SELECT s IS NULL,
+ xxhash64(s),
+ xxhash64(arrow_cast(NULL, 'Struct("a": Int64, "b": Utf8)'))
+FROM (SELECT nullif(s, s) AS s FROM (SELECT named_struct('a', 1, 'b', 'x') AS s));
+----
+true 42 42
Review Comment:
I think this Claude suggestion is reasonable:
```
P2: The tests can't tell "restore the incoming hash" from "reset to 42" (xxhash64.rs:427-452, xxhash64.slt:199-206)
- Both tests put the struct in the first and only argument, where the incoming hash is always the seed.
- I changed *hash = before[i] to *hash = 42: all 18 xxhash64 unit tests and xxhash64.slt still pass.
- Suggest adding this SLT. It fails on base (-376592131717664991) and on the mutant (42):
query II
SELECT xxhash64(7, nullif(s, s)), xxhash64(7)
FROM (SELECT named_struct('a', 1, 'b', 'x') AS s);
----
-2864211556195259663 -2864211556195259663
```
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: xxhash64 of a NULL struct ignores the struct fields [datafusion]
Posted by "neilconway (via GitHub)" <gi...@apache.org>.
neilconway merged PR #25774:
URL: https://github.com/apache/datafusion/pull/25774
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: xxhash64 of a NULL struct ignores the struct fields [datafusion]
Posted by "mrhard9090 (via GitHub)" <gi...@apache.org>.
mrhard9090 commented on PR #25774:
URL: https://github.com/apache/datafusion/pull/25774#issuecomment-5906777784
Added the requested SLT (NULL struct as second argument, hash must equal single-arg hash). Verified: 18 xxhash64 unit tests pass, spark/hash/xxhash64 SLT passes.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org