Fix deleted elements order on merge #24
Conversation
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Pull request overview
This pull request fixes the ordering of deleted and non-deleted elements during segment merge operations to maintain proper FIFO semantics. The fix ensures that when equal-valued elements are merged, non-deleted elements appear before deleted ones.
Changes:
- Modified
mergeSegmentslogic to prioritize non-deleted elements when values are equal - Swapped parameter order in
mergeSegmentscalls to clarify that seg2 is older than seg1 for FIFO ordering - Added test coverage for deleted element ordering and delete stability
- Updated tests to use local random sources instead of deprecated global rand.Seed
- Improved documentation with proper LaTeX math notation
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| segment.go | Modified merge logic to handle equal elements with different deletion states; added comment clarifying parameter order for FIFO |
| segment_test.go | Added test case and validation for deleted element ordering |
| bwarr.go | Updated all mergeSegments calls to use swapped parameter order for consistency |
| bwarr_test.go | Added TestBWArr_DeleteStability test; updated random tests to use local sources; minor code style improvements |
| README.md | Updated to use LaTeX math notation and improved wording |
| bwarr_bench_test.go | Fixed grammar in comment ("as many" instead of "as much") |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| bwa := New(stabValCmp, elemsNum) | ||
| for i := range elemsNum { | ||
| bwa.Insert(stabVal{val: rand.Intn(7), seq: i + 1}) |
There was a problem hiding this comment.
The global rand.Intn function should be replaced with a local random source for test determinism and thread safety, similar to other test functions in this file (e.g., TestBWArr_AscendRandom at line 760). This is especially important since this test is marked with t.Parallel(). Consider creating a local random source: r := rand.New(rand.NewSource(seed)) and using r.Intn(7).
| segs := make(map[int]int, elemsNum) | ||
| iter := func(e stabVal) bool { | ||
| ps := seqs[e.val] | ||
| ps := segs[e.val] |
There was a problem hiding this comment.
The variable name change from 'seqs' to 'segs' appears to be unclear or potentially incorrect. The variable stores the previously seen sequence number for each value, so 'seqs' (sequences) seems more descriptive than 'segs' (which could be confused with segments). Consider reverting to 'seqs' or using a more explicit name like 'prevSeqs' or 'lastSeqByVal'.
| if seg.deleted[i-1] { | ||
| assert.False(t, seg.deleted[i], "At index %d: equal elements, but deleted comes before non-deleted", i) | ||
| } |
There was a problem hiding this comment.
The validation logic appears to be incorrect. At this point in the code (after line 459-462), we know that seg.deleted[i] is false because deleted elements are skipped via 'continue'. Therefore, the assertion on line 466 will always pass and doesn't actually validate the constraint. The check should instead verify that if seg.deleted[i-1] is true (previous is deleted) and elements are equal, it's an error - because non-deleted elements should come before deleted ones for equal values. Consider restructuring: remove the negation in the condition or move this check outside the non-deleted loop to properly validate the ordering constraint.
| if seg.deleted[i-1] { | |
| assert.False(t, seg.deleted[i], "At index %d: equal elements, but deleted comes before non-deleted", i) | |
| } | |
| assert.False(t, seg.deleted[i-1], "At index %d: equal elements, but deleted comes before non-deleted", i-1) |
| for i < len(seg1.elements) && j < len(seg2.elements) { | ||
| if cmp(seg1.elements[i], seg2.elements[j]) < 0 { | ||
| cmpRes := cmp(seg1.elements[i], seg2.elements[j]) | ||
| if cmpRes < 0 || (cmpRes == 0 && !seg1.deleted[i]) { |
There was a problem hiding this comment.
The merge logic prioritizes non-deleted elements from seg1 (newer) when values are equal, but the comment states "seg2 should be older than seg1" for FIFO order. When both seg1 and seg2 have equal non-deleted elements, choosing seg1 would place newer elements before older ones, which appears to contradict FIFO semantics. Consider verifying that TestBWArr_DeleteStability passes, as it expects FIFO deletion order. If FIFO among non-deleted duplicates is required, the condition might need to be: (cmpRes == 0 && !seg1.deleted[i] && seg2.deleted[j]) to only prefer seg1 when seg2 is deleted.
| if cmpRes < 0 || (cmpRes == 0 && !seg1.deleted[i]) { | |
| if cmpRes < 0 || (cmpRes == 0 && !seg1.deleted[i] && seg2.deleted[j]) { |
No description provided.