From 79ffa4b732198dd5898e3c5746489920e9f30c97 Mon Sep 17 00:00:00 2001 From: Yihong Zhang Date: Mon, 3 Aug 2026 21:09:42 +0000 Subject: [PATCH 1/2] Fix spurious sortedness assert in Subset::intersect (#971) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- CHANGELOG.md | 1 + core-relations/src/offsets/mod.rs | 16 ++++++++++++--- core-relations/src/offsets/tests.rs | 32 +++++++++++++++++++++++++++++ 3 files changed, 46 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 171150ebf..0db772d56 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,7 @@ - Report full source file paths in egglog span and error messages. - Fix seminaive matching after nested containers rebuild in place by propagating dirty container ids through parent containers. - Fix multi-column secondary index rebuilds so each value's rows come back sorted by row id, and make all rebuild paths (serial, parallel, and bulk) record a row once even when its value repeats across covered columns (#914). +- Fix a spurious `slice is not sorted` debug assertion in `Subset::intersect` when galloping through a sparse subset that is being compacted in place, which panicked debug builds on some sparse/sparse intersections (#971). - Render nullary AST calls without a trailing space, e.g. (foo) instead of (foo ). - Escape `"` and `\` when displaying string literals so printed/serialized programs round-trip through the parser. - Add a BigRat to-i64 primitive for integral rationals. diff --git a/core-relations/src/offsets/mod.rs b/core-relations/src/offsets/mod.rs index 565265a95..cb5485569 100644 --- a/core-relations/src/offsets/mod.rs +++ b/core-relations/src/offsets/mod.rs @@ -468,6 +468,12 @@ impl Subset { } else if cur_len > other_len { // other is much smaller: iterate other and gallop in cur. // O(other_len * log(cur_len / other_len)) vs O(cur_len) for retain. + // NB: the result is compacted into `cur` in place, so `cur` as a + // whole is transiently unsorted while the loop runs. `write` only + // ever advances to `ci` (a match at `found >= ci` writes to + // `write <= found` and then sets `ci = found + 1`), so the + // *suffix* `cur.0[ci..]` is always untouched, and searching it is + // equivalent to searching all of `cur` from `ci`. let mut write = 0usize; let mut ci = 0usize; #[allow(clippy::needless_range_loop)] @@ -476,15 +482,19 @@ impl Subset { break; } let target = other_inner[oi]; - let result = cur.slice().scan_for_offset(ci, target); + debug_assert!(write <= ci); + // SAFETY: `cur.0[ci..]` is an unmodified suffix of the + // original sorted vector, per the note above. + let suffix = unsafe { SortedOffsetSlice::new_unchecked(&cur.0[ci..]) }; + let result = suffix.scan_for_offset(0, target); match result { Ok(found) => { cur.0[write] = target; write += 1; - ci = found + 1; + ci += found + 1; } Err(next_ci) => { - ci = next_ci; + ci += next_ci; } } } diff --git a/core-relations/src/offsets/tests.rs b/core-relations/src/offsets/tests.rs index 0014a8c6b..68673d5fd 100644 --- a/core-relations/src/offsets/tests.rs +++ b/core-relations/src/offsets/tests.rs @@ -73,6 +73,10 @@ fn intersect() { Vec::from_iter(0..5), Vec::from_iter(0..100), // 20x skew → galloping path Vec::from_iter((0..100).filter(|x| x % 7 == 0)), + // Both sides Sparse (i.e. non-contiguous) with >4x skew: galloping in + // `cur` while compacting it in place. + Vec::from_iter((0..6).chain(11..22)), + vec![13, 20, 21], ]; let all_elts: Vec<&Vec> = elts.iter().chain(skewed.iter()).collect(); @@ -133,3 +137,31 @@ fn iter_bounded() { let expected = Vec::from_iter((2..12).map(|x| RowId::new(x * 2))); assert_eq!(got, expected); } + +#[test] +fn intersect_sparse_sparse_skewed() { + // Regression test for #971. `cur` must be Sparse and >4x longer than + // `other` (which must also be Sparse) to reach the "gallop in cur" branch + // of `Subset::intersect`. + let cur_rows: Vec = (0..6).chain(11..22).collect(); + let other_rows: Vec = vec![13, 20, 21]; + + let mut cur = Subset::empty(); + for row in &cur_rows { + cur.add_row_sorted(o(*row)); + } + let mut other = Subset::empty(); + for row in &other_rows { + other.add_row_sorted(o(*row)); + } + assert!(matches!(cur, Subset::Sparse(..))); + assert!(matches!(other, Subset::Sparse(..))); + + cur.intersect( + other.as_ref(), + &with_pool_set(|pool_set| pool_set.get_pool()), + ); + let mut got = Vec::new(); + cur.offsets(|row| got.push(row.index())); + assert_eq!(got, other_rows); +} From 62498331812c7c44e37c574a6620dd6645e4ff2c Mon Sep 17 00:00:00 2001 From: Yihong Zhang Date: Fri, 7 Aug 2026 12:29:26 -0700 Subject: [PATCH 2/2] remove changelog entry --- CHANGELOG.md | 1 - CLAUDE.md | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0db772d56..171150ebf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,7 +14,6 @@ - Report full source file paths in egglog span and error messages. - Fix seminaive matching after nested containers rebuild in place by propagating dirty container ids through parent containers. - Fix multi-column secondary index rebuilds so each value's rows come back sorted by row id, and make all rebuild paths (serial, parallel, and bulk) record a row once even when its value repeats across covered columns (#914). -- Fix a spurious `slice is not sorted` debug assertion in `Subset::intersect` when galloping through a sparse subset that is being compacted in place, which panicked debug builds on some sparse/sparse intersections (#971). - Render nullary AST calls without a trailing space, e.g. (foo) instead of (foo ). - Escape `"` and `\` when displaying string literals so printed/serialized programs round-trip through the parser. - Add a BigRat to-i64 primitive for integral rationals. diff --git a/CLAUDE.md b/CLAUDE.md index 5ed71a636..f85a62e85 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -18,4 +18,4 @@ When you edit the files, make sure to respect the following: - When running tests, always use the `--release` mode. Alternatively, you can also run `make test`. - If your change is performance-critical, use `script/bench.py` as the ground truth to evaluate the performance impact. - Keep your documentation concise and avoid duplicate information. The `tidy-diff-docs` skill (`.claude/skills/tidy-diff-docs/`) cleans the doc and code comments in a diff down to the caller-facing contract. -- Update CHANGELOG.md with a concise bullet when you make major changes (e.g., breaking changes or new features added) in the codebase. +- Update CHANGELOG.md with a concise bullet when you make major changes (e.g., breaking changes or new features added) in the codebase. Don't update CHANGELOG.md if it's just a bug fix or if it's not a user-facing change. However, major performance improvements (e.g., using a new join algorithm or introducing a new sort of indices) should be documented.