Repository navigation
perf(tuple): serialize union results without cloning summaries - #322
Open
Aschenbath wants to merge 2 commits into
Open
Aschenbath wants to merge 2 commits into
Aschenbath wants to merge 2 commits into
Conversation
Add TupleUnion::serialize. It prepares the union result over borrowed entries and encodes the summaries in place, so serializing no longer clones every retained summary into an intermediate CompactTupleSketch, and the summary type does not need to implement Clone. The union result preparation (theta filtering, trimming to nominal capacity, ordering) is shared between the cloning and the borrowing paths, and CompactTupleSketch::serialize uses the same encoder. Closes apache#321 Generated-by: Claude Code (Claude Opus 5.5) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
tisonkun
reviewed
Nov 5, 2026
tisonkun
left a comment
Member
There was a problem hiding this comment.
Thanks for your contribution!
I have a few comments:
compact_sketch_state_withlooks over abstraction.- Perhaps you can do the same for ThetaUnion and we may see some shared logics or common pattern for peer review.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #321.
What
TupleUnion::serialize(&self, ordered: bool) -> Vec<u8>. It needsP::Summary: TupleSummaryValuebut notClone.UnionStatehas one result-preparation routine (theta filtering, trimming to nominal capacity, ordering), and it takes a projection.to_compact_sketch_stateprojects withE::clone, so the Theta path compiles to the same code as before. The newto_borrowed_compact_sketch_state(gated ontuple) projects to&E.SketchEntryis implemented for&Eto support this.serialize_compact_state, which is generic overE: Borrow<TupleEntry<S>>.CompactTupleSketch::serializeandTupleUnion::serializeboth use it, so the format logic exists only once.When
orderedistrue, the output is byte-identical tounion.to_sketch(true).serialize(). For unordered results with trimming, entries can come out in a different order than in the cloning path, becauseselect_nth_unstablemay pick a different partition scheme depending on the element size. The retained entries, theta, seed hash and flags are the same. The doc comment states this.I started with
TupleUniononly, as the issue suggests.TupleIntersectionand mutable-sketch compaction aren't touched.Tests
Added to
tests-integration/tests/serde_tests/tuple.rs:union_serialization_matches_compact_result(u64summaries) comparesserializewithto_sketch(..).serialize()for both orderings. The cases are: a fresh union, an empty input, a single entry with combined summaries, exact mode (30 entries), trimming to nominal capacity (lg_k 5 → 32 entries), a sampled union (p = 0.5), and a non-empty result with every entry screened by theta. Ordered results must match byte for byte. Unordered results must have the same entries, theta, seed hash and flags.union_serialization_with_heap_backed_summaries_supports_later_mergesuses a variable-sizeVec<String>summary with a custom codec, in estimation mode. It round-trips the result, merges the restored sketch with another sketch, and compares against mergingto_sketch()directly.cargo x test,cargo +nightly clippy --tests --all-features --all-targets --workspace -- -D warnings,cargo +nightly fmt --all --checkandtypospass, andcargo +1.86.0 check -p datasketches --all-features(MSRV) passes too. I ran these on Windows x86_64. I didn't runtaploorhawkeyelocally: no TOML changed, and the new benchmark file has the license header.Benchmarks
New
benchmarks/tuple/serde.rscomparesserializewithto_sketch(..).serialize(). One input sketch goes into a union (default lg_k 12). Medians from Divan withAllocProfiler, unordered (ordered is similar):Heap-backed summary (4 short
Strings per key):to_sketch().serialize()serialize()u64summary, from two runs with 500 samples:to_sketch().serialize()serialize()So the gain is for summaries that are expensive to clone. For small
Copysummaries the borrowed path is about 15–25% slower at small sizes, a few hundred nanoseconds. Cloning 16-byte entries into a contiguous vector is about as cheap as collecting pointers, and the encoder then has to dereference each pointer. The CHANGELOG entry only claims the heap-backed case. If you'd rather keepu64users on the cloning path, I can document that onserialize. Another option is to collect(hash, &summary)pairs so that sorting doesn't dereference.This PR was written by an AI agent (Claude Code): the code, tests, benchmarks and this description. The verification above is what the agent ran.
🤖 Generated with Claude Code