feat: enlarge indice size limit for sparse vectors#229
Merged
Conversation
Collaborator
Author
|
@greptile |
9562156 to
bc33e81
Compare
iaojnh
approved these changes
Mar 16, 2026
Collaborator
Author
|
@greptile |
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.
Greptile Summary
This PR enlarges the maximum non-zero element count for sparse vectors from 4096 to 16384 across both the flat and HNSW sparse index implementations, and fixes a pre-existing off-by-one inconsistency where some code paths used
>=(rejecting the boundary value) while others used>(accepting it).Key changes:
kSparseMaxDimSize/PARAM_FLAT_SPARSE_MAX_DIM_SIZEraised from4096→16384in all three constant definitions (constants.h,hnsw_sparse_entity.h,flat_sparse_utility.h)HnswSparseStreamer::add_impl,add_with_id_impl, andHnswSparseBuilderEntity::add_vectorcorrected from>=to>, making them consistent with the flat-sparse paths and the doc-layer validationsparse_indices.size() > kSparseMaxDimSizeguards added toDoc::validatefor bothSPARSE_VECTOR_FP16andSPARSE_VECTOR_FP32paths — previously validation at the document layer was missing entirelyVectorQuery::validatebyte-level check updated from>=to>, keeping it semantically aligned with the index-layer checks16385as the over-limit sentinel, but missing: a test asserting that exactly16384indices now passes (boundary regression risk), and tests for the two newDoc::validateguardsConfidence Score: 4/5
>=checks that caused the off-by-one inconsistency have been corrected to>, and the logic across doc-layer validation and the core index paths is now aligned. The only concerns are missing boundary test coverage (exactkSparseMaxDimSizecase) and no tests for the newly addedDoc::validateguards, which reduce confidence slightly.tests/db/index/common/doc_test.cc— the boundary case for exactly 16384 indices is not tested, and the newDoc::validatesparse size guards have no coverage at all.Important Files Changed
> kSparseMaxDimSize) toDoc::validatefor both FP16 and FP32 sparse types, and updatesVectorQuery::validateboundary from>=to>. Logic is correct and consistent, but the new validation paths lack test coverage.>=to>), and tests for the newDoc::validatesparse size checks.kSparseMaxDimSizefrom 4096 to 16384. Straightforward constant update, consistent with the other two limit constants.add_implandadd_with_id_implupdated from>=to>and receive improved error messages. Boundary check is now consistent with the builder and doc-layer validation.add_vectorupdated from>=to>and error message improved. Now consistent with streamer paths.add_implandadd_with_id_implto include the actual limit value. Both already used>so no semantic change here.PARAM_FLAT_SPARSE_MAX_DIM_SIZEfrom 4096 to 16384. Straightforward constant update.kSparseMaxDimSizefrom 4096 to 16384. Consistent with the algorithm-layer constants.Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[Sparse Vector Insert / Query] --> B{Layer} B --> C[Doc Layer\ndoc.cc] B --> D[HNSW Sparse\nStreamer / Builder] B --> E[Flat Sparse\nStreamer] C --> C1["Doc::validate\nsparse_indices.size() > kSparseMaxDimSize\n(constants.h → 16384)"] C --> C2["VectorQuery::validate\nbyte_size > kSparseMaxDimSize × sizeof(uint32_t)\n(constants.h → 16384)"] D --> D1["HnswSparseStreamer::add_impl\nsparse_count > kSparseMaxDimSize\n(hnsw_sparse_entity.h → 16384)"] D --> D2["HnswSparseStreamer::add_with_id_impl\nsparse_count > kSparseMaxDimSize"] D --> D3["HnswSparseBuilderEntity::add_vector\nsparse_count > kSparseMaxDimSize"] E --> E1["FlatSparseStreamer::add_impl\nsparse_count > PARAM_FLAT_SPARSE_MAX_DIM_SIZE\n(flat_sparse_utility.h → 16384)"] E --> E2["FlatSparseStreamer::add_with_id_impl\nsparse_count > PARAM_FLAT_SPARSE_MAX_DIM_SIZE"] C1 --> F[InvalidArgument] C2 --> F D1 --> G[IndexError_InvalidValue] D2 --> G D3 --> G E1 --> G E2 --> GComments Outside Diff (1)
tests/db/index/common/doc_test.cc, line 1258-1271 (link)Missing exact-boundary test case
The test only checks a clearly over-limit value (16385) and a clearly under-limit value (3 bytes). Because the semantic of the boundary changed from
>=(rejected) to>(accepted) in this PR, there is no assertion that verifies the new allowed boundary — exactlykSparseMaxDimSize(16384) indices (= 65536bytes) must now pass.Without this test, a future regression that accidentally reverts the
>to>=inVectorQuery::validatewould not be caught.Consider adding:
Last reviewed commit: c2c1aee