[SPARK-45900][SQL][PYTHON] Add xxh3_64 and xxh3_128 functions - #57690
[SPARK-45900][SQL][PYTHON] Add xxh3_64 and xxh3_128 functions#57690SreeramaYeshwanthGowd wants to merge 12 commits into
Conversation
| return hash128Hex(input, 0L); | ||
| } | ||
|
|
||
| /** Returns the XXH3 128-bit hash as a 32-character lowercase hex string (canonical big-endian). */ |
There was a problem hiding this comment.
This seems like > 100 chars, please fix to avoid lint issues.
|
@uros-b Thanks, fixed. Wrapped the comment to stay under the line limit. Also noting that the Base image build check is failing with a pull access denied error on docker.io/library/root:latest, unrelated to this PR. |
…ee Also link for xxh3
c20d455 to
c407be6
Compare
|
@cloud-fan Would you have a moment to review this when you get a chance? Thank you! |
cloud-fan
left a comment
There was a problem hiding this comment.
0 blocking, 1 non-blocking, 0 nits.
The API and algorithm integration are coherent; one small per-row allocation should be removed from the 128-bit formatting path.
Suggestions (1)
- sql/catalyst/src/main/java/org/apache/spark/sql/catalyst/expressions/XXH3.java:417: This adds an avoidable 16-byte allocation for every row evaluated by
xxh3_128. The array only stages the two hash lanes before the next loop reads them back, so please encodeh[1]andh[0]directly into the 32-byte hex buffer. -- see inline
Verification
I traced both public functions from SQL/Scala/PySpark entry points through FunctionRegistry to the Catalyst expressions and the shared Java implementation. The known-answer suite covers every algorithm length family for both widths and includes nonzero-seed vectors; I did not run tests during this review.
| public static UTF8String hash128Hex(byte[] input, long seed) { | ||
| long[] h = hash128(input, seed); | ||
| byte[] out = new byte[16]; | ||
| writeBE64(out, 0, h[1]); |
There was a problem hiding this comment.
This creates an avoidable 16-byte allocation for every row evaluated by xxh3_128. Since out only stages the two hash lanes before the next loop reads them back, please encode h[1] and h[0] directly into the 32-byte hex buffer.
|
@cloud-fan Thanks for catching that, applied it and also removed the now-unused writeBE64 helper. |
cloud-fan
left a comment
There was a problem hiding this comment.
1 addressed, 0 remaining, 2 new. (0 newly introduced, 2 late catches, 0 previously raised.)
0 blocking, 1 non-blocking, 1 nit.
The public API and algorithm integration remain coherent, but two small implementation/documentation issues should be cleaned up.
Nits: 1 minor item (see inline comments).
Suggestions (1)
- sql/catalyst/src/main/java/org/apache/spark/sql/catalyst/expressions/XXH3.java:415: The formatting path still allocates a two-element
long[]for everyxxh3_128evaluation, then immediately reads both lanes into the final hex buffer. Please add an internal path that writes the lanes directly to the caller's output (while keepinghash128for tests/callers that need the pair), so the hot SQL path allocates only its result buffer. -- see inline
Verification
I traced both public functions from the Scala/Python/SQL entry points through FunctionRegistry and the Catalyst expressions to the shared Java implementation. I also checked the known-answer coverage across both widths and all algorithm length families. I did not run tests during this review.
| * https://github.com/Cyan4973/xxHash, as specified in doc/xxhash_spec.md). The output is byte | ||
| * compatible with the reference implementation, so it matches `xxhsum` and other XXH3 tools. | ||
| * | ||
| * <p>All arithmetic is on 64-bit lanes; Java's signed {@code long} is used as an unsigned 64-bit |
There was a problem hiding this comment.
Please narrow this claim to the 64-bit multiplication/mixing machinery. The supported 1-3 byte XXH3-128 path uses 32-bit int composition, reversal, and rotation before widening, so All arithmetic is not true for the class as a whole.
| * Returns the XXH3 128-bit hash as a 32-character lowercase hex string (canonical big-endian). | ||
| */ | ||
| public static UTF8String hash128Hex(byte[] input, long seed) { | ||
| long[] h = hash128(input, seed); |
There was a problem hiding this comment.
The hot xxh3_128 path still allocates this two-element array per row and immediately copies both lanes into the result buffer. Please add an internal path that writes the lanes directly to the caller's output, while retaining hash128 for callers that need the pair, so expression evaluation allocates only the final hex buffer.
|
@cloud-fan Thanks, narrowed the class doc claim. On the long[] in hash128Hex: removing it fully means giving each of the six length-branch methods a second hex-writing form, since hash128 needs to stay as-is for other callers and Java can't return two longs without packaging them. Given you flagged it non-blocking, happy to do it if you'd still like it, but wanted to flag the duplication cost first. |
cloud-fan
left a comment
There was a problem hiding this comment.
2 addressed, 0 remaining, 5 new. (0 newly introduced, 5 late catches, 0 previously raised.)
0 blocking, 1 non-blocking, 4 nits.
The implementation and API integration are coherent; one focused seed-coverage gap and four public-doc wording nits remain.
Suggestions (1)
- sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/XXH3Suite.scala:27: Add non-zero-seed coverage for the distinct 1-3-byte branch in both width-specific vector lists. -- see inline
Nits: 4 minor items (see inline comments).
Verification
I traced both public functions from SQL, Scala, Python, and Connect entry points through FunctionRegistry and the Catalyst expressions to the shared Java implementation. I checked the reference-vector matrix across all input-length families and both widths; I did not run tests during this review.
| * (github.com/Cyan4973/xxHash). Inputs are prefixes of the same pseudo-random buffer the reference | ||
| * test harness uses (tests/sanity_test.c); expected values were produced with the reference | ||
| * `xxhash` library and cover every length branch (0, 1-3, 4-8, 9-16, 17-128, 129-240, and the | ||
| * long path) with both a zero and a non-zero seed. |
There was a problem hiding this comment.
The non-zero-seed vectors skip the 1-3-byte branch for both widths; they jump from length 0 to 8. Please add one non-zero-seed vector in that range to each list so this claim stays accurate and the branch's seed arithmetic is exercised.
| Parameters | ||
| ---------- | ||
| col : :class:`~pyspark.sql.Column` or column name | ||
| target column to compute on. A column of string or binary type. |
There was a problem hiding this comment.
| target column to compute on. A column of string or binary type. | |
| The target column to hash, which must have string or binary type. |
| Parameters | ||
| ---------- | ||
| col : :class:`~pyspark.sql.Column` or column name | ||
| target column to compute on. A column of string or binary type. |
There was a problem hiding this comment.
| target column to compute on. A column of string or binary type. | |
| The target column to hash, which must have string or binary type. |
| * Returns a 64-bit hash value of the argument using the XXH3 algorithm. | ||
| * | ||
| * @param col | ||
| * the column to hash. A column of string or binary type. |
There was a problem hiding this comment.
| * the column to hash. A column of string or binary type. | |
| * the column to hash, which must have string or binary type. |
| * Returns a 128-bit XXH3 hash of the argument as a 32-character hex string. | ||
| * | ||
| * @param col | ||
| * the column to hash. A column of string or binary type. |
There was a problem hiding this comment.
| * the column to hash. A column of string or binary type. | |
| * the column to hash, which must have string or binary type. |
|
@cloud-fan Thanks, added the 1-3 byte seed vectors and applied all four wording suggestions. CI is green. |
cloud-fan
left a comment
There was a problem hiding this comment.
5 addressed, 0 remaining, 2 new. (0 newly introduced, 0 late catches, 2 previously raised.)
0 blocking, 2 non-blocking, 0 nits.
The new API and hash implementation are coherent; one previously raised per-row allocation remains as a non-blocking optimization.
Already raised in existing discussion (2)
- The earlier allocation thread remains applicable because
hash128Hexstill stages both hash lanes in a temporarylong[]. -- existing discussion - The hot
xxh3_128path still allocates a temporary two-elementlong[]for every non-null row before formatting the final hex buffer. -- existing discussion
Verification
I traced both public functions from SQL, Scala, Python, and Connect entry points through FunctionRegistry and the Catalyst expressions to the shared Java implementation. I checked the reference-vector coverage across both widths, all input-length families, and non-zero-seed short inputs. I did not run tests during this review.
|
@cloud-fan Removed the long[] entirely, the six length branches now write hex digits directly, hash128 is unchanged for callers that need the pair. hash128Hex now allocates only the hex buffer. |
cloud-fan
left a comment
There was a problem hiding this comment.
0 addressed, 1 remaining, 0 new.
0 blocking, 1 non-blocking, 0 nits.
The API and hash integration are coherent, but the previously raised result-buffer-only allocation goal remains incomplete for medium-length inputs.
Remaining from prior review (1)
- The new hex-writing path still allocates a temporary two-element long array for every 17-240 byte input, so the stated goal that hash128Hex allocate only its result buffer is not met for these common length branches. Keep both accumulator lanes in scalar locals (or use scalar-returning helpers) before writing the final hex bytes. -- existing thread
Verification
I traced the Scala, Python, Connect, and SQL entry points through FunctionRegistry and the Catalyst expressions to the XXH3 implementation. I compared the direct hexadecimal-writing branches with the raw 128-bit paths, checked their canonical lane order, and reviewed the reference-vector matrix across every algorithm length family and both seed modes. I did not run tests during this review.
|
@cloud-fan Updated since to 4.4.0, since branch-4.3 is now cut. Also scalarized the accumulator in the 17-240 byte branches, so that comment is addressed too. |
cloud-fan
left a comment
There was a problem hiding this comment.
1 addressed, 0 remaining, 1 new. (0 newly introduced, 1 late catch, 0 previously raised.)
0 blocking, 0 non-blocking, 1 nit.
The implementation and API integration are coherent; one internal allocation comment overstates the long-input path.
Nits: 1 minor item (see inline comments).
Verification
I traced the SQL, Scala, Python, and Connect entry points through FunctionRegistry and the Catalyst expressions to the Java helper. I compared the direct hexadecimal-writing branches with the raw 128-bit path, checked canonical lane order and the length-family vector coverage, and confirmed the previously raised medium-input accumulator allocation was removed. I did not run tests during this review.
|
|
||
| // Hex-writing siblings of the length-branch methods above: same arithmetic, but they encode | ||
| // the two lanes directly into the caller's hex buffer instead of returning a long[], so the | ||
| // hot hash128Hex path below allocates only that buffer. hash128 above is kept array-returning |
There was a problem hiding this comment.
Please narrow this to say the Into variants avoid the temporary result-pair array. Inputs longer than 240 bytes still call hashLongAccumulate, which allocates a long[], so the current claim that this path allocates only the hex buffer is broader than the implementation.
|
@cloud-fan Thanks, narrowed the comment to scope the allocation claim to the Into variants only. |
cloud-fan
left a comment
There was a problem hiding this comment.
1 addressed, 0 remaining, 0 new.
0 blocking, 0 non-blocking, 0 nits.
The previously raised allocation-comment issue is addressed, and the current implementation and API integration are ready from this review's perspective.
Verification
I traced the SQL, Scala, Python, and Connect entry points through FunctionRegistry and the Catalyst expressions to the XXH3 helper. I compared the direct hexadecimal-writing branches with the raw 128-bit path, checked canonical lane order and reference-vector coverage across length families and seed modes, and confirmed the latest comment accurately scopes the remaining long-input accumulator allocation. I did not run tests during this review.
|
@cloud-fan CI is green. Thans for the review. |
### What changes were proposed in this pull request?
Add two built in scalar SQL functions that hash their argument with the XXH3 algorithm:
- `xxh3_64(expr)` returns a 64-bit hash as a `BIGINT`.
- `xxh3_128(expr)` returns a 128-bit hash as a 32-character lowercase hex `STRING`.
```
> SELECT xxh3_64('Spark');
80997306238743657
> SELECT xxh3_128('Spark');
7d57dd84c60c86ca1f4e82ab91a12b5e
```
API surface added:
- SQL: `xxh3_64`, `xxh3_128`
- Scala DataFrame: `functions.xxh3_64(col)`, `functions.xxh3_128(col)`
- PySpark, classic and Spark Connect: `pyspark.sql.functions.xxh3_64` / `xxh3_128`
Implementation notes:
- A new `XXH3` Java class ports the XXH3 64-bit and 128-bit hashes from the reference implementation (github.com/Cyan4973/xxHash, per `doc/xxhash_spec.md`). It is a self contained port with no new dependency, mirroring the existing `XXH64` helper.
- The output is byte compatible with the reference implementation, using the default seed 0 and the standard canonical serialization for the 128-bit result (high 64 bits then low 64 bits, big endian).
- The two expressions live next to the other digest functions in `hash.scala` and follow the `md5` / `crc32` pattern: single argument, input implicitly cast to `BinaryType`, `nullIntolerant`, with `xxh3_64` returning `LongType` (like `crc32`) and `xxh3_128` returning a hex string (like `md5`).
### Why are the changes needed?
Spark's existing `xxhash64` returns 64 bits, which collides too often for large scale surrogate keys or deterministic sampling (a birthday collision becomes likely in the billions of rows). XXH3 is the modern successor to XXH64: `xxh3_128` provides a collision resistant 128-bit digest, and `xxh3_64` is a faster 64-bit alternative. This is a natural extension of the existing hash function family (`md5`, `crc32`, `xxhash64`).
Design decisions (worth an explicit review, as this is a permanent public API):
- These are single argument digests that hash the raw bytes of one value with the default seed 0, so the output is byte compatible with the reference XXH3 and interoperates with the reference tools (for example `xxh128sum`). This differs from `xxhash64`, which is a variadic, structural hash with an internal seed (not reference compatible). The 128-bit result is the canonical XXH3 hex; the 64-bit result is the two's complement view of the unsigned XXH3 value, so its hex matches the reference while the signed decimal representation may differ.
- Two functions with static return types are provided instead of one `xxhash3(expr, bits)` function, because a Spark expression needs a statically known result type.
- `xxh3_128` returns a hex `STRING` (like `md5`), matching the reference tool's canonical hex output, rather than raw `BINARY`. A seeded variant could be added as a follow up.
### Does this PR introduce _any_ user-facing change?
Yes. It adds two new built in SQL functions and their Scala and PySpark DataFrame API entries. No existing behavior changes.
### How was this patch tested?
- `XXH3Suite` validates the port against the reference implementation's known-answer vectors, covering every length branch (0, 1-3, 4-8, 9-16, 17-128, 129-240, and the long path) and block boundaries with both a zero and a non-zero seed, for both the 64-bit and 128-bit outputs.
- `HashExpressionsSuite` checks the expressions' values, null handling, and interpreted vs codegen consistency.
- SQL golden tests in `misc-functions.sql` (string and binary input, and null propagation), and regenerated `sql-expression-schema.md`.
- PySpark doctests.
### Was this patch authored or co-authored using generative AI tooling? No
Closes #57690 from SreeramaYeshwanthGowd/add-xxh3-hash-functions.
Authored-by: SreeramaYeshwanthGowd <yeshwanthgowdsreerama@gmail.com>
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
(cherry picked from commit 1eef893)
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
|
Oh! I've been working on this problem too, was just about to tidy it up today and saw this PR come through, what luck! I've been working on a performant version of this based on a java implementation i've been working on over the years. I've also expanded the hashing type to more easily easily support map and variant types, something which the older hashing methods have not been able to do as easily. Curious @cloud-fan @SreeramaYeshwanthGowd how you think about my approach and if there is room to weave in some of my thinking. The main difference when it comes to the API is that I've used byte[]/BINARY (xxhash128) and long/BIGINT (xxhash3) for the implementation - rather than string, which ensures we can take advantage of future benefits (especially if we implement a fixedbinary type) that would allow us to have better performance when it comes to SIMD. (especially for variants like Photon which would be able to speed this up a lot more). Users can still have the string return type by wrapping it within hex() if they explicitly need that kind of bheaviour. The other major change I made was to be null aware within the hashing logic of xxhash64 where (x, null, y) is hashed the same as (x, null, null, y). This resolves a common issue i've seen in the field where someone has bound the wrong column to the ingestion process in bronze - fixes it - and the hash still remains the same as the hashing method doesn't have a concept of arity. I'll have my blog post up soon so would love to hear what you think! |
|
@NathanNZ Thanks for sharing this, and glad the timing worked out for you to catch it! I appreciate you raising these points. On the return type: xxh3_64/xxh3_128 returning a hex string was a deliberate match to Spark's existing hash function family. md5, sha1, sha2, and crc32 all document returning a hex string (e.g. md5's scaladoc: "returns the value as a 32 character hex string"). On the arity point: xxh3_64/xxh3_128 each take a single column (def xxh3_64(col: Column)), so there's no multi-argument concatenation happening here for arity to be unaware of. That concern would apply to a multi-argument hash function like the existing hash()/xxhash64(), not to these. As with other PRs in this area, could you raise a JIRA ticket for this so it can be tracked and discussed properly, then open a PR against it? Happy to take a look once that's up. |
|
Cheers for your reply @SreeramaYeshwanthGowd - and for getting the xxh3 family across the line! Just so happens I was the original author of JIRA-45900, as part of that ticket I did hint at my concerns in passing about SHA2 and MD5 returning string types - but I can see how that wasn't explicit in the ticket - and I can see the argument that consistency in API's is king, so I'll open new ticket instead to build on top of these changes rather than re-open. My current thinking is something along the lines of xxh3_64_bytes and xxh3_128_bytes, would love to see the xxhash64 multi-argument functionality make it in - found that customers love a simple API that amounts too "hash these columns so I can compare them against other columns for changes" over having to roll their own, will add more detail in the ticket about how that could look. In the meantime I'll clean up my code so I can rebase it on top of your changes without turning the commit history into spaghetti. Just so the new jira ticket can come with a practical example (rather than waiting 3 years before I get around to implementing something 😉) |
|
https://issues.apache.org/jira/browse/SPARK-58677 - I've opened this ticket to hold the discussion of potential improvements to this patch. I believe that we are in a good space (if we need to as we're not within a position where a change to the ABI would cause breaking change to users) to use a BINARY return type rather than a STRING return type - so would love to hear your feedback on that ticket! As well as supporting a multi-column ABI as per xxhash64 to make this a true successor function. This is a bit of a change on my previous message but looking at the docs with both methods side by side I was feeling it was a bit clunky without providing any benefits except for aligning with the other hashing methods.. but seeing that one doesn't join hashing methods on other hashing methods I couldn't justify it internally to keep the STRING return type. There are likely some tweaks to what could or should be implemented, but I'll see if I can get both codebases running side by side to demonstrate some of the benefits and potential downsides of the change! |
What changes were proposed in this pull request?
Add two built in scalar SQL functions that hash their argument with the XXH3 algorithm:
xxh3_64(expr)returns a 64-bit hash as aBIGINT.xxh3_128(expr)returns a 128-bit hash as a 32-character lowercase hexSTRING.API surface added:
xxh3_64,xxh3_128functions.xxh3_64(col),functions.xxh3_128(col)pyspark.sql.functions.xxh3_64/xxh3_128Implementation notes:
XXH3Java class ports the XXH3 64-bit and 128-bit hashes from the reference implementation (github.com/Cyan4973/xxHash, perdoc/xxhash_spec.md). It is a self contained port with no new dependency, mirroring the existingXXH64helper.hash.scalaand follow themd5/crc32pattern: single argument, input implicitly cast toBinaryType,nullIntolerant, withxxh3_64returningLongType(likecrc32) andxxh3_128returning a hex string (likemd5).Why are the changes needed?
Spark's existing
xxhash64returns 64 bits, which collides too often for large scale surrogate keys or deterministic sampling (a birthday collision becomes likely in the billions of rows). XXH3 is the modern successor to XXH64:xxh3_128provides a collision resistant 128-bit digest, andxxh3_64is a faster 64-bit alternative. This is a natural extension of the existing hash function family (md5,crc32,xxhash64).Design decisions (worth an explicit review, as this is a permanent public API):
xxh128sum). This differs fromxxhash64, which is a variadic, structural hash with an internal seed (not reference compatible). The 128-bit result is the canonical XXH3 hex; the 64-bit result is the two's complement view of the unsigned XXH3 value, so its hex matches the reference while the signed decimal representation may differ.xxhash3(expr, bits)function, because a Spark expression needs a statically known result type.xxh3_128returns a hexSTRING(likemd5), matching the reference tool's canonical hex output, rather than rawBINARY. A seeded variant could be added as a follow up.Does this PR introduce any user-facing change?
Yes. It adds two new built in SQL functions and their Scala and PySpark DataFrame API entries. No existing behavior changes.
How was this patch tested?
XXH3Suitevalidates the port against the reference implementation's known-answer vectors, covering every length branch (0, 1-3, 4-8, 9-16, 17-128, 129-240, and the long path) and block boundaries with both a zero and a non-zero seed, for both the 64-bit and 128-bit outputs.HashExpressionsSuitechecks the expressions' values, null handling, and interpreted vs codegen consistency.misc-functions.sql(string and binary input, and null propagation), and regeneratedsql-expression-schema.md.Was this patch authored or co-authored using generative AI tooling? No