Widen CSR nnz-offset to 64-bit (offset_t) so one segment can hold >2.1B nnz - #54
Conversation
940a72c to
8019fcb
Compare
|
could you benchmark on 8.8M? |
| std::string("CSR row count exceeds 32-bit doc-id range: ") + | ||
| file_path); | ||
| } | ||
| if (nnz > std::numeric_limits<offset_t>::max()) { |
There was a problem hiding this comment.
nnz is already int64_t and offset_t is int64_t. remove
There was a problem hiding this comment.
Removed the guard (kept the meaningful num_rows > idx_t::max())
| std::string("CSR row count exceeds 32-bit doc-id range: ") + | ||
| file_path); | ||
| } | ||
| if (nnz > std::numeric_limits<offset_t>::max()) { |
| const auto indptr = narrow<idx_t>(wide_indptr, "indptr", interchange_path); | ||
| write_or_throw(out, indptr.data(), indptr.size() * sizeof(idx_t), | ||
| const auto indptr = | ||
| narrow<offset_t>(wide_indptr, "indptr", interchange_path); |
There was a problem hiding this comment.
narrow<offset_t> on an int64_t vector range-checks nothing and copies (rows+1)*8 bytes — ~1.1 GB at 138M rows. write wide_indptr directly?
There was a problem hiding this comment.
Write wide_indptr (already int64) directly; kept a cheap in-place negative-offset check (no allocation) so throws_on_negative_indptr still holds.
| // on itemsize, not view.format (NumPy leaves format null for some int | ||
| // dtypes, and strcmp(nullptr, ...) would crash). | ||
| const char* fmt = view.format; | ||
| const bool int_fmt = fmt == nullptr || strcmp(fmt, "i") == 0 || |
There was a problem hiding this comment.
a float64 buffer with a null format would then be accepted on itemsize alone and read as int64. Have you verified numpy leaves format null under PyBUF_FORMAT? reject null instead?
There was a problem hiding this comment.
Now rejects null/non-int format; dispatches on explicit 'i'/'l'/'q'
| // Round-trips a small corpus whose terminal offset fits int32; the point is that | ||
| // indptr survives serialize/deserialize/mmap as 64-bit words, exercising the | ||
| // widened read_padded<offset_t>/read_array<offset_t> format path. | ||
| TEST(OffsetWidth, IndptrRoundTripsAsSixtyFourBit) { |
There was a problem hiding this comment.
comment says serialize/deserialize/mmap, but the test only calls add_vectors and reads indptr_data(). serialize it?
There was a problem hiding this comment.
Now actually serializes → deserialize and mmap_deserialize, asserting indptr matches as 64-bit across all three
| // must keep the full 64-bit count rather than wrapping negative. Disabled by | ||
| // default because the single indices/values buffer needs ~6.5 GB; run manually | ||
| // on a large host with --gtest_also_run_disabled_tests. | ||
| TEST(OffsetWidth, DISABLED_CrossesInt32Boundary) { |
There was a problem hiding this comment.
this only exercises map_vectors. With clusters/CSC/off[]/id-map all still int32, has a >2.1B-nnz corpus actually been built and searched end to end?
There was a problem hiding this comment.
Rewrote the DISABLED_ test to add → build → search a >2³¹-nnz corpus on a SeismicIndex (real clustering/forward-index/search path)
Signed-off-by: Zirui Song <zrsong@amazon.com>
8019fcb to
0d4a3d3
Compare
|
Sure: |
Description
The forward-index CSR
indptr(cumulative-nnz prefix sum) was 32-bit (idx_t) everywhere, so it wraps past the ~2.1-billionth nnz — capping a single segment well below a full MS MARCO V2 corpus (138M docs / 28.7B nnz).This adds a distinct
offset_t = int64_t(intypes.h, besideidx_t) for CSR nnz-offsets only, and widens everyindptrwriter/reader in lockstep: theSparseVectorsstorage root, theIndexAPI and all overrides, the native.mcsrlayout + reader/writer, the build-time corpus scans, the SIMD summary kernels, and the disk/mmap serialization. The two int32 nnz caps are removed.Kept narrow on purpose (offsets-only): doc-ids, labels, term-ids, per-row nnz counts, cluster/centroid CSC, the per-block on-disk
off[]/doc_id[](uint32) and its INT32_MAX cap, and the id-map's int32 external ids. Only the O(N_docs) indptrgrows (4→8 B/entry, ~+553 MB over 138M docs); the O(nnz)indices/values` arrays are untouched.GPU build (off by default) stays int32 — cuSPARSE's CSR API is itself 32-bit — but now fails closed with a clear error on a >INT32_MAX corpus instead of silently truncating.
Testing
offset_width_test.cpp: type invariants, an 8-byte-indptr native-layout regression guard, and aDISABLED_>2³¹-nnz acceptance test (the real overflow gate; needs a large-RAM host).The Python bindings (SWIG) are widened too: the indptr typemap accepts a 32- or 64-bit int buffer and widens to offset_t, and the buffer views are zero-initialised so a wrong dtype raises TypeError instead of segfaulting. The Python bindings build and their pytest suite run in CI (green).
Issues Resolved
#53
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.