Fix spurious slice is not sorted assert in Subset::intersect (#971) - #973
Open
yihozhang wants to merge 1 commit into
Open
Fix spurious slice is not sorted assert in Subset::intersect (#971)#973yihozhang wants to merge 1 commit into
slice is not sorted assert in Subset::intersect (#971)#973yihozhang wants to merge 1 commit into
Conversation
The `(Sparse, Sparse)` "gallop in cur" branch of `Subset::intersect` compacts the intersection into `cur` in place, but re-derived a `SortedOffsetSlice` over the *whole* vector on each iteration via `cur.slice()`. Once the first match has been written to `cur.0[write]`, the region between `write` and `ci` still holds stale originals, so the vector as a whole is transiently unsorted and `slice()`'s `debug_assert!` fires on the next iteration. The searches themselves were already correct: `write <= ci` always holds and `scan_for_offset` never reads below its `start`, so every read lands in the untouched suffix. Only the assertion was wrong, so release builds (with the assertion compiled out) produced correct intersections. Search `cur.0[ci..]` — the untouched suffix — instead of all of `cur`, and make the returned offsets absolute again. Same complexity, no extra work in the hot path. The existing `intersect` test corpus missed this because `add_row_sorted` collapses contiguous row sets to `Dense`, so its skewed pairs never put two `Sparse` subsets into this branch. Add a targeted regression test plus two entries to the exhaustive corpus that do. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #973 +/- ##
==========================================
+ Coverage 86.59% 86.63% +0.04%
==========================================
Files 95 95
Lines 29676 29678 +2
==========================================
+ Hits 25699 25713 +14
+ Misses 3977 3965 -12 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Merging this PR will not alter performance
Comparing Footnotes
|
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.
Fixes #971.
Problem
Subset::intersect's(Sparse, Sparse)"other is much smaller: iterate other and gallop in cur" branch compacts the intersection intocurin place, but re-derived aSortedOffsetSliceover the whole vector on every iteration:Once the first match is written to
cur.0[write], the region betweenwriteandcistill holds stale originals, socuras a whole is transiently unsorted andslice()'sdebug_assert!fires on the next iteration.That is exactly the panic in #971.
curwas[0,1,2,3,4,5,11..=21](17 rows) and the first match wasRowId(13), written over index 0:Despite the identical assertion message, this is a different bug from #911/#914 — that one was in
hash_index; this one is inoffsets, reached viaDatabase::process_constraints.This was only the assertion, not the results
write <= cialways holds (a match atfound >= ciwrites towrite <= found, then setsci = found + 1) andscan_for_offsetnever reads below itsstart, so every read already landed in the untouched suffix. Confirmed empirically: with the assertion compiled out, the pre-fix code produces correct intersections. So release builds were unaffected; debug builds panicked.Fix
Search
cur.0[ci..]— the suffix that is guaranteed untouched — instead of all ofcur, and make the returned offsets absolute again.scan_for_offset's duplicate back-walk floors atstart, which becomes suffix-relative0, so the semantics are unchanged. Same complexity, no extra work in the hot path.Testing
The existing
offsets::tests::intersectcorpus missed this becauseadd_row_sortedcollapses contiguous row sets toDense, so its skewed pairs never put twoSparsesubsets into this branch. Added:offsets::tests::intersect_sparse_sparse_skewed, a targeted regression test that reproduces Bug: debug_assert failure: slice is not sorted #971's slice byte-for-byte in ~20 lines with no external dependencies.intersectcorpus, so all-pairs coverage now reaches this branch.Verified at
53b9721:cargo test --workspace: 1132 passed, 0 failed (including the 792-test integration suite).cd rust && cargo test -- repro— passes against the patched egglog, as does the rest of that suite.cargo fmt --checkandcargo clippy --all-targets -- -D warningsclean.I also audited the other
SortedOffsetVector::slice()/scan_for_offsetcall sites; this was the only one whose receiver can be mid-mutation. One adjacent fragility, left alone as it is not a live bug:binary_search_from's duplicate back-walk is bounded byfound > 0rather thanfound > start, so it can return an index belowstart; at the solestart != 0call site the values make that unreachable.🤖 Generated with Claude Code