fix(filter): handle constant vectors in CBloomFilter/CRoaring/Cbitmap#25720
fix(filter): handle constant vectors in CBloomFilter/CRoaring/Cbitmap#25720cpegeric wants to merge 3 commits into
Conversation
…matrixorigin#25621) The three cgo membership filters confused a vector's logical row count with its physical element count for CONSTANT vectors. A constant stores a single physical value (none for const-null) but reports Length() = row count, so passing the logical length to C over the 1-element (or empty) physical buffer read past the buffer — false negatives ([1,0,0] instead of [1,1,1]) — or panicked on the empty const-null buffer (&data[0]). This affects PK/partition filtering and IVF_FLAT bloom ops, where a single-value predicate folds to a constant vector. Process only the PHYSICAL element(s) and broadcast the result to all logical rows: - bloomfilter/cbloomfilter.go: vecPhysCount (1 for non-null const, 0 for const-null, Length() otherwise), emitConstNull (all-zero result, every row NULL), broadcastResults (expand the single result across rows). All six fixed/varlena test/add/testAndAdd helpers short-circuit const-null, guard nitem==0 (also fixes a latent panic on regular length-0 fixed vectors), call C with the physical count, then broadcast. - docfilter/croaring.go: vecFixedArgs computes the physical nitem; new finalizeVecResults broadcasts for constants. cbitmap.go shares both (its TestVector rewired). Build-from-constant now contains the value. Non-const vectors take a byte-identical path. Tests: TestCBloomFilterConstVector (const int64/varchar, non-null + null, across Test/Add/TestAndAdd), TestCRoaringConstVector + TestCbitmapConstVector (present/absent/null probes, build-from-const). All fail on the original code (verified via git-revert) and pass under -race. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
…rigin#25618) matrixorigin#25618 (fixed-width vector APIs panic with "index out of range [0] with length 0" on a zero-length vector) is already resolved by the constant-vector fix (matrixorigin#25621): an empty vector has physical count 0, so the nitem==0 guards short- circuit before indexing &fixedData[0]/&results[0]. Add the regression test to lock it in — TestVector returns empty, AddVector/TestAndAddVector are no-ops that never invoke the callback, for both fixed-width and variable-length types. Verified: the test panics on the pre-fix code and passes with the matrixorigin#25621 guards. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
The stateless TestVector/AddVector fixes look correct, but TestAndAddVector does not preserve the logical per-row semantics for a constant vector.
Reproduced on head 1a3664c with both INT64 and VARCHAR. On a fresh filter, a regular vector containing [99, 99, 99] reports callback states [false, true, true]: the first logical row inserts the value, so subsequent duplicate rows observe it. The equivalent constant vector (value 99, logical length 3) reports [false, false, false].
The C test-and-add implementation is intentionally stateful and processes elements sequentially. This PR calls it once for the single physical constant value, then uses broadcastResults to copy the pre-insert result to every logical row. Broadcasting is valid for TestVector, but not for TestAndAddVector because the filter state changes after the first logical row. This can let repeated logical values all appear new to a deduplication callback and also makes constant and flat representations of the same rows observably different.
Please give TestAndAddVector state-aware constant handling and add fixed-width plus varlena equivalence tests against a flat repeated-value vector. The current test only asserts [false, false, false], which bakes in the incorrect behavior.
3 participants
What type of PR is this?
Which issue(s) this PR fixes:
issue #25621 #25618
What this PR does / why we need it:
The three cgo membership filters confused a vector's logical row count with its physical element count for CONSTANT vectors. A constant stores a single physical value (none for const-null) but reports Length() = row count, so passing the logical length to C over the 1-element (or empty) physical buffer read past the buffer — false negatives ([1,0,0] instead of [1,1,1]) — or panicked on the empty const-null buffer (&data[0]). This affects PK/partition filtering and IVF_FLAT bloom ops, where a single-value predicate folds to a constant vector.
Process only the PHYSICAL element(s) and broadcast the result to all logical rows:
Non-const vectors take a byte-identical path. Tests: TestCBloomFilterConstVector (const int64/varchar, non-null + null, across Test/Add/TestAndAdd), TestCRoaringConstVector + TestCbitmapConstVector (present/absent/null probes, build-from-const). All fail on the original code (verified via git-revert) and pass under -race.