Eliminate FFM round-trips via nextDoc piggyback on collectDocs

[aasom143]

collectDocs now returns packed i64: upper 32 bits = next matching docId beyond maxDoc, lower 32 bits = wordsWritten. No new FFM calls needed.

Rust consumers use nextDoc for three levels of optimization:

1. Full RG skip: nextDoc >= rgMax → no collectDocs call
2. Tightened range: nextDoc > rgMin → start from nextDoc, smaller bitset
3. No benefit: nextDoc <= rgMin → call as before

Implemented in both SingleCollectorEvaluator (AtomicI32 across RGs) and CollectorLeafBitmaps/BitmapTree (per-leaf HashMap keyed by Arc identity).

Description

[Describe what this change achieves]

Related Issues

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

Check List

• Functionality includes testing.
• API changes companion pull request created, if applicable.
• Public documentation issue/PR created, if applicable.

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]

PR Reviewer Guide 🔍

(Review updated until commit 714ea99)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review Inconsistent skip/tighten condition The RG-level skip uses last_next > max_doc (line 295), but the per sub-range check uses last_next > *r_max (line 382), while the PR description states the skip should apply when nextDoc >= rgMax. Since max_doc is exclusive, if last_next == max_doc the RG contains no matching doc within [min_doc, max_doc), yet the code will still proceed to call the collector with a possibly-empty tightened range. This is a correctness/efficiency mismatch with the documented contract; consider >= to match the Javadoc ("nextDoc >= rgMax → no collectDocs call"). // Skip RG if the previous collectDocs told us the next match is beyond this RG. let last_next = self .last_next_doc .load(std::sync::atomic::Ordering::Acquire); if last_next > max_doc { native_bridge_common::log_debug!( "SingleCollector: skipping RG {} — nextDoc={} > maxDoc={}", rg.index, last_next, max_doc ); return Ok(None); } Stale nextDoc when scorer skipped In the else branch (line 251-252, when scanTo <= scanFrom so the scorer is not advanced this call), the returned nextDoc is computed from a possibly-stale handle.currentDoc that predates maxDoc. If handle.currentDoc < maxDoc, Math.max(handle.currentDoc, maxDoc) == maxDoc, which is then returned as nextDoc even though there is no proof of a matching doc at or beyond maxDoc. This can cause the Rust side to under-skip (harmless) or, if callers ever apply >= skipping, over-skip. Consider returning Integer.MAX_VALUE or maxDoc only when the scorer has actually been advanced past maxDoc. nextDoc = (handle.currentDoc == DocIdSetIterator.NO_MORE_DOCS) ? Integer.MAX_VALUE : Math.max(handle.currentDoc, maxDoc); } catch (IOException exception) { LOGGER.warn("IOException during collectDocs, returning partial bitset", exception); } } else { nextDoc = (handle.currentDoc == DocIdSetIterator.NO_MORE_DOCS) ? Integer.MAX_VALUE : Math.max(handle.currentDoc, maxDoc); } AtomicI32 assumes ordered RG evaluation last_next_doc is a single AtomicI32 shared across all RGs in the evaluator. The optimization is only correct if RGs are prefetched strictly in increasing docId order. If prefetch_rg is invoked concurrently or out-of-order (e.g., parallel RG evaluation, or later reprocessing), a small next_doc from a later RG could be stored and cause earlier or unrelated RGs to be incorrectly skipped/tightened. Please confirm the caller guarantees monotonic RG order, or gate updates by fetch_max-style logic. /// Reverse map: absolute RG index → position in `rg_can_match` vectors. rg_index_to_pos: HashMap<usize, usize>, /// Next matching docId from the last collectDocs call. When next_doc >= rg.max_doc, /// the RG can be skipped without an FFM call. Initialized to i32::MIN (no skip info). last_next_doc: std::sync::atomic::AtomicI32, Arc pointer identity as HashMap key leaf_key = Arc::as_ptr(collector) as usize is used as the HashMap key. If a collector Arc is ever dropped and a new Arc is allocated at the same address later in the same query, the stale next_doc entry could be applied to a different logical leaf and cause incorrect skipping. Since CollectorLeafBitmaps outlives individual RG evaluations but is per-segment, this is usually safe, but the invariant should be documented, and consider keying by a stable per-leaf id (annotation_id / provider_key) instead. /// Per-leaf next_doc tracking. Key = Arc pointer identity of the collector. /// Value = next matching docId from the last collectDocs call for that leaf. leaf_next_doc: std::sync::Mutex<HashMap<usize, i32>>, } impl CollectorLeafBitmaps { pub fn new(ffm_collector_calls: Option<datafusion::physical_plan::metrics::Count>) -> Self { Self { ffm_collector_calls, leaf_next_doc: std::sync::Mutex::new(HashMap::new()), } } /// Construct a `CollectorLeafBitmaps` with no metrics. pub fn without_metrics() -> Self { Self::new(None) } } impl LeafBitmapSource for CollectorLeafBitmaps { fn leaf_bitmap( &self, collector_node: &ResolvedNode, _leaf_dfs_index: usize, // This is not used in this implementation ctx: &RgEvalContext, ) -> Result<RoaringBitmap, String> { let collector = match collector_node { ResolvedNode::Collector { collector, .. } => collector, _ => { return Err("CollectorLeafBitmaps: non-Collector node passed to leaf_bitmap".into()) } }; let leaf_key = Arc::as_ptr(collector) as *const () as usize; let last_next_doc = { let map = self.leaf_next_doc.lock().unwrap(); map.get(&leaf_key).copied().unwrap_or(i32::MIN) };

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 714ea99

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Use stable leaf identifier as key Using Arc::as_ptr address as a HashMap key is unsafe: Arcs can be dropped and their memory reused for a different collector, causing stale next_doc entries to incorrectly skip or tighten a new leaf's calls. Use a stable identifier such as the resolved node's provider_key or a per-leaf index (e.g., _leaf_dfs_index) that uniquely identifies the leaf within this evaluation. sandbox/plugins/analytics-backend-datafusion/rust/src/indexed_table/eval/bitmap_tree.rs [1020] -let leaf_key = Arc::as_ptr(collector) as *const () as usize; +let leaf_key = _leaf_dfs_index; Suggestion importance[1-10]: 8 __ Why: Using raw Arc::as_ptr addresses as HashMap keys is a real correctness/soundness concern. If an Arc is dropped and memory reused, stale entries could cause incorrect skip/tighten decisions for a new collector. Using a stable identifier is safer.	Medium
	Avoid inflating nextDoc past true position Using Math.max(handle.currentDoc, maxDoc) can incorrectly report a nextDoc greater than the true next matching doc when currentDoc < maxDoc (e.g., when the iterator stopped inside the range). The contract states nextDoc should be the next matching docId beyond maxDoc; if currentDoc < maxDoc, the correct value is currentDoc itself (which is >= maxDoc iff scanTo was reached), otherwise report currentDoc. Return handle.currentDoc directly (it's already >= scanTo) to avoid over-skipping subsequent RGs. sandbox/plugins/analytics-backend-lucene/src/main/java/org/opensearch/be/lucene/LuceneFilterDelegationHandle.java [245-247] nextDoc = (handle.currentDoc == DocIdSetIterator.NO_MORE_DOCS) ? Integer.MAX_VALUE - : Math.max(handle.currentDoc, maxDoc); + : handle.currentDoc; Suggestion importance[1-10]: 7 __ Why: Valid concern about correctness of the nextDoc value. Using Math.max(handle.currentDoc, maxDoc) may over-report nextDoc when the iterator's current position is inside the range, potentially causing legitimate subsequent RGs to be incorrectly skipped. However, handle.currentDoc after the while loop should already be >= scanTo in typical cases.	Medium
	Disambiguate error sentinel from packed value The error sentinel -1 (all bits set) will not be caught by packed < 0 in all cases because a legitimate packed value with nextDoc = -1 in the upper 32 bits (e.g., cast from Integer.MAX_VALUE math) could also produce a negative long. More importantly, (packed_u >> 32) as i32 will produce a negative next_doc for valid Java Integer.MAX_VALUE values only when the top bit is set — but Java only returns Integer.MAX_VALUE or valid docIds, so a nextDoc with the sign bit set is spurious. Check the sentinel explicitly as packed == -1 to disambiguate. sandbox/plugins/analytics-backend-datafusion/rust/src/indexed_table/ffm_callbacks.rs [237-245] -if packed < 0 { +if packed == -1 { return Err(format!( "collectDocs(context_id={}, key={}) failed: {}", self.context_id, self.key, packed )); } let packed_u = packed as u64; let words_written = (packed_u & 0xFFFF_FFFF) as usize; let next_doc = (packed_u >> 32) as i32; Suggestion importance[1-10]: 3 __ Why: The concern is partially valid but the reasoning is weak. packed < 0 catches -1 correctly, but could also mistakenly trigger when the upper 32 bits represent a nextDoc with sign bit set. Since Java only returns valid docIds or Integer.MAX_VALUE, this is unlikely in practice, but the explicit check packed == -1 is cleaner.	Low
General	Propagate nextDoc across sub-range calls When call_ranges contains multiple sub-ranges (PageRangeSplit strategy), using last_next (the stored value from the previous RG) for every sub-range is stale after the first call — subsequent sub-ranges should use the freshly returned next_doc_out to further tighten/skip. Update the compared value to next_doc_out inside the loop to correctly propagate progress across sub-ranges. sandbox/plugins/analytics-backend-datafusion/rust/src/indexed_table/eval/single_collector.rs [379-385] let mut bm = RoaringBitmap::new(); let mut next_doc_out = last_next; for (r_min, r_max) in &call_ranges { - if last_next > *r_max { + if next_doc_out > *r_max { continue; } - let effective_min = last_next.max(*r_min); + let effective_min = next_doc_out.max(*r_min); Suggestion importance[1-10]: 8 __ Why: Valid correctness issue for PageRangeSplit strategy. Using stale last_next across sub-ranges misses the opportunity to further tighten/skip based on freshly returned next_doc_out, potentially causing unnecessary work or incorrect behavior when sub-ranges overlap the newly advanced iterator position.	Medium

---

Previous suggestions

Suggestions up to commit 98014ef

Category	Suggestion	Impact
Possible issue	Report true iterator position as nextDoc Using Math.max(handle.currentDoc, maxDoc) clobbers the true next-doc when currentDoc < maxDoc (e.g. when the scan clamped at scanTo = partitionMaxDoc < maxDoc), producing an incorrect nextDoc that lies about how far the iterator has advanced. Return handle.currentDoc directly so downstream skip/tighten decisions use the actual iterator position. sandbox/plugins/analytics-backend-lucene/src/main/java/org/opensearch/be/lucene/LuceneFilterDelegationHandle.java [245-247] nextDoc = (handle.currentDoc == DocIdSetIterator.NO_MORE_DOCS) ? Integer.MAX_VALUE - : Math.max(handle.currentDoc, maxDoc); + : handle.currentDoc; Suggestion importance[1-10]: 7 __ Why: Valid concern: Math.max(handle.currentDoc, maxDoc) inflates nextDoc when the iterator was clamped by partitionMaxDoc < maxDoc, potentially causing incorrect downstream skip decisions. However, the impact depends on whether callers treat nextDoc as a lower bound or exact position.	Medium
	Fix off-by-one in skip comparison The skip condition should be last_next >= max_doc since max_doc is exclusive: if next_doc == max_doc, the next match is at or beyond the RG's exclusive upper bound and this RG has no matches. The current > also disagrees with the doc comment which says "skip subsequent RGs where nextDoc >= rgMax". sandbox/plugins/analytics-backend-datafusion/rust/src/indexed_table/eval/single_collector.rs [292-295] let last_next = self .last_next_doc .load(std::sync::atomic::Ordering::Acquire); -if last_next > max_doc { +if last_next >= max_doc { Suggestion importance[1-10]: 7 __ Why: Legitimate off-by-one concern: since max_doc is exclusive, last_next == max_doc means no matches in the RG, so >= matches the documented contract. This is a minor correctness improvement.	Medium
General	Tighten error sentinel to avoid packing collisions The success/error check if packed < 0 relies on signed interpretation, but a legitimate packed value with next_doc = Integer.MAX_VALUE and any wordsWritten produces packed_u >> 32 == 0x7FFF_FFFF, which as i64 is positive — good. However, if the Java side ever encodes nextDoc such that the high bit is set (e.g. accidentally negative int cast), packed as i64 becomes negative and is misinterpreted as error. Consider defining a distinct sentinel (e.g. only -1L exactly) rather than any negative value, or documenting that nextDoc must be non-negative and asserting it. sandbox/plugins/analytics-backend-datafusion/rust/src/indexed_table/ffm_callbacks.rs [237-245] +if packed == -1 { + return Err(format!( + "collectDocs(context_id={}, key={}) failed", + self.context_id, self.key + )); +} let packed_u = packed as u64; let words_written = (packed_u & 0xFFFF_FFFF) as usize; let next_doc = (packed_u >> 32) as i32; Suggestion importance[1-10]: 3 __ Why: The concern is theoretical — nextDoc values from Java are documented as non-negative doc IDs or Integer.MAX_VALUE, so the high bit of the packed long shouldn't be set in success cases. Minor defensive suggestion.	Low

Suggestions up to commit 12632af

Category	Suggestion	Impact
Possible issue	Avoid pointer-identity keys and fix skip bound Using the Arc pointer as a usize key is unsafe: if a collector Arc is dropped and a new one is allocated at the same address, stale next_doc state from a previous leaf will be applied to a different collector, causing incorrect skips. Additionally, the skip check should use >= since max_doc is exclusive. Consider keying on a stable identifier (e.g., the provider_key from ResolvedNode::Collector, or a leaf_dfs_index) instead of pointer identity. sandbox/plugins/analytics-backend-datafusion/rust/src/indexed_table/eval/bitmap_tree.rs [1020-1029] let last_next_doc = { let map = self.leaf_next_doc.lock().unwrap(); map.get(&leaf_key).copied().unwrap_or(i32::MIN) }; - if last_next_doc > ctx.max_doc { + if last_next_doc >= ctx.max_doc { return Ok(RoaringBitmap::new()); } Suggestion importance[1-10]: 7 __ Why: Using Arc::as_ptr as a usize key is a real correctness risk: if an Arc is dropped and a new one reused at the same address, stale state could apply. The _leaf_dfs_index parameter is available and would be a more stable key. This is a valid concern worth addressing.	Medium
	Fix nextDoc computation when currentDoc < maxDoc When handle.currentDoc is within [scanFrom, scanTo) (e.g. iterator not yet advanced or exactly at scanFrom), Math.max(currentDoc, maxDoc) returns maxDoc, which incorrectly tells callers the next match is at maxDoc even though the actual next match may lie inside the current range. The nextDoc should reflect the next matching doc >= maxDoc; if currentDoc < maxDoc, this range wasn't fully scanned so nextDoc should be maxDoc only after confirming iteration reached that point. Consider only setting nextDoc from currentDoc when currentDoc >= maxDoc, otherwise fall back to maxDoc. sandbox/plugins/analytics-backend-lucene/src/main/java/org/opensearch/be/lucene/LuceneFilterDelegationHandle.java [245-252] nextDoc = (handle.currentDoc == DocIdSetIterator.NO_MORE_DOCS) ? Integer.MAX_VALUE - : Math.max(handle.currentDoc, maxDoc); + : (handle.currentDoc >= maxDoc ? handle.currentDoc : maxDoc); } catch (IOException exception) { LOGGER.warn("IOException during collectDocs, returning partial bitset", exception); } } else { - nextDoc = (handle.currentDoc == DocIdSetIterator.NO_MORE_DOCS) ? Integer.MAX_VALUE : Math.max(handle.currentDoc, maxDoc); + nextDoc = (handle.currentDoc == DocIdSetIterator.NO_MORE_DOCS) + ? Integer.MAX_VALUE + : (handle.currentDoc >= maxDoc ? handle.currentDoc : maxDoc); } Suggestion importance[1-10]: 5 __ Why: The suggestion raises a plausible concern about nextDoc semantics when currentDoc < maxDoc, but in the code path currentDoc is set after the iterator advances past scanTo (which is <= maxDoc), so Math.max(currentDoc, maxDoc) effectively yields maxDoc in the boundary case. The change is essentially equivalent in the main path but adds clarity; moderate impact.	Low
General	Use inclusive comparison for RG skip check The skip condition uses > but the documented contract states "skip subsequent RGs where next_doc >= rg_max". Since max_doc is exclusive, when last_next == max_doc no matching doc lies in [min_doc, max_doc) and the RG can be safely skipped. Using >= matches the contract and skips one more RG per query in the boundary case. sandbox/plugins/analytics-backend-datafusion/rust/src/indexed_table/eval/single_collector.rs [292-295] let last_next = self .last_next_doc .load(std::sync::atomic::Ordering::Acquire); - if last_next > max_doc { + if last_next >= max_doc { Suggestion importance[1-10]: 6 __ Why: Since max_doc is exclusive and the PR's own doc comment says "skip where next_doc >= rg_max", using >= matches the documented contract and can prune one additional RG in boundary cases. Minor correctness/consistency improvement.	Low
	Preserve monotonic next_doc across sub-ranges When call_ranges contains multiple sub-ranges (e.g. from PageRangeSplit), each collector call returns a next_doc and the loop overwrites next_doc_out on each iteration, keeping only the last value. If a later range's next_doc is smaller than an earlier range's, we lose the tighter skip hint; conversely, if the collector's iterator state advances, using only the last value is fine — but ranges are processed in order so the last is authoritative. Ensure call_ranges is sorted ascending, or take the max, to avoid regressing last_next_doc and incorrectly skipping RGs later. sandbox/plugins/analytics-backend-datafusion/rust/src/indexed_table/eval/single_collector.rs [379-383] // Call collector for each range, merge into one RG-relative bitmap. let mut bm = RoaringBitmap::new(); let mut next_doc_out = last_next; for (r_min, r_max) in &call_ranges { let result = collector .collect_packed_u64_bitset(*r_min, *r_max) + // ... (after obtaining result) + // next_doc_out = max(next_doc_out, result.next_doc) to preserve monotonicity Suggestion importance[1-10]: 4 __ Why: The concern is legitimate for PageRangeSplit where multiple sub-ranges are processed; taking the last value could be incorrect if the collector's iterator returns non-monotonic hints. However, in practice the collector's currentDoc advances monotonically, so the last value is typically authoritative. The suggestion adds defensive robustness.	Low

Suggestions up to commit 9984bf2

Category	Suggestion	Impact
Possible issue	Avoid incorrectly skipping RGs on I/O error On IOException, nextDoc remains Integer.MAX_VALUE (its initial value), which will cause callers to skip all subsequent row groups even though the scorer is not exhausted. On partial-bitset return due to I/O error, set nextDoc to maxDoc (or leave handle.currentDoc unchanged and report it) so subsequent RGs are still evaluated. sandbox/plugins/analytics-backend-lucene/src/main/java/org/opensearch/be/lucene/LuceneFilterDelegationHandle.java [245-250] nextDoc = (handle.currentDoc == DocIdSetIterator.NO_MORE_DOCS) ? Integer.MAX_VALUE : Math.max(handle.currentDoc, maxDoc); } catch (IOException exception) { LOGGER.warn("IOException during collectDocs, returning partial bitset", exception); + nextDoc = maxDoc; } Suggestion importance[1-10]: 7 __ Why: Valid concern: on IOException, nextDoc stays at its initialized Integer.MAX_VALUE, which would cause callers to skip all remaining RGs despite the scorer not being exhausted. Setting nextDoc = maxDoc avoids the false-skip on partial results.	Medium
	Avoid pointer-identity keys for leaf tracking Using Arc::as_ptr(collector) as a stable identity is unsafe: if a collector Arc is dropped and a new one is allocated at the same address, the stale next_doc entry will be applied to a different collector, causing incorrect skipping. Also, the map grows unbounded across queries. Consider keying by (provider_key, collector_key) from ResolvedNode::Collector, or scoping the map's lifetime to a single query/evaluator instance. sandbox/plugins/analytics-backend-datafusion/rust/src/indexed_table/eval/bitmap_tree.rs [1020-1025] - let leaf_key = Arc::as_ptr(collector) as *const () as usize; + let leaf_key = match collector_node { + ResolvedNode::Collector { provider_key, .. } => *provider_key as usize, + _ => unreachable!(), + }; let mut last_next_doc = { let map = self.leaf_next_doc.lock().unwrap(); map.get(&leaf_key).copied().unwrap_or(i32::MIN) }; Suggestion importance[1-10]: 7 __ Why: Valid concern: using Arc::as_ptr as a stable identity is fragile if collectors get dropped and reallocated, and the map grows unbounded. Using a more stable key or scoping the map's lifetime would improve correctness.	Medium
	Use inclusive comparison for RG skip The skip condition uses > but the Java side returns nextDoc >= maxDoc as the exhausted-for-this-range sentinel (and stores Math.max(currentDoc, maxDoc)), so last_next == max_doc for a new RG whose min_doc == prev max_doc also means "no matches in this RG". Use >= (or compare against min_doc) to correctly skip when last_next equals the current RG's max_doc boundary and to avoid re-fetching an RG whose entire range is beyond last_next. sandbox/plugins/analytics-backend-datafusion/rust/src/indexed_table/eval/single_collector.rs [292-295] let mut last_next = self .last_next_doc .load(std::sync::atomic::Ordering::Acquire); - if last_next > max_doc { + if last_next >= max_doc { Suggestion importance[1-10]: 6 __ Why: Reasonable observation: since Java sets nextDoc = Math.max(currentDoc, maxDoc) on exhaustion, using >= instead of > may better align semantics and avoid unnecessary FFM calls. Impact is a minor optimization/correctness edge case.	Low
General	Restrict test-only conversion to prevent misuse Defaulting next_doc to i32::MIN in the From<Vec> conversion is fine for tests but risky if production code accidentally uses it: i32::MIN means "no skip info" only by convention, and downstream code that computes last_next.max(min_doc) will silently work but any comparison like last_next > max_doc could misbehave if the sentinel changes. Consider documenting the sentinel semantics on the field explicitly, or gate the From impl behind #[cfg(test)] to prevent misuse. sandbox/plugins/analytics-backend-datafusion/rust/src/indexed_table/index.rs [58-65] +#[cfg(test)] impl From<Vec<u64>> for CollectDocsResult { fn from(words: Vec<u64>) -> Self { CollectDocsResult { words, next_doc: i32::MIN, } } } Suggestion importance[1-10]: 4 __ Why: Minor maintainability suggestion; gating with #[cfg(test)] could prevent accidental misuse but the From impl is likely used by non-test code paths too, making the suggestion of limited impact.	Low

Suggestions up to commit edb60a7

Category	Suggestion	Impact
Possible bug	Avoid pointer-identity keys for leaves Using an Arc pointer as a stable identity key is unsafe: once an Arc is dropped its allocation can be reused by an unrelated collector, causing spurious next_doc reuse across leaves and potentially skipping valid RGs. Prefer a stable identifier stored on the collector/node (e.g., an annotation_id, provider key, or leaf index) or key on the DFS leaf index that's already passed in. sandbox/plugins/analytics-backend-datafusion/rust/src/indexed_table/eval/bitmap_tree.rs [1020] -let leaf_key = Arc::as_ptr(collector) as *const () as usize; +let leaf_key = _leaf_dfs_index; Suggestion importance[1-10]: 8 __ Why: Legitimate correctness concern: using an Arc raw pointer as a stable identity key is fragile because allocations can be reused after drop, potentially causing incorrect next_doc reuse across unrelated leaves. Using a stable identifier like _leaf_dfs_index is safer.	Medium
	Report accurate nextDoc without clamping Clamping nextDoc to Math.max(handle.currentDoc, maxDoc) is incorrect: if the iterator already advanced past scanTo but still lies within [minDoc, maxDoc) (e.g. scanTo < maxDoc due to partitionMaxDoc), the true next matching doc can be less than maxDoc. Return handle.currentDoc directly (only replacing with MAX_VALUE when exhausted) so callers get an accurate skip hint and don't miss subsequent matches. sandbox/plugins/analytics-backend-lucene/src/main/java/org/opensearch/be/lucene/LuceneFilterDelegationHandle.java [245-247] nextDoc = (handle.currentDoc == DocIdSetIterator.NO_MORE_DOCS) ? Integer.MAX_VALUE - : Math.max(handle.currentDoc, maxDoc); + : handle.currentDoc; Suggestion importance[1-10]: 7 __ Why: Valid concern: clamping nextDoc to Math.max(handle.currentDoc, maxDoc) can misreport the actual next matching doc when the iterator advanced within the range but stopped before maxDoc (e.g., due to partitionMaxDoc). This could cause incorrect skip decisions downstream.	Medium
	Guard against concurrent nextDoc races SingleCollectorEvaluator stores a single last_next_doc per evaluator, but prefetch_rg can be invoked concurrently from multiple partitions/threads on the same evaluator instance. Interleaved reads/writes of this atomic can cause one partition's next_doc from a later RG to incorrectly skip an earlier RG on another partition. Consider keying last_next_doc per-partition or per-thread, or documenting/enforcing single-threaded use. sandbox/plugins/analytics-backend-datafusion/rust/src/indexed_table/eval/single_collector.rs [292-295] let mut last_next = self .last_next_doc .load(std::sync::atomic::Ordering::Acquire); +// NOTE: assumes prefetch_rg is called serially per evaluator instance. if last_next > max_doc { Suggestion importance[1-10]: 7 __ Why: Valid concern about thread-safety: a single AtomicI32 shared across partitions can cause cross-partition interference, potentially skipping RGs incorrectly. Worth investigating whether partitions share the evaluator instance.	Medium
General	Decode packed nextDoc more defensively packed is i64; the sign-check packed < 0 only rules out the whole value being negative, but a legitimate nextDoc = Integer.MAX_VALUE with a valid wordsWritten results in a positive i64, while any nextDoc with the high-bit set (unlikely but possible if future callers ever encode differently) would be treated as error. More importantly, cast the low-32 correctly and consider validating next_doc >= min_doc to catch callback bugs early. sandbox/plugins/analytics-backend-datafusion/rust/src/indexed_table/ffm_callbacks.rs [243-245] let packed_u = packed as u64; let words_written = (packed_u & 0xFFFF_FFFF) as usize; -let next_doc = (packed_u >> 32) as i32; +let next_doc = ((packed_u >> 32) & 0xFFFF_FFFF) as u32 as i32; Suggestion importance[1-10]: 3 __ Why: The current decoding (packed_u >> 32) as i32 already correctly extracts the upper 32 bits; the suggested alternative is functionally equivalent. The remark about additional validation is minor.	Low

Suggestions up to commit 772f4f3

Category	Suggestion	Impact
Possible issue	Use exact sentinel for error check The packed < 0 error check is unreliable because valid packed values with nextDoc >= 0x80000000 (e.g., Integer.MAX_VALUE in Java is 0x7FFFFFFF, but any negative-signed high-bit interpretation) may not trigger it — however Java returns -1L on error, which as i64 is 0xFFFF_FFFF_FFFF_FFFF, and as packed this would decode to next_doc = -1, words = MAX. The check packed < 0 correctly catches -1L, but a valid packed value where the upper 32 bits form a negative i32 (any nextDoc with bit 31 set) would also make packed negative. Explicitly compare to -1 to distinguish the error sentinel from valid packed data. sandbox/plugins/analytics-backend-datafusion/rust/src/indexed_table/ffm_callbacks.rs [237-245] -if packed < 0 { +if packed == -1 { return Err(format!( "collectDocs(context_id={}, key={}) failed: {}", self.context_id, self.key, packed )); } let packed_u = packed as u64; let words_written = (packed_u & 0xFFFF_FFFF) as usize; let next_doc = (packed_u >> 32) as i32; Suggestion importance[1-10]: 7 __ Why: Valid concern: a valid packed value with nextDoc having bit 31 set (negative i32) would make the entire i64 negative and be misinterpreted as an error. However, in practice nextDoc values are non-negative doc IDs bounded by Integer.MAX_VALUE, so the risk is limited but the fix improves robustness.	Medium
	Return actual next matching docId Using Math.max(handle.currentDoc, maxDoc) masks the real next-matching docId when currentDoc < maxDoc (e.g., when the scanner stopped due to scanTo < maxDoc because of partition bounds). This can cause callers to under-skip. Return handle.currentDoc directly (it is already >= scanTo after the loop when not exhausted, and is the true next matching doc otherwise). sandbox/plugins/analytics-backend-lucene/src/main/java/org/opensearch/be/lucene/LuceneFilterDelegationHandle.java [245-247] nextDoc = (handle.currentDoc == DocIdSetIterator.NO_MORE_DOCS) ? Integer.MAX_VALUE - : Math.max(handle.currentDoc, maxDoc); + : handle.currentDoc; Suggestion importance[1-10]: 6 __ Why: Valid observation: Math.max(handle.currentDoc, maxDoc) inflates nextDoc when the scanner stops early due to partition bounds (scanTo < maxDoc), potentially causing incorrect skip decisions. However, the impact depends on whether callers rely on the exact next-doc semantics for correctness or just for skip optimization.	Low
General	Fix off-by-one in skip condition **The skip condition last_next_doc > *call_max combined with effective_min = last_next_doc.max(call_min) can produce effective_min == call_max (empty range) when last_next_doc == call_max, which is wasteful. Also, if last_next_doc >= call_max the range is empty and should be skipped. Use >= to match the doc-file semantics ("next matching docId beyond maxDoc" — an exclusive upper bound). sandbox/plugins/analytics-backend-datafusion/rust/src/indexed_table/eval/bitmap_tree.rs [1034-1038] -if last_next_doc > *call_max { +if last_next_doc >= *call_max { continue; } let effective_min = last_next_doc.max(*call_min); let result = collector.collect_packed_u64_bitset(effective_min, *call_max)?; Suggestion importance[1-10]: 5 __ Why: Reasonable minor optimization: when last_next_doc == call_max, the range [call_max, call_max) is empty and calling the collector is wasteful. Using >= avoids the unnecessary call, though it's a boundary optimization rather than a correctness bug.	Low
	Use inclusive comparison for skip The same off-by-one issue exists here: next_doc is documented as "next matching docId beyond max_doc", so when last_next == max_doc there is still no match inside [min_doc, max_doc) and the RG can be skipped. Use >= for consistency with the FFI contract and to avoid an unnecessary FFM call in the boundary case. sandbox/plugins/analytics-backend-datafusion/rust/src/indexed_table/eval/single_collector.rs [292-295] let mut last_next = self .last_next_doc .load(std::sync::atomic::Ordering::Acquire); -if last_next > max_doc { +if last_next >= max_doc { Suggestion importance[1-10]: 5 __ Why: Same boundary optimization for consistency: when last_next == max_doc, no match can exist in [min_doc, max_doc), so >= is more precise. Minor efficiency improvement.	Low

[github-actions]

Persistent review updated to latest commit 55cac5b

[github-actions]

Persistent review updated to latest commit 454eafc

[github-actions]

❌ Gradle check result for 454eafc: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

Persistent review updated to latest commit e7cda94

[github-actions]

✅ Gradle check result for e7cda94: SUCCESS

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 71.43%. Comparing base (dbde5fb) to head (edb60a7).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #22493      +/-   ##
============================================
- Coverage     73.41%   71.43%   -1.98%     
- Complexity    76419    76625     +206     
============================================
  Files          6104     6132      +28     
  Lines        346628   357308   +10680     
  Branches      49886    52076    +2190     
============================================
+ Hits         254469   255241     +772     
- Misses        71860    81748    +9888     
- Partials      20299    20319      +20     

☔ 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.

[github-actions]

Persistent review updated to latest commit abcdfd2

[github-actions]

Persistent review updated to latest commit f06f05c

[github-actions]

Persistent review updated to latest commit 4051f8a

[github-actions]

❌ Gradle check result for 4051f8a: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

Persistent review updated to latest commit c8c76ac

[github-actions]

❌ Gradle check result for c8c76ac: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

Persistent review updated to latest commit 15a465a

[github-actions]

❌ Gradle check result for 15a465a: null

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

Persistent review updated to latest commit f24bb98

[github-actions]

✅ Gradle check result for f24bb98: SUCCESS

[github-actions]

Persistent review updated to latest commit b7c9979

[github-actions]

❌ Gradle check result for b7c9979: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

Persistent review updated to latest commit 2fb7234

[github-actions]

Persistent review updated to latest commit f2ced7a

[github-actions]

❌ Gradle check result for f2ced7a: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

Persistent review updated to latest commit 772f4f3

[github-actions]

❌ Gradle check result for 772f4f3: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

Persistent review updated to latest commit edb60a7

[github-actions]

✅ Gradle check result for edb60a7: SUCCESS

[alchemist51]

Can we extend this into the prefetch thread as well?

[alchemist51]

let's a single line above it for readability

[alchemist51]

Should we use the effective min here as well?

[aasom143]

No, this is the performance-delegated collector path, which creates a fresh collector per RG with its own independent Lucene scorer.

[aasom143]

Can we extend this into the prefetch thread as well?

All these changes are in prefetch already. What changes do you think is not present in prefetch?

[alchemist51]

Can we use the effective doc min here?

[aasom143]

We can't update here as page pruning happened using the original min_doc.

[alchemist51]

We can use the effective min here as well?

[alchemist51]

Can we extend this into the prefetch thread as well?

All these changes are in prefetch already. What changes do you think is not present in prefetch?

fn prefetch_rg(
        &self,
        rg: &RowGroupInfo,
        min_doc: i32,
        max_doc: i32,
    )

I'm slightly concerned on how we are changing the min_doc in few places in the prefetch_rg function. Can we not send the effective min doc from the caller itself? Currently feels like it's a patch and is leaking the details at different point

[github-actions]

Persistent review updated to latest commit 9984bf2

[github-actions]

❌ Gradle check result for 9984bf2: null

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

Persistent review updated to latest commit 12632af

[github-actions]

Persistent review updated to latest commit 98014ef

[github-actions]

❌ Gradle check result for 98014ef: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

Persistent review updated to latest commit 714ea99

[github-actions]

❌ Gradle check result for 714ea99: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?