Add rich comparison operators via PartialOrd trait implementation

[stochastical]

To address #49 I've implemented PartialOrd . This should use the correct multiset partial ordering semantics as in the Python implmentation.

• PartialEq now compares keys using the implici 'zero' element, rather than using equality on the underlying map.
• PartialOrd is now implemented to allow for the <, >, <=, >= operator overloads. Migrated is_subset and is_superset methods to wrap around the ParitialOrd::partial_cmp method.
• Added a small test (happy to add more too:)

I'm not sure if adding Zero as a trait bound is considered a breaking change, though?

Specifically, I think there's a bug in the master branch in that I don't think it's correct to use self.map == other.map for equality comparisons. If you manually set a key to be 0 on a Counter, it should be considered to be equal to a new Counter.

counter-rs/src/lib.rs

Lines 302 to 305 in e518825

	fn eq(&self, other: &Self) -> bool {
	// ignore the zero
	self.map == other.map
	}

[qodo-code-review]

PR Summary by Qodo

Implement PartialOrd and fix zero-aware equality for Counter

🐞 Bug fix ✨ Enhancement 🧪 Tests 🕐 10-20 Minutes

AI Description

• Fix Counter equality to treat missing keys as implicit zero counts.
• Implement PartialOrd to enable , >= using multiset partial-order semantics.
• Rebase is_subset/is_superset on partial_cmp and add a basic ordering test.

Diagram

classDiagram
class Counter {
  +map: HashMap
  +zero: N
}
class PartialEq
class Eq
class PartialOrd
Counter ..|> PartialEq : implements
Counter ..|> Eq : implements
Counter ..|> PartialOrd : implements

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Single-pass partial_cmp (no iterator cloning)

• ➕ Avoids cloning the chained keys iterator twice
• ➕ Can short-circuit earlier by tracking both le/ge flags in one traversal
• ➖ Slightly more complex implementation
• ➖ Same asymptotic behavior; benefit may be marginal for typical sizes

2. Deduplicate key iteration via HashSet union

• ➕ Avoids comparing the same key twice when present in both counters
• ➕ Makes intent (set union of keys) explicit
• ➖ Allocates a HashSet and hashes all keys
• ➖ Can be slower than allowing duplicates for small counters

3. Keep bespoke is_subset/is_superset logic (no PartialOrd)

• ➕ Avoids adding PartialOrd (and potentially additional trait bounds)
• ➕ Subset/superset semantics remain explicit and potentially optimized
• ➖ Does not enable , >= operators
• ➖ Logic duplication risks divergence from partial order semantics over time

Recommendation: The PR’s approach is the right direction: expressing multiset comparison semantics through PartialOrd both fixes correctness and unlocks rich operators, while reusing partial_cmp in is_subset/is_superset reduces duplication. Consider (optional) a single-pass partial_cmp to remove iterator cloning, but avoid HashSet-based dedup unless profiling shows a benefit. Note: adding Zero as a bound for PartialEq/Eq/PartialOrd can be a breaking change for custom count types that previously only implemented (Partial)Eq/(Partial)Ord.

Files changed (2) +40 / -20

Bug fix (1) +31 / -20

lib.rsCorrect zero-aware equality and add PartialOrd-based multiset ordering +31/-20

Correct zero-aware equality and add PartialOrd-based multiset ordering

• Updates PartialEq/Eq to compare counters using implicit zero semantics instead of raw map equality, so explicit zero entries don’t affect equality. Adds a PartialOrd implementation that returns Less/Greater/Equal/None based on elementwise count comparisons across both counters’ keysets. Refactors is_subset and is_superset to delegate to partial_cmp for consistent ordering semantics.

src/lib.rs

Tests (1) +9 / -0

unit_tests.rsAdd unit test for superset comparisons via rich operators +9/-0

Add unit test for superset comparisons via rich operators

• Introduces a small test asserting that a counter built from "ab" is strictly greater than one built from "a", validating PartialOrd-backed >= and > operator behavior.

src/unit_tests.rs

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 📎 Requirement gaps (0) 📜 Skill insights (0)

Context used

✅ Tickets: 🎫 `impl PartialOrd` via `is_{sub,super}set`

1. Comparisons depend on wrong zero 🐞 Bug ≡ Correctness

Description

