Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR extends hash-sorted-map to support deterministic hash-ordered iteration by introducing explicit iterator types and a sort_by_hash operation that reorders entries in-place (within hash-prefix group chains) before returning an owning HashSortedContainer. It also updates documentation around performance trade-offs and adds new Criterion benchmarks for iter/sort/merge scenarios.
Changes:
- Refactors core storage into
HashSortedContainer+Groupand re-exports the container. - Adds
Iter/IterMut/IntoIterfor bothHashSortedContainerandHashSortedMap, includingIntoIteratorimpls. - Adds
HashSortedMap::sort_by_hash()plus additional benchmarks and updated optimization notes.
Show a summary per file
| File | Description |
|---|---|
| crates/hash-sorted-map/src/lib.rs | Wires up new modules and re-exports container + iterator types. |
| crates/hash-sorted-map/src/iter.rs | Implements shared cursor-based iteration for container/map (immutable, mutable, and owning). |
| crates/hash-sorted-map/src/hash_sorted_map.rs | Refactors map to wrap HashSortedContainer, adds sort_by_hash, and updates insertion/lookup paths accordingly. |
| crates/hash-sorted-map/src/group.rs | Extracts Group struct and NO_OVERFLOW constant. |
| crates/hash-sorted-map/src/container.rs | Adds HashSortedContainer with allocation helpers and Drop that owns entry destruction. |
| crates/hash-sorted-map/OPTIMIZATIONS.md | Updates design notes and publishes new benchmark results/interpretation. |
| crates/hash-sorted-map/Cargo.toml | Adds smallvec dependency for sorting scratch storage. |
| crates/hash-sorted-map/benchmarks/performance.rs | Adds new benchmarks for iter/sort/merge, including k-way merge comparison. |
| crates/hash-sorted-map/benchmarks/Cargo.toml | Adds itertools dependency for k-way merge benchmark. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (1)
crates/hash-sorted-map/src/hash_sorted_map.rs:946
- Same issue as above: this test assumes hashes are strictly increasing (
h > prev_hash), but hash collisions can occur (especially with strings), so the correct property is non-decreasing order. Update the assertion to tolerate equal hashes or validate ordering with a deterministic tie-breaker.
let mut prev_hash = 0u64;
let mut first = true;
for (k, _) in &container {
let h = hasher.hash_one(k);
if !first {
assert!(h > prev_hash, "hash order violated: {prev_hash:#x} > {h:#x}");
}
- Files reviewed: 9/9 changed files
- Comments generated: 3
…into aneubeck/merge
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.
This PR
Note: hashmap remains intact after sorting now.