[Sparse ANN] [Native] Wire the native sparse engine into the plugin, API layer - #1974
Open
chishui wants to merge 23 commits into
Open
[Sparse ANN] [Native] Wire the native sparse engine into the plugin, API layer#1974chishui wants to merge 23 commits into
chishui wants to merge 23 commits into
Conversation
PR Code Analyzer ❗AI-powered 'Code-Diff-Analyzer' found issues on commit 8714ce5. ⛔ Hard block: Issues at High severity or above will block this PR from merging. 'Diff too large, requires skip by maintainers after manual review' Pull Requests Author(s): Please update your Pull Request according to the report above. Repository Maintainer(s): You can Thanks. |
chishui
force-pushed
the
sparse-native-plugin
branch
from
August 31, 2026 07:09
3bfdb01 to
29512d9
Compare
chishui
marked this pull request as ready for review
September 1, 2026 02:36
chishui
requested review from
VijayanB,
bzhangam,
heemin32,
jmazanec15,
junqiu-lei,
martin-gaievski,
minalsha,
model-collapse,
naveentatikonda,
navneet1v,
owaiskazi19,
sean-zheng-amazon,
vamshin,
vibrantvarun,
yuye-aws,
zane-neo and
zhichao-aws
as code owners
September 1, 2026 02:36
chishui
changed the title
Second of two PRs for the native sparse ANN engine. PR opensearch-project#1972 added the JNI layer and the nsparse library binding; this connects it to the plugin so the engine is reachable from a real index. * codec - NativeDocValuesConsumer and DefaultNativeIndexWriter build a native index per segment and stream it out through IndexOutputWrapper; SparseCompoundFormat/SparseCompoundDirectory keep the engine files out of the Lucene compound file; SparseDocValuesProducer loads them back. * query - NativeIndexScorer and ResultsDocValueIterator run a query against a loaded native index and expose the hits as doc values. * mapper/settings - SparseEngine selects between the Lucene and native engines, behind SPARSE_NATIVE_ENGINE_FEATURE_ENABLED_SETTING. Two changes were needed on top of the original combined branch, both because the JNI layer moved under it: * NeuralSearch.getSettings() folds in SparseSettings.state().getSettings() rather than listing the sparse settings inline, so the set can depend on whether the native engine is available. Upstream's SEMANTIC_MODEL_SELECTION_MODEL_ID is unrelated and stays in the static list. NeuralSearchTests now asserts the sparse settings actually arrive instead of only counting them -- a bare count passes even if that call is dropped, as long as something else was added in the same change. * CodecUtils passes Locale.ROOT to String.format, which forbiddenApis requires and the rest of the plugin already does. DefaultNativeIndexWriter writes the native payload at file offset 0 with no codec header before it. That is load-bearing rather than incidental: nsparse computes its alignment padding from the writer's file offset and DiskSeismicIndex borrows those arrays from an mmap, so a header written before the payload would need the matching offset threaded through loadIndex as well. Verified: ./gradlew check -x integTest green, 2621 Java tests, and the JNI suite still 60/60 (no jni/ file is touched by this change). Signed-off-by: Liyun Xiu <xiliyun@amazon.com>
References the design issue; swap in the PR number once the PR is opened. Signed-off-by: Liyun Xiu <xiliyun@amazon.com>
…index Bumps jni/external/neural-sparse-cpp from 99bc33f to c0dfcfa to pick up disk_seismic_sq (upstream #37), a DiskSeismicIndex over scalar-quantized codes. The forward_index mapping parameter that follows selects it, and it does not exist at the old pin. The bump also brings in upstream #36, which put a format version in the index file header, and #38/#39 (benchmarks and kernel tests). nsparse/io/index_io.h is unchanged across the range, so the entry points the JNI uses -- read_index(char*, int), write_index, IndexIoFlag::kUseMmap -- are the same and nothing here had to move. buildSearchParameters() gains the DiskSeismicSQSearchParameters case. The subtype is still inferred from which keys the Java map carries, and only that subtype carries a query range into a disk_seismic_sq index: query_quantizer() and decode_scores() both dynamic_cast for it, so a range-less DiskSeismicSearchParameters would quantize the query at the index's build-time range and leave the decoded scores unscaled. SparseEngine.version stays at "102" despite the header change. The engine is unreleased, so no file written before it has to be readable, and a stale file is now rejected rather than misread: the new reader lands on the old dimension field where the version belongs and refuses anything outside 1..format_version(). Signed-off-by: Liyun Xiu <xiliyun@amazon.com>
Adds "forward_index": "shared" | "per_block" alongside "engine" in a sparse_vector
field's method block, selecting how the native engine lays out the per-document
vectors a seismic index scores candidates against:
* shared (default, the existing behaviour) - one forward index for the whole
field, nsparse seismic_sq.
* per_block - each block's vectors stored inline next to the block and mmap'd at
search time, so a query reads only the blocks it selects: nsparse
disk_seismic_sq.
Both quantize to 8-bit codes over the same range, so a given weight is clamped and
rounded identically either way, and identically to the JVM path. Only the index
type differs.
The value rides the same path as "engine": parsed and validated in
SparseMethodContext, stored as a Lucene field attribute by SparseVectorFieldMapper,
read back through SparseFieldUtils.getSparseForwardIndex(). per_block on any engine
but native is rejected at mapping time rather than silently ignored, since no other
engine builds a forward index it can lay out.
NativeIndexScorer sends k_prime for a per_block field, pinned to nsparse's own
default (50). Not a tuning knob yet -- there is no query parameter for it -- but it
is what makes the search-side quantization range land on
DiskSeismicSQSearchParameters, the only subtype disk_seismic_sq reads a range from.
SparseForwardIndexIT covers the per_block path end to end: build, force-merge, mmap
load, search. Verified it is the new index type under test by renaming the factory
string, which fails the test with "Unknown index type" rather than passing anyway.
Signed-off-by: Liyun Xiu <xiliyun@amazon.com>
…y boost Two defects, both only reachable through the native engine. explain returned a noMatch for any document the native engine scored. SparseQueryWeight built its reader from a SparseBinaryDocValuesPassThrough, which SparseDocValuesProducer only wraps for the Lucene engine, so the native path fell through to NOOP_READER and SparseExplanationBuilder reported "document not found or has no sparse vector". No JNI read-back is needed to fix it: a native field's raw vectors reach disk through the delegate consumer too (BaseSparseDocValuesConsumer#addBinaryField calls it unconditionally), so the document is recomputable from doc values alone. The reader is built directly over them and deliberately not routed through the forward index cache -- a native segment's index is an mmap'd file and nothing else populates that cache for it, so filling it here would charge the circuit breaker for memory no query benefits from. That also drops a getOrCreate() the old code ran for native segments regardless. The arithmetic already lines up, which is why the quantized breakdown is reused as-is: `raw * ceiling_ingest * ceiling_search / 255 / 255` is exactly nsparse's decode_dot_product at vmin=0, and ByteQuantizer.quantize and encode_8bit round the same way. A sub-threshold segment needed its own path. explain delegated to the fallback query there, but that query scores FeatureFields and a native field never writes any (SparseVectorFieldMapper#parseCreateField), so it explained a noMatch for a document that had scored. Those segments are scored by nsparse's InvertedIndex, which holds unquantized floats and computes an exact dot product, so quantizing would have explained a score nothing produced -- hence explainExactFloatScore(), which reports float contributions with no rescaling step. Second, the boost never reached a native score. NativeIndexScorer took no boost and returned nsparse's decoded score verbatim, so a boosted sparse_ann query ranked as if unboosted, while explain multiplied by it -- explaining a score the scorer never produced. It now takes the boost and applies it in score(); there is no rescaling to fold it into, unlike the Lucene path's SimScorer. Also makes the filter explanation engine-aware: with P > k the native engine restricts the ANN search to the filtered set, where Lucene post-filters. Testing: the three explain ITs are no longer gated to Lucene, so SEISMIC_EXPLAIN_IS_LUCENE_ONLY is gone, and testSearchWithExplain_ FallBackToRankFeaturesScoring gains a native branch -- its premise, that native falls back to rank_features, was the bug. The assertion that actually pins the arithmetic is new: assertExplanationScoreMatchesHit compares the explained value against the hit's _score, and it holds on both engines. testSearchWithExplain_BoostIsAppliedAndExplained pins the boost from both sides (scores scale, and explain agrees); reverting the one-line fix fails it with 0.251 against an expected 0.753. Known limit, documented on SparseExplanationBuilder rather than fixed: SparseVector folds token ids modulo 32768 and merges collisions by max weight, while the native engine indexes raw ids up to 65535, so the quantized path can diverge for a field using ids at or above 32768. The exact-float path is unaffected. Signed-off-by: Liyun Xiu <xiliyun@amazon.com>
Moves jni/external/neural-sparse-cpp from c0dfcfa to 4082c24 (upstream #40), which extends the distance-kernel equivalence tests to NEON and SVE. Tests and their CMake wiring only -- no library or header change, so nothing on the JNI side moves and SparseEngine.version stays put. Signed-off-by: Liyun Xiu <xiliyun@amazon.com>
Documents the nine classes the native engine added that had no class comment: DefaultNativeIndexWriter, NativeDocValuesConsumer, SparseVectorBinaryConsumer, SparseCompoundFormat, SparseCompoundDirectory, SparseEngine, CodecUtils, BinaryVectorUtils and NativeIndexScorer, plus method docs where the signature alone did not say enough. The notes worth having in the source rather than in a commit: the compound pair exists because nsparse loads an index by filesystem path, so an engine file folded into a .cfs as a slice cannot be mmapped; SparseEngine.version is part of the engine file name, so it has to be bumped whenever the payload layout changes; and NativeIndexScorer takes its whole top-k in the constructor, so k is fixed before iteration and a collector cannot terminate the query early. Comments only -- no behavior change. Signed-off-by: Liyun Xiu <xiliyun@amazon.com>
Both readToMap and readToList passed ArrayUtil.copyOfSubArray(bytes, offset, length), but that method takes (from, to). Any BytesRef whose offset was not 0 was therefore cut to length - offset bytes: trailing token/weight pairs were dropped silently, or the read threw EOFException part way through a pair. Every caller so far happens to hand over an offset of 0, which is why it has held. Windowed with ByteArrayInputStream(bytes, offset, length) instead of copying, which is both correct and one less array allocation per document. BinaryVectorUtilsTests covers this and the rest of the decode -- order and duplicate tokens preserved for readToList, last weight wins for readToMap, offset/length honoured, empty vector. Signed-off-by: Liyun Xiu <xiliyun@amazon.com>
…index lifetime These four classes had no unit coverage: every path through the rest of the native engine ends in a NativeLibrary call, but these do not, so they can be tested without the shared object. CodecUtilsTests pins the file name shape and the lookup rules, including the compound suffix -- the name is the only contract between the writer that creates the engine file and the reader that has to find it again. SparseCompoundFormatTests and SparseCompoundDirectoryTests cover the pair that keeps the engine file out of the .cfs. nsparse loads by filesystem path, so a file folded in as a slice cannot be mmapped, and a silent break here makes every native query fail to find its index. SegmentNativeIndexTests covers what can be reached without loading: both resolveIndexPath failure modes, and who owns the handle -- one index per (core, field) so a per-query load cannot happen, a private one when the reader exposes no core to free it. Coverage on the four goes from 0% to 96-100%, except SegmentNativeIndex at 80%: its remaining branches are behind address(), which loads the library. Signed-off-by: Liyun Xiu <xiliyun@amazon.com>
SparseMethodContext has been Writeable since 3.3 (opensearch-project#1514) and is in every release from 3.3.0 to 3.8.0, so the two fields the native engine adds cannot go on the stream unconditionally. A pre-3.9 peer writes the name followed directly by the component context; a reader that consumed two optional strings first would take the parameter map's bytes for them. MethodComponentContext then decides on available() > 0, so the loss is silent -- the seismic parameters come back null rather than failing. Guarded both directions on MINIMAL_SUPPORTED_VERSION_SPARSE_NATIVE_ENGINE (3.9), added to MinClusterVersionUtil beside the other feature versions. Reading from an older peer falls back to the engine and forward_index defaults, which is what such a node meant: it only ever ran the Lucene engine, and a null engine would reach the comparison in SparseAnnQueryBuilder. Three tests, each verified to fail without the guard. The one that matters is asymmetric -- it hand-writes the 3.8 layout rather than round-tripping, because a symmetric round trip passes either way and only a mixed cluster reads a stream it did not write. Note this is latent rather than live: nothing serializes SparseMethodContext over the transport today (MappedFieldType is not Writeable and mappings travel as XContent), which is also why no qa/ IT can reach it. Signed-off-by: Liyun Xiu <xiliyun@amazon.com>
NeuralSearch.java: the import block is back to upstream's, with only the delta this feature needs (+ArrayList, +SEMANTIC_MODEL_SELECTION_MODEL_ID, -DEFAULT_INDEX_THREAD_QTY). The reshuffle was most of the file's diff and made the real change hard to find. build.gradle: dropped -Pexcluded.cluster.plugins. It works around a jackson-core jar hell in the 3.9.0-SNAPSHOT distro that stops opensearch-ml from starting, which has nothing to do with this feature; it belongs in its own change. SparseSettings.getSettingValue now falls back to the setting's default when clusterService is null instead of throwing. Only isNativeEngineEnabled guarded for that, yet DefaultNativeIndexWriter reads the thread count and the streaming memory limit through it during a flush, which must not fail on a node that never called initialize(). The guard inside isNativeEngineEnabled is redundant now: the dynamic gate's default is off, so the answer is unchanged. SparseQueryWeight.selectScorer: filter != null was implied by filterBitIterator != null, and cardinality() -- a scan of the bitset's words -- ran twice. Computed once and reused for both the iterator's cost and the exact match threshold, with filter scoped to the block that builds the iterator. NativeIndexScorer: java.util.* imported rather than inlined, filterIdsType's bare 0 named, and constructFilterList grows a long[] sized from the iterator's cost instead of boxing every doc id into a List<Long> to unbox it again. WriteIndex renamed to writeIndex, and the C-style int tokens[] / long dataAddresses[] declarations moved to int[] tokens. No behavior change beyond the getSettingValue fallback. Signed-off-by: Liyun Xiu <xiliyun@amazon.com>
…e JNI library Codecov put patch coverage at 51.96% with 343 lines missing. Most of the flagged files end every path in a NativeLibrary call, but these do not, so they are coverable as they stand. SparseExplanationBuilder was the largest addressable gap at 52 lines: the exact float path nsparse's inverted index takes below the approximate threshold, and the filter wording the native engine needs because it hands the filter to nsparse as a candidate set rather than post-filtering. Covered including the contribution ordering, the tokens absent from the document, all three read failures, and both engines' filter descriptions. Also covered: Seismic's quantization ceiling validation, both branches of SparseQueryWeight#explain that pick the reader for a native segment plus its missing-doc-values exit, the SparseFieldUtils attribute getters and the null FieldInfo default, and SparseSettings' pre-initialize fallback. SparseDocValuesProducer's private NativeBinaryDocValues is deleted rather than tested: nothing has ever instantiated it, so its eight lines were dead. SparseSettingsTests registered three of the sparse settings where a real node registers five, so the streaming memory limit could not be read through the cluster settings at all. Registers every node-scoped one now. Uncovered added lines: 311 -> 225. The remaining 199 are NativeIndexScorer, DefaultNativeIndexWriter, OffHeapSparseVectorsBuffer and the rest of SegmentNativeIndex, which cannot be unit tested without a seam in front of the native calls -- Mockito cannot stub a native method. Signed-off-by: Liyun Xiu <xiliyun@amazon.com>
…rary I was wrong that these classes needed a seam in front of NativeLibrary. The test task already depends on buildJniLib and sets java.library.path, so a unit test can drive the real shared object -- the same thing NativeLibraryContractTests relies on. Nothing in production had to change. NativeIndexRoundTripTests drives the write half end to end: doc values through OffHeapSparseVectorsBuffer and DefaultNativeIndexWriter into an engine file, then loads it back and queries it. Covers both forward index layouts, the inverted index below the approximate threshold, the footer-only file for a segment with no document in the field, and the handle sharing in SegmentNativeIndex. Two of them pin bugs the code comments call out: tokens are asserted out of order, since a writer taking the last element rather than the max would undersize the dimension, and vectors are pushed across several buffer flushes, since a flush that did not rebase its CSR offsets would score later documents against the wrong vectors. NativeIndexScorerTests scores a real index. The index is kept unquantized so the scores are exact dot products and the assertions can pin the arithmetic rather than a ranking: the boost multiply, the doc-order sort that stops a conjunction tripping a Lucene assertion, filters as a candidate set, and the deleted-hit inflation of the fetch size. NativeDocValuesConsumerTests now covers the write and merge bodies with both gates open, not just the skips. Assertions are on what comes back out of the index because the CSR buffers and the built index are opaque native memory. Uncovered added lines: 225 -> 31, patch coverage 52% -> 96%. Signed-off-by: Liyun Xiu <xiliyun@amazon.com>
…ection Two CI failures, one of them mine. Windows, mine: NativeIndexRoundTripTests left engine files mmapped, and Windows will not delete a mapped file, so the test framework's temp-dir cleanup threw "The process cannot access the file because it is being used by another process". Every handle the test loads is now recorded and freed in tearDown rather than at the end of each test, so an assertion that throws early cannot leak one. The core-scoped case is freed by capturing the closed listener and firing it, since nothing else closes a core here -- which also exercises the listener path. BWC: SparseTestCommon#addSeismicField wrote engine and forward_index into every sparse_vector mapping it built, including when both were their defaults. The restart-upgrade tests create their index on the old cluster, and a pre-3.9 node rejects an unknown method key outright, so SparseAnnNestedIT failed with "Invalid parameter: engine" for every version pair. Emitted only when non-default now, so a mapping that does not need them is byte-identical to what those nodes already accept. Signed-off-by: Liyun Xiu <xiliyun@amazon.com>
Integ tests: every sparse test failed with "Connection refused" because the node had already exited. ml-commons brings DJL's precxx11 PyTorch flavour, which loads its own bundled libstdc++.so.6 into the process; the loader then satisfies our SONAME reference from that copy, which is older than the toolchain that built us, so dlopen failed with "version `GLIBCXX_3.4.32' not found". The resulting UnsatisfiedLinkError is an Error, and OpenSearch halts the node on one. Which plugin loads libstdc++ first is not something we control, so the C++ runtime is linked in statically on Linux and the dependency is gone. Verified inside the almalinux8 CI image (gcc 13.3, the toolchain that produced the 3.4.32 requirement): the .so no longer names libstdc++.so.6, carries no GLIBCXX version references, and still dlopens with its JNI entry points intact. Restart-upgrade BWC: MapperService#assertMappingVersion killed every upgraded node holding a sparse index created on the old cluster. That index's mapping source has neither engine nor forward_index, but toXContent emitted both unconditionally, so re-serializing it did not match the stored source byte for byte and the assertion fired. Each field is now left out when it resolves to its default. The cost is that an explicit "engine": "lucene" is no longer echoed back, the same way the rest of the mapping drops values it resolved to a default; SparseIndexingIT asserted the old behaviour and now asserts the new one. Signed-off-by: Liyun Xiu <xiliyun@amazon.com>
The field held only the static opensearch.yml flag, but it was named nativeEngineFeatureEnabled and read inside isNativeEngineEnabled(), where it sits next to the dynamic gate. That reads as though it were already the combination of the two. Renamed to staticNativeEngineEnabled, and the comment now says which gate it is and where the other one is read. No behaviour change: isNativeEngineEnabled() still requires both gates, and the setting keys are untouched. Signed-off-by: Liyun Xiu <xiliyun@amazon.com>
…er's outcome SparseSettings is a process-wide singleton, and several test classes initialize it with a ClusterSettings that registers only the settings they care about. Every later class in the same JVM then reads through that instance, and any setting it did not register throws SettingsException instead of falling back to its default. NativeIndexScorerTests touches SparseSettings not at all -- it needs the singleton uninitialized -- so it failed with "vector_streaming_memory.limit has not been registered" whenever it happened to land after one of those classes. The hazard is latent rather than new: which classes share a JVM depends on worker timing, not on tests.seed, so it reproduces only intermittently and passes under -Dtests.jvms=1. Resetting the singleton in the base setUp means no class inherits another's initialization; the classes that initialize it deliberately do so after super.setUp() and are unaffected. Signed-off-by: Liyun Xiu <xiliyun@amazon.com>
…iteIndex insertToIndex was the only routine that freed the CSR vectors transferVectors allocates -- it adopts all three into unique_ptrs -- so any path that transferred without reaching it dropped the last handle on memory nothing would ever release. The reachable one: writeIndex streams doc values into a buffer that transfers off-heap each time the streaming limit is hit, and the buffer was a local inside writeToBuffer. A throw partway (a corrupt value, an I/O error mid-merge) left the addresses to die with the frame. Measured at 182 MiB abandoned over six failed writes of a 20k-document segment. OffHeapSparseVectorsBuffer#close made it worse: it called flush(), which allocates off-heap and then abandons what it allocated, so closing the buffer grew the footprint instead of releasing it. close() now frees through a new NativeLibrary#freeVectors, which zeroes each address as it frees it -- that is both how Java learns the buffer owns nothing and what makes a second call a no-op. Handing the vectors over is now insertInto(), which drops the Java-side addresses before calling insertToIndex: the native side adopts all three before anything that can throw and frees them on every path out, so after entry the buffer must never free them again. writeIndex creates the buffer in its try-with-resources next to the IndexOutput, which is what actually closes the leak, and writeToBuffer takes it as a parameter. The index handle's own ownership dance is unchanged. Its inner try stays where it was: indexAddress does not exist until after the empty-segment early return, so hoisting it would need a mutable sentinel and would move freeIndex to after the IndexOutput closes. OffHeapVectorOwnershipTests covers the three invariants, and the leak test is measured against resident size rather than a handle because after the throw there is no handle: it runs two identical batches and asserts on the second one's growth, so an allocator that keeps freed arenas settles during the first batch and only real per-attempt leakage registers. Verified to fail without the fix. FreeVectorsTest covers the free routine itself, including a double free and all-zero addresses; clean under ASan/LSan. Two round-trip tests read the addresses inside a try-with-resources and inserted after the block, relying on close() to flush. That is a use-after-free now, so both flush and insert inside the block. Signed-off-by: Liyun Xiu <xiliyun@amazon.com>
Both merge paths caught Exception and only logged it, so a failure while writing the sparse side files left the merge reporting success. Lucene then took the merged segment's file list from the tracking directory and committed it, and the engine file in it was truncated: writeIndex writes its footer as the last statement of the success path, and a file is only registered as a segment file after the write completes (IndexWriter#5402 for a merge, DocumentsWriterPerThread#504 for a flush). Nothing downstream catches that -- the engine file is read by path through nsparse::read_index, never through a Lucene IndexInput, so no footer or checksum is ever verified, and SparseDocValuesProducer#checkIntegrity only delegates. The failure surfaced as nsparse misreading a mapped file at query time. Rethrowing is what makes the file transient: the exception aborts the merge, the setFiles call is never reached, and deleteNewFiles removes what was written. The flush path already propagated and was never affected. The swallow predates the split of SparseDocValuesConsumer into BaseSparseDocValuesConsumer plus the two SparseVectorBinaryConsumers; that refactor duplicated it and also moved delegate.merge inside the try, which swallowed core Lucene doc-values merge failures too. Both are gone. testMerge_WithSparseField_noCachedVector was passing for the wrong reason: it never stubbed binaryValue(), so the decode NPE'd and the swallow hid it while the verify still passed. It now hands back a real encoded vector. Merges that fail will now be visible and retried instead of silently producing an unreadable segment, so errors currently hidden in the log will start surfacing. Signed-off-by: Liyun Xiu <xiliyun@amazon.com>
Both APIs walk every sparse field in the shard and drive the JVM caches -- ForwardIndexCache and ClusteredPostingCache -- for each one. A native-engine field is searched from its own mmap'd index file through NativeIndexScorer and never reads either cache, so warming them spends heap on data no query will look at. The waste was real rather than theoretical because SparsePostingsConsumer is not engine-aware: it writes the Lucene terms and clustered-posting files for a sparse field whatever engine the field declares, so the files a native segment does not need are there, and warm up loaded them successfully. Measured on a single-shard native index, the sparse memory stat went from 0.13 to 0.21 across a warm up that should have been a no-op. Clear cache had the mirror-image problem: it reported success having evicted entries that only warm up had created. collectCacheOperationContexts now drops a field whose engine attribute is native, before it builds any reader for it. Both APIs stay successful on a native-only index -- a no-op is the honest answer, not a shard failure -- and on a mixed-engine index they still cover the Lucene fields. NeuralSparseNativeCacheOperationIT covers the API contract on a native index: 200, zero failed shards, and a sparse memory stat that does not move. It fails without the fix. The unit tests assert the cache keys directly, with the same reader on the Lucene engine as the positive control so the assertion cannot pass vacuously. prepareIndexReaderWithSparseField takes an engine for that. The Lucene-engine suite keeps its own memory-delta assertions and stays Lucene-only; its class comment now points at the native one. Signed-off-by: Liyun Xiu <xiliyun@amazon.com>
chishui
force-pushed
the
sparse-native-plugin
branch
from
September 4, 2026 03:32
68cce68 to
287507e
Compare
… the build in windows The native engine transferred a segment's whole CSR to off-heap memory to hand it to nsparse, so at build time the vectors were resident anonymous memory that the kernel cannot reclaim. CsrFileNativeIndexWriter stages them as the two files IDMapIndex::read_csr_and_ids reads instead -- a native-layout CSR and a row-aligned doc-id file -- and nsparse borrows them from the mapping, so they land in page cache. Values are staged at the width the target index borrows at, its code_element_size(): 8-bit codes for the quantized seismic layouts, and float32 for the unquantized inverted index a sub-threshold segment gets. Both writers quantize with the same ByteQuantizer over the same range, so either path builds the same index -- pinned by tests asserting identical doc ids and scores rather than approximate agreement. DefaultNativeIndexWriter stays as the fallback for a directory nsparse cannot map, which in practice means an in-memory one; supports() is the check. Measured on base_full (8.8M docs, A-B-A): peak RssAnon during forcemerge 31.1 -> 18.1 GiB (-42%), peak VmRSS 39.3 -> 30.7 GiB, with 2.7 GiB reappearing as reclaimable RssFile. Wall time unchanged, recall and latency unchanged. Also adds the clustering_batch_size mapping parameter, which bounds the build's memory further by clustering the term space in windows and spilling each window. It reaches both writers through the shared NativeIndexParameters, and moves forward_index into method.parameters so the two are validated together. Both are gated on the index created version: mapping parsing runs inside a cluster state applier, where reading the applied state is illegal, so index.version.created is what records whether every node can read them. Removes plugins.neural_search.sparse.vector_streaming_memory.limit. Its only consumer was DefaultNativeIndexWriter's transfer batch size, which now derives the same 1% of heap the setting defaulted to; the knob only set how often the transfer happened, not how much memory the segment held. nsparse: bumped to d9d0df15, which carries read_csr_and_ids, the mmapped codes CSR for quantized indexes, batched term-space building, and the InvertedIndex vector-count fix (opensearch-project/neural-sparse-cpp#48). Signed-off-by: Liyun Xiu <xiliyun@amazon.com>
yuye-aws
reviewed
Sep 4, 2026
| final boolean maskDeletedDocs = docsIds == null && acceptedDocs != null; | ||
| // Deleted hits are dropped after the fact, so ask for enough of them that they cannot eat | ||
| // into the k results the caller wanted. Capped at maxDoc, which already covers every doc. | ||
| final int fetchSize = maskDeletedDocs ? Math.min(segmentInfo.maxDoc(), resultSize + leafReader.numDeletedDocs()) : resultSize; |
Member
There was a problem hiding this comment.
Can you check whether numDeletedDocs() is a segment-wide count of deletions? If the segment has 10K deleted documents but only top-10 is wanted, this could be over fetching a lot.
Collaborator
Author
There was a problem hiding this comment.
Yes, segment-wide, so k=10 with 10K deletions asked for a top-10010, and a larger k also weakens seismic's pruning. Now it fetches k and re-queries larger only while survivors fall short, capped at maxDoc. Tests: testFetchGrowsWhenTheWholeTopKIsDeleted, testFetchStopsGrowingWhenTooFewDocsAreLive.
Comment on lines
+48
to
+51
| for (String engineFile : engineFiles) { | ||
| String compoundFile = engineFile + SparseConstants.COMPOUND_EXTENSION; | ||
| dir.copyFrom(dir, engineFile, compoundFile, context); | ||
| } |
Member
There was a problem hiding this comment.
Is there any where to delete the copy source file?
Collaborator
Author
There was a problem hiding this comment.
Lucene deletes it: both callers snapshot the file set before write() and delete it after (DocumentsWriterPerThread.sealFlushedSegment on flush, IndexWriter.mergeMiddle on merge). setFiles() rebinds to a fresh HashSet, so those snapshots keep the .nsparse name. Added a javadoc note.
numDeletedDocs() is segment-wide, so asking nsparse for k + numDeletedDocs() turned a k=10 query on a segment with 10K deletions into a top-10010. That is not just a larger heap: a bigger k raises the admission threshold more slowly, so seismic visits far more blocks -- a cost paid on every query against any segment carrying deletions, whether or not a deleted doc is anywhere near the top k. Start at k instead and re-query only when deleted hits actually ate into the results, growing by max(2x, k / surviving fraction) up to maxDoc. Deletions rarely reach the top k, so the common case is one call at exactly k, and the pathological case converges in a couple. Signed-off-by: Liyun Xiu <xiliyun@amazon.com>
The mapped-CSR build and clustering_batch_size were listed as enhancements, but the engine they enhance has not shipped yet -- to a reader of the release notes they are part of the one feature, not changes to it. Signed-off-by: Liyun Xiu <xiliyun@amazon.com>
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
2 participants
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.
Description
Second of two PRs for the native sparse ANN engine. #1972 adds the JNI layer; this wires it into the plugin (codec, query, mapper, behind a feature flag) and adds a
forward_indexmapping parameter:shared(default, nsparseseismic_sq) orper_block(nsparsedisk_seismic_sq, per-block vectors read via mmap). Also fixes two native-only defects:explainreturned noMatch for any natively scored document, and the queryboostnever reached a native score.Draft: stacked on #1972, so its commits appear here too. I'll rebase and mark ready once it merges. The last 4 commits are new here; their messages carry the detail.
Related Issues
Relates to #1802
Depends on #1972
Check List
--signoff.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.