BitSliceIndexing: find min and max by bit plane, not by row

[tamirms]

Description

BSI.MinMax returns wrong answers for values of mixed sign. With two columns holding -45 and 1, and no concurrency at all, it answers 0, a value that was never stored.

This replaces the row-by-row search with the bit plane descent that roaring64.MinMaxBig already uses, and adds a regression test.

Type of Change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Performance improvement
• Code refactoring
• Documentation update
• Test improvements
• Build/CI changes

Changes Made

What was changed?

• MinMax now narrows the candidate columns one bit plane at a time, highest plane first, and decodes only the single column that survives.
• minOrMax, which had no other caller, is removed.
• MinMax intersects the found set with the existence bitmap, so a column carrying no value can no longer contribute a zero.
• Added TestMinMaxMixedSign and BenchmarkMinMax.

Why was it changed?

• The old path divided the found set into one batch per core and had each goroutine rebuild every column's value bit by bit, carrying a running minimum or maximum. That running comparison mishandles mixed signs.
• Because the answer depended on how many batches the work was divided into, the same data gave different results on machines with different core counts.
• TestMinMaxWithNil already catches this, but only when its clock-seeded random values land on a failing pattern, roughly once in two hundred runs. That reads as a flaky test rather than a wrong answer, which is how it has survived.

How was it changed?

• Below the sign, two's complement orders the same way as unsigned, so the maximum keeps the columns whose bit is set and the minimum keeps those whose bit is clear. A plane that would leave nothing is skipped, since every remaining candidate then agrees on that bit.
• The sign plane, which exists only in a 64 bit wide BSI, is settled first.
• The parallelism argument is kept for compatibility and ignored, matching roaring64.MinMaxBig.

Testing

TestMinMaxMixedSign is added and fails on master with expected: 1, actual: 0.

Checked against a brute force scan over 20000 random cases covering three bit widths, both signs, cardinalities from 1 to 40, found sets that are nil or a random subset, found sets containing a column with no value, and three values of parallelism.

go test ./...            # root, BitSliceIndexing and roaring64 all pass
go test -tags appengine ./...
GOOS=linux GOARCH=386 go build ./...    # also arm, arm64, ppc64

Formatting

gofmt -s -l . reports nothing. go vet ./... reports only the pre-existing ReadFrom signature complaint on master.

Fuzzing

The step above cannot be run as written: there is no FuzzSmat target on master. smat.go refers to a smat_fuzz_test.go that is not in the repository, so go test -fuzz=FuzzSmat matches nothing and simply runs the ordinary tests. What does run is the smat corpus, which passes:

go test -tags=gofuzz -run 'TestGenerateSmatCorpus|TestSmatHits'
ok  github.com/RoaringBitmap/roaring/v2

Performance Impact

BenchmarkMinMax is added. Apple M1, median of three.

Before:

BenchmarkMinMax/non-negative/planes7/rows100-10      39931     9160 ns/op    1456 B/op   23 allocs/op
BenchmarkMinMax/non-negative/planes7/rows10000-10     1294   273206 ns/op   41940 B/op   23 allocs/op
BenchmarkMinMax/signed/planes64/rows100-10            6793    52196 ns/op    1456 B/op   23 allocs/op
BenchmarkMinMax/signed/planes64/rows10000-10           420   877593 ns/op   41938 B/op   23 allocs/op

After:

BenchmarkMinMax/non-negative/planes7/rows100-10     311082     1147 ns/op    1368 B/op   36 allocs/op
BenchmarkMinMax/non-negative/planes7/rows10000-10    33345    10988 ns/op   28312 B/op   36 allocs/op
BenchmarkMinMax/signed/planes64/rows100-10           43772     8061 ns/op   13856 B/op  207 allocs/op
BenchmarkMinMax/signed/planes64/rows10000-10          6210    60001 ns/op   31032 B/op  150 allocs/op

Performance Analysis

• Between 6.5 and 25 times faster across the four shapes.
• The old cost is proportional to rows times planes in random bitmap lookups, plus one goroutine and slice per core and a channel per call. The new cost is proportional to the plane count in bulk bitmap operations, over a candidate set that shrinks as it descends.
• Memory falls at ten thousand rows, from 41940 to 28312 bytes and from 41938 to 31032 bytes.
• The trade is a hundred rows across 64 planes, which rises from 1456 to 13856 bytes. Every plane intersection builds a bitmap however few rows remain, so the floor is set by the plane count rather than by the data.

Breaking Changes

No signature changes. Two behaviours differ:

• parallelism is now ignored. Callers passing a value get the same answer as before, only correct, and there is nothing left to divide.
• A found set containing columns with no value no longer lets those columns contribute a zero. The empty result still returns Min64BitSigned for MAX and Max64BitSigned for MIN, as before.

Related Issues

None filed. I could not find an issue or pull request covering this.

Additional Notes

roaring64 fixed the same defect in passing. #544 rewrote MinMaxBig to prune by sign and value bit planes, and its checklist marks it a performance and refactoring change with the bug fix box unchecked, so the correctness repair went unrecorded. I confirmed the 64 bit path is now correct under the same fixture that breaks this one, with zero disagreements in 20000 trials. Its superseded minOrMax is still present and unreachable at roaring64/bsi64.go, which I have left alone as out of scope here.

🤖 Generated with Claude Code

[guymolinari]

I'm fine with this Daniel when you are ready to merge