PartialEq/PartialOrd now use indexing (self[k], other[k]) for implicit-zero semantics, which
relies on the struct field zero; but Counter::default() initializes that field with
N::default() rather than N::zero(). For N where Default and Zero differ, equality and
ordering (including subset/superset via partial_cmp) will yield incorrect results for missing
keys.

Code

src/lib.rs[R303-317]

    fn eq(&self, other: &Self) -> bool {
-        // ignore the zero
-        self.map == other.map
+        self.keys().chain(other.keys()).all(|k| self[k] == other[k])
+    }
+}
+
+impl<T, N, S> PartialOrd for Counter<T, N, S>
+where
+    T: Eq + Hash,
+    N: PartialOrd + Zero,
+    S: BuildHasher,
+{
+    fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
+        let keys = self.keys().chain(other.keys());
+        let le = keys.clone().all(|k| self[k] <= other[k]);
+        let ge = keys.clone().all(|k| self[k] >= other[k]);

Evidence

Indexing for missing keys uses the stored zero field, not N::zero(). The Default impl
initializes that field with N::default(). With this PR, equality and ordering now depend on
indexing, so a non-zero default sentinel changes comparison results for absent keys.

src/lib.rs[297-325]
src/impls/index.rs[9-49]
src/impls/create.rs[35-45]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`PartialEq`/`PartialOrd` use `self[k]`/`other[k]` to treat missing keys as zero. However, `Index::index` returns `&self.zero` for missing keys, and `Counter::default()` sets `zero` to `N::default()`, not `N::zero()`. This makes comparisons wrong for valid `N: Zero` types where `Default::default()` is not the additive identity.

### Issue Context
A non-breaking fix is to avoid `Index` inside `eq`/`partial_cmp` and instead compare via `map.get(k)` with a local fallback `let z = N::zero();` (using `unwrap_or(&z)`), so the comparison semantics are tied to `Zero` rather than the stored sentinel.

### Fix Focus Areas
- src/lib.rs[297-325]
- src/impls/index.rs[9-49]
- src/impls/create.rs[35-45]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. partial_cmp scans keys twice ✓ Resolved 🐞 Bug ➹ Performance

Description

Counter::partial_cmp evaluates both le and ge by iterating the union of keys twice, so
is_subset/is_superset (now implemented via partial_cmp) can do roughly double the HashMap
lookups compared to the prior single-pass implementations. This is a behavioral no-op but a real
worst-case performance regression for large counters or frequent subset/superset checks.

Code

src/lib.rs[R314-317]

+    fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
+        let keys = self.keys().chain(other.keys());
+        let le = keys.clone().all(|k| self[k] <= other[k]);
+        let ge = keys.clone().all(|k| self[k] >= other[k]);

Evidence

The new partial_cmp explicitly iterates the combined key iterator twice (once for le, once for
ge). is_superset/is_subset no longer perform their own single-pass all(...) checks, and
instead call partial_cmp, inheriting this potential double traversal.

src/lib.rs[308-325]
src/lib.rs[582-628]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`Counter::partial_cmp` currently computes `le` and `ge` by cloning the combined keys iterator and scanning it twice. `is_subset`/`is_superset` now delegate to `partial_cmp`, so they inherit this extra pass.

### Issue Context
This can be implemented in a single pass by tracking two flags (`le`, `ge`) and updating them per-key based on comparing counts; early-exit when both become false, and return `None` immediately if any per-key comparison is incomparable.

### Fix Focus Areas
- src/lib.rs[308-325]
- src/lib.rs[582-628]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

_{To customize comments, go to the Qodo configuration screen, or learn more in the docs.}

[coriolinus]

Thanks for implementing this! I do need two changes:

1. Remove the double-scan of the keys in partial_cmp.
2. Restore the wrongly-deleted doc comment.

[coriolinus]

This is in fact a breaking change, but I'm inclined to permit it on the basis that this is long overdue to replace the zero field with references to N: ConstZero, and that would also break the same things.

[stochastical]

Thanks for implementing this! I do need two changes:

1. Remove the double-scan of the keys in partial_cmp.
2. Restore the wrongly-deleted doc comment.

Thanks @coriolinus ! I've incorporated those amemndments :)

[coriolinus]

Looks good, thanks for putting in the work!