Skip to content

[Sparse ANN] [Native] Add support for neural-sparse-cpp's wider 64-bit nnz-offset - #2001

Merged
chishui merged 1 commit into
opensearch-project:mainfrom
zirui-song-18:sparse-ann-int64-csr-offset
Sep 11, 2026
Merged

chishui merged 1 commit into
opensearch-project:mainfrom
zirui-song-18:sparse-ann-int64-csr-offset

Conversation

@zirui-song-18

Copy link
Copy Markdown
Contributor

Description

neural-sparse-cpp #54 widened the CSR forward-index offset from 32-bit to a new offset_t = int64_t so a single segment can hold more than ~2.1B non-zeros. The 32-bit cap was hit by large corpora — e.g. MS MARCO V2 (138M docs, ~28.7B nnz) overflows a signed-int32 indptr and cannot be built as a single force-merged segment on the native engine.

This PR bumps the submodule to the merged nsparse commit and adapts the plugin's JNI + Java side so the wider offset flows through end to end. Doc-ids, labels, term-ids, and the per-block on-disk structures deliberately stay 32-bit (nsparse idx_t) — only the CSR nnz-offset widens.

Changes

  • Submodule bump — jni/external/neural-sparse-cpp 354c756 → da0d43c ("Widen the CSR offset to 64 bit", Widen CSR nnz-offset to 64-bit (offset_t) so one segment can hold >2.1B nnz neural-sparse-cpp#54, now merged to nsparse main).
  • Streaming path (jni/src/common.h) — the off-heap CSR indptr accumulator widens from std::vector<int32_t> to std::vector<int64_t>, so a segment's cumulative nnz can exceed INT32_MAX. Each per-flush indptr is still a relative int32 array (reset to 0 per flush, bounded by the streaming flush limit) and is widened before the cumulative offset is added — so the transferVectors/insertToIndex JNI signatures are unchanged.
  • Insert / query (jni/src/nsparse_wrapper.cpp) — hands add_with_ids and search the indptr as nsparse::offset_t (was idx_t) on both the insert and the single-query paths.
  • Disk .csr writer (CsrSparseVectorsFile.java) — emits int64 indptr entries (8 bytes/row, matching nsparse read_csr); the id file's external_ids stay int32 (idx_t). Removes the now-obsolete INT_MAX nnz cap. CsrSparseVectorsFileTests updated for the 8-byte indptr (reads + byte-offset math); all 11 tests pass.

Cost (measured on MS MARCO V1, 8.8M docs, native seismic_sq, 1 segment)

Widening the offset is effectively free — only the indptr array grows 4→8 bytes/row:

metric int32 int64 Δ
disk size (1 segment) 41.361 GB 41.406 GB +0.11% (~45 MB)
peak JVM RSS 105,617 MB 105,616 MB ~0
build time 2227 s 2223 s ~0

Related Issues

Resolves #[Issue number to be closed when this PR is merged]

Check List

  • New functionality includes testing.
  • New functionality has been documented.
  • API changes companion pull request created.
  • Commits are signed per the DCO using --signoff.
  • Public documentation issue/PR created.

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.

@github-actions

github-actions Bot commented Sep 10, 2026 •

Copy link
Copy Markdown

PR Code Analyzer ❗

AI-powered 'Code-Diff-Analyzer' found issues on commit d29f374.

⛔ Hard block: Issues at High severity or above will block this PR from merging.

PathLineSeverityDescription
jni/external/neural-sparse-cpp1highGit submodule pointer updated from 354c7567e to da0d43c31. This is a native C++ dependency change that cannot be verified from the diff alone — the new commit could introduce arbitrary native code (data exfiltration, backdoors, memory corruption) that executes within the JNI boundary. Maintainers must independently verify the new submodule commit contains only the expected int32→int64 offset_t widening changes and nothing else.

The table above displays the top 10 most important findings.

Total: 1 | Critical: 0 | High: 1 | Medium: 0 | Low: 0


Pull Requests Author(s): Please update your Pull Request according to the report above.

Repository Maintainer(s): You can bypass diff analyzer by adding label skip-diff-analyzer after reviewing the changes carefully, then re-run failed actions. To re-enable the analyzer, remove the label, then re-run all actions.


⚠️ Note: The Code-Diff-Analyzer helps protect against potentially harmful code patterns. Please ensure you have thoroughly reviewed the changes beforehand.

Thanks.

@codecov

codecov Bot commented Sep 10, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 84.23%. Comparing base (f15d73a) to head (4155a4d).

Additional details and impacted files
@@            Coverage Diff            @@
##               main    #2001   +/-   ##
=========================================
  Coverage     84.22%   84.23%           
- Complexity     4239     4241    +2     
=========================================
  Files           317      317           
  Lines         14877    14879    +2     
  Branches       2426     2425    -1     
=========================================
+ Hits          12530    12533    +3     
  Misses         1484     1484           
+ Partials        863      862    -1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@chishui chishui left a comment •

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you should update DefaultNativeIndexWriter as well, although it's not used

@zirui-song-18
zirui-song-18 force-pushed the sparse-ann-int64-csr-offset branch 2 times, most recently from 275767c to d29f374 Compare September 10, 2026 09:17
@zirui-song-18

Copy link
Copy Markdown
Contributor Author

you should update DefaultNativeIndexWriter as well, although it's not used

Ack. The streaming path's indptr lives in OffHeapSparseVectorsBuffer (DefaultNativeIndexWriter only orchestrates it), so the guard goes there.

@chishui chishui added the skip-diff-analyzer Maintainer to skip code-diff-analyzer check, after reviewing issues in AI analysis. label Sep 10, 2026
Signed-off-by: Zirui Song <zrsong@amazon.com>
@zirui-song-18
zirui-song-18 force-pushed the sparse-ann-int64-csr-offset branch from d29f374 to 4155a4d Compare September 10, 2026 09:33
@github-actions

Copy link
Copy Markdown

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🧪 PR contains tests
🔒 No security concerns identified
✅ No TODO sections
🔀 No multiple PR themes
⚡ No major issues detected

@github-actions

Copy link
Copy Markdown

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Also reject negative addedNnz in overflow guard

The guard is called before ensureNnzCapacity, but downstream code paths (like
ensureNnzCapacity and System.arraycopy into tokens/values arrays) also use nnzSize +
vectorTokens.length as an int. If addedNnz itself is close to INT_MAX or negative,
this check should also validate that addedNnz >= 0 to avoid pathological inputs
bypassing the check via negative addition.

src/main/java/org/opensearch/neuralsearch/sparse/codec/nativeindex/OffHeapSparseVectorsBuffer.java [114-121]

 static void requirePerFlushNnzFitsInt(int currentNnz, int addedNnz) {
-    if ((long) currentNnz + addedNnz > Integer.MAX_VALUE) {
+    if (addedNnz < 0 || (long) currentNnz + addedNnz > Integer.MAX_VALUE) {
         throw new IllegalStateException(
             "a single streaming flush exceeded Integer.MAX_VALUE non-zeros; flush more often (the "
                 + "cumulative offset is 64-bit, but one flush's relative indptr must fit an int)"
         );
     }
 }
Suggestion importance[1-10]: 3

__

Why: The suggestion is a minor defensive check. In practice addedNnz comes from vectorTokens.length, which cannot be negative, so the added guard has limited practical value.

Low

@chishui
chishui merged commit 5a91261 into opensearch-project:main Sep 11, 2026
96 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

skip-diff-analyzer Maintainer to skip code-diff-analyzer check, after reviewing issues in AI analysis.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants