Add PushPop, PushPopMax, DrainAsc, DrainDesc, Offer, and bounded heap support#8
Conversation
When orderedBubbledown moves an element via a grandchild hop, it may swap the intermediate parent (p = hparent(v)) to fix a cross-level violation. Previously only the last such swap position was recorded in r and bubbled up by the caller; earlier fixup positions were silently dropped. This caused PushPopMax to produce invalid heaps when the pushed element was small enough to trigger multiple grandchild hops, each with their own fixup swap — e.g. seed 353 (n=39, o=0): element 0 ended up at index 3 (min level) with value smaller than the root at index 0. Fix: call orderedBubbleup immediately at each fixup-swap position p inside orderedBubbledown, matching the same inline-bubbleup pattern used by the generic Fix method.
Also fix orderedBubbledown: remove the spurious inline orderedBubbleup call introduced in 7447c65. That call corrupted From() (Floyd's construction) and Pop() by bubbling up intermediate fixup positions mid-loop, leaving nodes out of order. The PushPopMax caller that needed all fixup positions handled now uses Fix() instead, matching v1's approach (Fix handles both up and down sifting). The bug was exposed by the new oracle-based DrainAsc/DrainDesc randomized tests.
There was a problem hiding this comment.
Pull request overview
This PR expands the deheap API with combined push+pop operations, drain iterators based on iter.Seq[T], and “bounded heap” helpers intended to keep only the N smallest elements.
Changes:
- Add
PushPop/PushPopMaxto both the generic (ordered.go) and v1 (deheap.go) APIs. - Add
DrainAsc/DrainDesciterators that consume the heap in sorted order. - Add bounded-heap constructors (
NewBounded,FromBounded),MaxLen, andOffer, and update Go/tooling to Go 1.23+.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| ordered.go | Adds generic PushPop/PushPopMax, drain iterators, and bounded-heap support (maxSize, constructors, MaxLen, Offer). |
| ordered_test.go | Extends fuzz commands and adds extensive tests/benchmarks for new ordered/generic APIs. |
| deheap.go | Adds v1 PushPop/PushPopMax implementations for heap.Interface. |
| deheap_test.go | Adds v1 unit tests, fuzz commands, and benchmarks for PushPop/PushPopMax. |
| example_ordered_test.go | Adds runnable examples for the newly introduced operations. |
| go.mod | Bumps the module to Go 1.23 to support iter.Seq[T]. |
| .github/workflows/ci.yml | Updates CI matrix to Go 1.23/1.24 and moves Codecov upload to the 1.24 job. |
| .github/workflows/release.yml | Updates release workflow Go matrix to Go 1.23/1.24. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // NewBounded returns an empty Deheap with a maximum size of maxSize. | ||
| // It panics if maxSize <= 0. | ||
| func NewBounded[T cmp.Ordered](maxSize int) *Deheap[T] { | ||
| if maxSize <= 0 { | ||
| panic("deheap: NewBounded maxSize must be positive") | ||
| } | ||
| return &Deheap[T]{maxSize: maxSize} |
There was a problem hiding this comment.
NewBounded/MaxLen are documented as a strict “maximum size”, but (*Deheap).Push does not consult maxSize, so callers can grow a bounded heap beyond MaxLen() by using Push directly. Either enforce the bound in Push (and define what happens on overflow), or adjust the docs/API to make it explicit that maxSize is only honored by Offer/FromBounded and that Push can exceed the limit.
There was a problem hiding this comment.
Fixed. MaxLen now documents that the limit is enforced by Offer and that Push does not check it. Push doc updated to say the same.
| // Offer adds o to the heap. If the heap has a maximum size and is at | ||
| // capacity, the largest element is evicted. If o itself is the largest, | ||
| // it is returned immediately without modifying the heap. | ||
| // | ||
| // For unbounded heaps (MaxLen() == 0), Offer behaves like Push and | ||
| // never evicts. | ||
| // | ||
| // Returns the evicted element and true if an eviction occurred, or the | ||
| // zero value and false if o was simply added. | ||
| func (p *Deheap[T]) Offer(o T) (evicted T, didEvict bool) { | ||
| if p.maxSize == 0 || len(p.items) < p.maxSize { | ||
| p.Push(o) | ||
| return evicted, false | ||
| } | ||
| evicted = p.PushPopMax(o) | ||
| return evicted, true | ||
| } |
There was a problem hiding this comment.
Offer returns didEvict=true whenever the heap is at capacity, but in the “rejected” case (o is >= current max) no element is actually evicted and the heap is unchanged (since PushPopMax returns o without modifying the heap). The current doc wording (“largest element is evicted”, “true if an eviction occurred”) is therefore inaccurate; consider documenting didEvict as “heap was at capacity; returned value is the element not retained” (evicted or rejected), or change the return semantics to distinguish eviction vs rejection.
There was a problem hiding this comment.
Fixed. The return-value doc now reads: 'Returns the element not retained and true if the heap was at capacity (o was evicted to make room, or o itself was rejected as the largest); returns the zero value and false if o was simply added.'
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
…ative tests - Makefile: add cover (profile + HTML) and covercheck (90% threshold) targets - deheap_test.go: TestV1PushPopMaxSingle covers the l==2 else branch where o < items[0] on a single-element heap (h.Swap + h.Pop path was unreachable) - deheap_test.go, ordered_test.go: TestV1VerifyInvalid / TestOrderedVerifyInvalid exercise all four return-false branches in valid/orderedValid with crafted invalid heaps (max parent < min child, max child < min parent, min grandchild < min grandparent, max grandchild > max grandparent)
MaxLen doc now states the limit is enforced by Offer, not Push. Push doc notes it bypasses MaxLen. Offer return-value doc corrects "eviction occurred" to cover both the eviction and rejection cases (didEvict=true whenever at capacity).
Summary
iter.Seq[T]iterators that consume the heap in sorted order; early break leaves the heap valid with remaining elementsNewBounded,FromBounded,MaxLen, andOffer— capacity-limited heaps that evict the largest element when full, composing directly onPushPopMaxiter.Seq[T]; CI matrix updated to 1.23 + 1.24{(PushPop) and}(PushPopMax) commandsTest Plan
go test -race -count=3 ./...passesgo vet ./...cleanBenchmarkOrderedGenericPushPopshows 0 allocs/opFuzzOrderedPushPopandFuzzV1PushPoprun 30s each without failures