[Analytics Engine] Add data node row ID emission and fetch-by-row-ids for QTF late materialization

[alchemist51]

Adds the data-node half of Query-Then-Fetch late materialization:

• Row ID emission (indexed path): Compute __row_id__ from position via PositionMap in finalize_batch for all filter classifications (SingleCollector, Tree, predicate-only without sort)
• Row ID emission (ListingTable path): ShardTableProvider with row_base partition column + ProjectRowIdOptimizer for queries requiring data-node sort without a collector
• Fetch-by-row-ids: Rust execution path that resolves global IDs → per-file positions → ParquetAccessPlan with row-level RowSelection, skipping entire row groups with no target rows
• Reader context: ReaderContextStore holds shard readers open across query→fetch phases with configurable keep-alive (analytics.qtf.reader_context.keep_alive, default 5m) and reaper
• Shared helpers: Extracted build_query_runtime_env, build_shard_file_infos, store_url_from_table_path for reuse across query and fetch paths

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 449ed67)

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 Race Condition The closed flag is checked without synchronization before the inUse CAS in markInUse(). A thread could see closed=false, then another thread sets closed=true and closes the reader, then the first thread proceeds with the CAS and returns true, allowing use of a closed reader. The check and CAS must be atomic or the flag must be volatile with proper ordering guarantees. public boolean markInUse() { if (closed) return false; lastAccessTime.set(System.currentTimeMillis()); return inUse.compareAndSet(false, true); Reaper Concurrency The reaper iterates activeContexts.values() and calls freeContext() which modifies the map during iteration. ConcurrentHashMap's iterator is weakly consistent and won't throw ConcurrentModificationException, but freeContext() removes the entry the iterator just returned. If another thread acquires the context between isExpired() check and freeContext() call, the context is removed while in use, causing the reader to be closed under an active fetch operation. private class Reaper implements Runnable { @Override public void run() { for (ReaderContext ctx : activeContexts.values()) { if (ctx.isExpired()) { logger.debug("[ReaderContextStore] Freeing expired context for query={}", ctx.getQueryId()); freeContext(ctx.getQueryId()); } } } Unsafe Pointer Dereference decode_file_metadata dereferences raw pointers from FFI without validating alignment or that the memory region is actually allocated and accessible. If Java passes a misaligned pointer or one pointing to unmapped memory (e.g., after GC moves objects), this causes undefined behavior. The function should validate pointer alignment and consider using a safer FFI boundary with explicit memory ownership transfer. pub unsafe fn decode_file_metadata(ptr: i64, count: usize) -> Option<Vec<FileRowMetadata>> { if ptr == 0 || count == 0 { return None; } let wire_slice = std::slice::from_raw_parts(ptr as *const WireFileMetadata, count); let mut result = Vec::with_capacity(count); for wire in wire_slice { let num_rgs = wire.num_row_groups as usize; let rg_counts = if wire.row_group_row_counts_ptr == 0 || num_rgs == 0 { Vec::new() } else { let counts_ptr = wire.row_group_row_counts_ptr as *const i64; std::slice::from_raw_parts(counts_ptr, num_rgs) .iter() .map(|&c| c as u64) .collect() }; result.push(FileRowMetadata { row_group_row_counts: rg_counts, }); } Some(result) } Stale Documentation The writeTo method's documentation at line 137 still states "at least 68 bytes" but BYTE_SIZE is now 72. The comment should be updated to match the new struct size to avoid confusion during debugging or when reasoning about buffer allocation. * @param segment the target memory segment (at least 68 bytes)

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 449ed67

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Reuse reader from context store The executeFetchByRowIds method acquires a new reader via readerProvider.acquireReader() instead of reusing the reader stored in the query context. This violates the QTF design where the query phase acquires a reader, stores it in ReaderContext, and the fetch phase reuses the same reader. The method should retrieve the reader from ReaderContextStore using the queryId, not acquire a fresh one. sandbox/plugins/analytics-engine/src/main/java/org/opensearch/analytics/exec/AnalyticsSearchService.java [229-257] public org.opensearch.analytics.backend.EngineResultStream executeFetchByRowIds( String queryId, long[] rowIds, String[] columns, IndexShard shard ) { - IndexReaderProvider readerProvider = shard.getReaderProvider(); - if (readerProvider == null) { - throw new IllegalStateException("No ReaderProvider on " + shard.shardId()); + ReaderContext ctx = readerContextStore.acquireContext(queryId); + if (ctx == null) { + throw new IllegalStateException("No reader context for query " + queryId); } try { - GatedCloseable<Reader> gatedReader = readerProvider.acquireReader(); + Reader reader = ctx.getReader(); ... } catch (Exception e) { + readerContextStore.releaseContext(queryId); throw new RuntimeException("Failed to execute fetch-by-row-ids on " + shard.shardId(), e); } } Suggestion importance[1-10]: 9 __ Why: This is a critical design flaw. The method acquires a fresh reader instead of reusing the one from the query phase, violating the core QTF design where the same reader must be used across both phases. The suggested fix correctly retrieves the reader from ReaderContextStore, ensuring consistency and preventing resource leaks.	High
	Fix race condition in markInUse The markInUse() method has a race condition. If the context is closed between the if (closed) check and the compareAndSet call, the method could successfully mark the context as in-use even though it's closed. This violates the invariant that closed contexts cannot be acquired. Move the closed check inside a synchronized block or use atomic operations to ensure atomicity. sandbox/plugins/analytics-engine/src/main/java/org/opensearch/analytics/exec/ReaderContext.java [59-63] public boolean markInUse() { - if (closed) return false; lastAccessTime.set(System.currentTimeMillis()); - return inUse.compareAndSet(false, true); + if (!inUse.compareAndSet(false, true)) { + return false; + } + if (closed) { + inUse.set(false); + return false; + } + return true; } Suggestion importance[1-10]: 8 __ Why: The race condition between the closed check and compareAndSet is a critical concurrency bug. If the context is closed after the check but before the CAS, the method could mark a closed context as in-use, violating the invariant that closed contexts cannot be acquired. The suggested fix properly handles this by checking closed after the CAS and rolling back if needed.	Medium
	Prevent closing in-use reader contexts The freeContext method doesn't verify that the context is not currently in-use before closing it. If a fetch phase is actively using the context (marked in-use), calling freeContext will close the reader while it's being accessed, leading to resource corruption. Add a check to ensure the context is not in-use, or force mark it as done before closing. sandbox/plugins/analytics-engine/src/main/java/org/opensearch/analytics/exec/ReaderContextStore.java [101-110] public void freeContext(String queryId) { ReaderContext ctx = activeContexts.remove(queryId); if (ctx != null) { + ctx.markDone(); try { ctx.close(); } catch (Exception e) { logger.warn("[ReaderContextStore] Failed to close context for query={}", queryId, e); } } } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that freeContext could close a reader while it's actively being used. Adding ctx.markDone() before closing ensures the context is marked as not-in-use, preventing concurrent access issues. However, this doesn't fully prevent the race if another thread is between markInUse and actual usage.	Medium
	Prevent row ID overflow The conversion from u64 to i64 via as cast can silently overflow for row IDs exceeding i64::MAX. For large shards, this could produce negative or incorrect row IDs. Add overflow checking or use a saturating conversion. sandbox/plugins/analytics-backend-datafusion/rust/src/indexed_table/row_id_injection.rs [57-68] let ids = compute_row_ids( &ctx.eval_mask, current_mask, mask_offset_before, batch_len, ctx.batch_offset, ctx.position_map.as_ref(), ctx.base, ); -Arc::new(Int64Array::from_iter_values(ids.into_iter().map(|id| id as i64))) +Arc::new(Int64Array::from_iter_values(ids.into_iter().map(|id| i64::try_from(id).unwrap_or(i64::MAX)))) Suggestion importance[1-10]: 7 __ Why: Valid concern about potential overflow when converting u64 to i64 for large row IDs. The suggested try_from with unwrap_or(i64::MAX) provides safer handling, though the likelihood depends on shard sizes in practice.	Medium
General	Use proper plan inspection The substring search for row_id in raw plan bytes is fragile and may produce false positives if the string appears in comments, literals, or other non-column contexts. Use proper plan inspection via plan_requests_row_ids after decoding instead of byte-level matching. sandbox/plugins/analytics-backend-datafusion/rust/src/ffm.rs [760] -let has_row_id = plan_bytes.windows(crate::ROW_ID_COLUMN_NAME.len()).any(|w| w == crate::ROW_ID_COLUMN_NAME.as_bytes()); +let plan = decode_substrait_plan(plan_bytes) + .map_err(|e| format!("decode substrait: {}", e))?; +let has_row_id = crate::indexed_table::substrait_to_tree::plan_requests_row_ids(&plan); Suggestion importance[1-10]: 8 __ Why: The byte-level substring search for __row_id__ is indeed fragile and could produce false positives. Using proper plan inspection via plan_requests_row_ids after decoding the substrait plan would be more robust and semantically correct.	Medium
	Validate row IDs are within bounds The fetch_by_row_ids function doesn't validate that row_ids are within valid bounds for the shard. If a caller passes row IDs that exceed the total row count across all segments, the function will create empty access plans but won't fail explicitly. Add validation to check that all row IDs fall within [0, total_rows) and return an error for out-of-bounds IDs. sandbox/plugins/analytics-backend-datafusion/rust/src/api.rs [500-507] pub async unsafe fn fetch_by_row_ids( shard_view: &ShardView, runtime: &DataFusionRuntime, manager: &crate::runtime_manager::RuntimeManager, row_ids: Vec<i64>, columns: Vec<String>, ) -> Result<i64, DataFusionError> { + let total_rows: i64 = segments.iter().map(|s| s.max_doc).sum(); + for &id in &row_ids { + if id < 0 || id >= total_rows { + return Err(DataFusionError::Execution( + format!("Row ID {} out of bounds [0, {})", id, total_rows) + )); + } + } + ... Suggestion importance[1-10]: 6 __ Why: Adding validation for out-of-bounds row IDs improves robustness and provides clearer error messages. However, the suggested code references segments before it's defined in the function (it's built later at line 535). The validation would need to be moved after segment building, reducing the impact slightly.	Low

---

Previous suggestions

Suggestions up to commit c274c9b

Category	Suggestion	Impact
Possible issue	Handle position lookup failures The unwrap_or(delivered_idx) fallback silently masks position map lookup failures, potentially returning incorrect row IDs. This could cause data corruption in the fetch phase. Consider propagating the error or at minimum logging a warning. sandbox/plugins/analytics-backend-datafusion/rust/src/indexed_table/row_id_injection.rs [131-138] -fn position_to_global_id(delivered_idx: usize, pm: Option<&PositionMap>, base: u64) -> u64 { +fn position_to_global_id(delivered_idx: usize, pm: Option<&PositionMap>, base: u64) -> Result<u64, String> { let rg_pos = match pm { - Some(p) => p.rg_position(delivered_idx).unwrap_or(delivered_idx), + Some(p) => p.rg_position(delivered_idx) + .ok_or_else(|| format!("position_map lookup failed for idx {}", delivered_idx))?, None => delivered_idx, }; - base + rg_pos as u64 + Ok(base + rg_pos as u64) } Suggestion importance[1-10]: 9 __ Why: The suggestion identifies a critical correctness issue: silently falling back to delivered_idx when rg_position lookup fails could return incorrect row IDs, leading to data corruption in the fetch phase. The proposed solution to propagate errors is appropriate for ensuring data integrity. This is a high-impact suggestion addressing a potential bug.	High
	Validate row IDs are within bounds The row ID to segment mapping assumes row_ids are valid global IDs within the shard's range, but doesn't validate this. If a row ID is out of bounds (negative, or exceeds total rows), partition_point may return an incorrect segment index, leading to wrong data or panics. Add bounds checking to reject invalid row IDs early. sandbox/plugins/analytics-backend-datafusion/rust/src/api.rs [541-549] -pub async unsafe fn fetch_by_row_ids( - shard_view: &ShardView, - runtime: &DataFusionRuntime, - manager: &crate::runtime_manager::RuntimeManager, - row_ids: Vec<i64>, - columns: Vec<String>, -) -> Result<i64, DataFusionError> { - ... - for &gid in &row_ids { - let seg_idx = segments - .partition_point(|s| s.global_base <= gid as u64) - .saturating_sub(1); - let local_pos = (gid as u64 - segments[seg_idx].global_base) as u32; - per_segment.entry(seg_idx).or_default().insert(local_pos); +for &gid in &row_ids { + if gid < 0 { + return Err(DataFusionError::Execution(format!("Invalid row_id: {}", gid))); } - ... + let seg_idx = segments + .partition_point(|s| s.global_base <= gid as u64) + .saturating_sub(1); + if seg_idx >= segments.len() { + return Err(DataFusionError::Execution(format!("Row ID {} out of range", gid))); + } + let seg = &segments[seg_idx]; + let local_pos = gid as u64 - seg.global_base; + if local_pos >= seg.max_doc as u64 { + return Err(DataFusionError::Execution(format!("Row ID {} exceeds segment bounds", gid))); + } + per_segment.entry(seg_idx).or_default().insert(local_pos as u32); } Suggestion importance[1-10]: 8 __ Why: Critical validation missing. The code assumes all row_ids are valid without checking bounds. Invalid row IDs (negative or exceeding segment bounds) could cause incorrect data retrieval or panics. The suggested bounds checking is essential for robustness and should be added to prevent runtime errors from malformed input.	Medium
	Use proper plan parsing Scanning the entire substrait plan bytes for row_id string is fragile and may produce false positives (e.g., matching the string in comments or other contexts). Use proper substrait plan parsing to detect row ID projection instead of byte pattern matching. sandbox/plugins/analytics-backend-datafusion/rust/src/ffm.rs [725] -let has_row_id = plan_bytes.windows(crate::ROW_ID_COLUMN_NAME.len()).any(|w| w == crate::ROW_ID_COLUMN_NAME.as_bytes()); +// Parse substrait plan to detect __row_id__ projection +let has_row_id = { + let plan = substrait::proto::Plan::decode(plan_bytes) + .map_err(|e| format!("decode substrait: {}", e))?; + crate::indexed_table::substrait_to_tree::plan_requests_row_ids(&plan) +}; Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies a critical issue: byte pattern matching for __row_id__ in the substrait plan is fragile and can produce false positives. The proposed solution to use proper substrait plan parsing is the right approach. However, the improved code references a function plan_requests_row_ids that takes a substrait proto Plan, but the actual implementation in the PR takes a LogicalPlan, so the suggestion needs adjustment.	Medium
	Auto-release reader context when stream closes The executeFetchByRowIds method acquires a reader context but only releases it on exception. If the fetch succeeds, the context remains marked as in-use indefinitely, preventing the reaper from cleaning it up. The caller must explicitly call completeFetch after consuming the stream, but this creates a resource leak if the caller forgets. Consider wrapping the stream to auto-release the context when closed. sandbox/plugins/analytics-engine/src/main/java/org/opensearch/analytics/exec/AnalyticsSearchService.java [142-177] public org.opensearch.analytics.backend.EngineResultStream executeFetchByRowIds( String queryId, long[] rowIds, String[] columns, IndexShard shard ) { if (readerContextStore == null) { throw new IllegalStateException("ReaderContextStore not initialized"); } ReaderContext readerCtx = readerContextStore.acquireContext(queryId); if (readerCtx == null) { throw new IllegalStateException("No reader context for queryId=" + queryId + " on shard " + shard.shardId()); } try { ... - return stream; + return new AutoCloseableEngineResultStream(stream, () -> { + readerCtx.markDone(); + readerContextStore.freeContext(queryId); + }); } catch (Exception e) { readerCtx.markDone(); + readerContextStore.freeContext(queryId); throw new RuntimeException("Failed to execute fetch-by-row-ids on " + shard.shardId(), e); } } Suggestion importance[1-10]: 7 __ Why: Valid concern about resource leaks if completeFetch is not called. The current design requires explicit cleanup by the caller, which is error-prone. Wrapping the stream to auto-release the context on close would make the API safer and prevent leaks if callers forget to call completeFetch.	Medium
	Ensure reader context cleanup on failure The executeFragment method creates a new FragmentResources context that wraps the reader, but when readerContextStore is enabled, the reader lifecycle is managed by ReaderContext. If the fragment execution fails, the reader may be closed twice (once by FragmentResources, once by ReaderContext cleanup). Consider ensuring the reader wrapper passed to FragmentResources doesn't close the underlying reader when the context store is active. sandbox/plugins/analytics-engine/src/main/java/org/opensearch/analytics/exec/AnalyticsSearchService.java [104-123] -public FragmentExecutionResponse executeFragment(FragmentExecutionRequest request, IndexShard shard) { - return executeFragment(request, shard, null); -} - public FragmentExecutionResponse executeFragment(FragmentExecutionRequest request, IndexShard shard, AnalyticsShardTask task) { ResolvedFragment resolved = resolveFragment(request, shard); long startNanos = System.nanoTime(); try (FragmentResources ctx = startFragment(request, resolved, shard, task)) { FragmentExecutionResponse response = collectResponse(ctx.stream(), task); long tookNanos = System.nanoTime() - startNanos; listener.onFragmentSuccess(resolved.queryId, resolved.stageId, resolved.shardIdStr, tookNanos, response.getRowCount()); return response; } catch (TaskCancelledException | IllegalStateException | IllegalArgumentException e) { listener.onFragmentFailure(resolved.queryId, resolved.stageId, resolved.shardIdStr, e); + if (readerContextStore != null) { + readerContextStore.releaseContext(resolved.queryId); + } throw e; } catch (Exception e) { listener.onFragmentFailure(resolved.queryId, resolved.stageId, resolved.shardIdStr, e); + if (readerContextStore != null) { + readerContextStore.releaseContext(resolved.queryId); + } throw new RuntimeException("Failed to execute fragment on " + shard.shardId(), e); } } Suggestion importance[1-10]: 3 __ Why: The suggestion identifies a potential double-close issue, but the code already handles this via the non-closing wrapper created at line 197 (readerForFragment). The wrapper's close callback only calls releaseContext, not the underlying reader's close. The explicit releaseContext calls in catch blocks would be redundant since the try-with-resources already handles cleanup.	Low
General	Avoid unnecessary array clone The eval_mask.clone() creates an unnecessary deep copy of potentially large boolean arrays on every batch. Since eval_mask is consumed after row ID computation, consider using Option::take() or restructuring to avoid the clone. sandbox/plugins/analytics-backend-datafusion/rust/src/indexed_table/stream.rs [585-595] let row_id_ctx = if self.row_id_output_index.is_some() { Some(super::row_id_injection::RowIdContext { batch_offset: self.batch_offset, position_map: self.current_position_map.as_ref().cloned(), base: self.global_base + self.current_rg_first_row as u64, - eval_mask: eval_mask.clone(), + eval_mask: eval_mask.as_ref().cloned(), }) } else { None }; Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies that eval_mask.clone() creates a deep copy of potentially large boolean arrays. However, the proposed fix using eval_mask.as_ref().cloned() is semantically identical to eval_mask.clone() for Option<BooleanArray>. A better approach would require restructuring the code flow to avoid the clone entirely, which the suggestion doesn't fully address.	Low
	Handle query-fetch transition race condition The acquireContext method returns null if the context is already in-use, but this creates a race condition in QTF: if the query phase hasn't released the context yet when the fetch phase arrives, the fetch will fail with null. Consider adding a short retry loop or blocking wait to handle the transition window between query completion and context release. sandbox/plugins/analytics-engine/src/main/java/org/opensearch/analytics/exec/ReaderContextStore.java [79-86] public ReaderContext acquireContext(String queryId) { ReaderContext ctx = activeContexts.get(queryId); if (ctx == null) return null; - if (ctx.markInUse()) { - return ctx; + + // Retry a few times to handle race between query release and fetch acquire + for (int i = 0; i < 3; i++) { + if (ctx.markInUse()) { + return ctx; + } + try { + Thread.sleep(10); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + return null; + } } return null; } Suggestion importance[1-10]: 5 __ Why: The suggestion identifies a potential race condition between query phase release and fetch phase acquire. However, adding a retry loop with sleep is a workaround rather than a proper solution. A better approach would be using proper synchronization primitives or ensuring the query phase doesn't release until fetch is ready. The suggested fix is functional but not ideal.	Low
	Eliminate redundant type conversion The compute_row_ids returns Vec which is then converted to i64 via iterator mapping. This creates an intermediate allocation. Consider returning Vec directly from compute_row_ids to eliminate the conversion step. sandbox/plugins/analytics-backend-datafusion/rust/src/indexed_table/row_id_injection.rs [57-68] let ids = compute_row_ids( &ctx.eval_mask, current_mask, mask_offset_before, batch_len, ctx.batch_offset, ctx.position_map.as_ref(), ctx.base, ); -Arc::new(Int64Array::from_iter_values(ids.into_iter().map(|id| id as i64))) +Arc::new(Int64Array::from(ids)) Suggestion importance[1-10]: 5 __ Why: The suggestion identifies a valid optimization opportunity by proposing to return Vec<i64> directly from compute_row_ids instead of Vec<u64>. However, this would require changing the return type and all call sites of compute_row_ids, which the suggestion doesn't fully demonstrate. The impact is moderate as it eliminates one allocation and conversion step.	Low

Suggestions up to commit c60165b

Category	Suggestion	Impact
Possible issue	Prevent Arrow vector memory leak The rowIdVector is allocated but never closed if an exception occurs before returning the stream. This causes a memory leak. Wrap the allocation in a try-with-resources or ensure cleanup in the catch block. Also, readerCtx.markDone() should be called after successful completion, not on error. sandbox/plugins/analytics-engine/src/main/java/org/opensearch/analytics/exec/AnalyticsSearchService.java [150-185] public org.opensearch.analytics.backend.EngineResultStream executeFetchByRowIds( String queryId, long[] rowIds, String[] columns, IndexShard shard ) { if (readerContextStore == null) { throw new IllegalStateException("ReaderContextStore not initialized"); } ReaderContext readerCtx = readerContextStore.acquireContext(queryId); if (readerCtx == null) { throw new IllegalStateException("No reader context for queryId=" + queryId + " on shard " + shard.shardId()); } + org.apache.arrow.vector.BigIntVector rowIdVector = null; try { - org.apache.arrow.vector.BigIntVector rowIdVector = new org.apache.arrow.vector.BigIntVector("__row_id__", allocator); + rowIdVector = new org.apache.arrow.vector.BigIntVector("__row_id__", allocator); rowIdVector.allocateNew(rowIds.length); for (int i = 0; i < rowIds.length; i++) { rowIdVector.set(i, rowIds[i]); } rowIdVector.setValueCount(rowIds.length); AnalyticsSearchBackendPlugin backend = backends.values().iterator().next(); - org.opensearch.analytics.backend.EngineResultStream stream = backend.fetchByRowIds( - readerCtx.getReader(), - rowIdVector, - columns, - allocator - ); - return stream; + return backend.fetchByRowIds(readerCtx.getReader(), rowIdVector, columns, allocator); } catch (Exception e) { + if (rowIdVector != null) { + rowIdVector.close(); + } readerCtx.markDone(); throw new RuntimeException("Failed to execute fetch-by-row-ids on " + shard.shardId(), e); } } Suggestion importance[1-10]: 9 __ Why: Critical resource leak issue. The rowIdVector is allocated but never closed if an exception occurs. The suggestion correctly identifies that rowIdVector.close() should be called in the catch block. Additionally, readerCtx.markDone() should only be called after successful completion, not on error, as the context needs to remain available for retry or cleanup.	High
	Validate row IDs before indexing If row_ids contains a negative value or a value less than the first segment's global_base, partition_point returns 0, then saturating_sub(1) wraps to usize::MAX, causing an out-of-bounds panic. Validate row IDs are non-negative and within bounds before indexing. sandbox/plugins/analytics-backend-datafusion/rust/src/api.rs [520-526] -pub async unsafe fn fetch_by_row_ids( - shard_view: &ShardView, - runtime: &DataFusionRuntime, - manager: &crate::runtime_manager::RuntimeManager, - row_ids: Vec<i64>, - columns: Vec<String>, -) -> Result<i64, DataFusionError> { - ... - for &gid in &row_ids { - let seg_idx = segments - .partition_point(|s| s.global_base <= gid as u64) - .saturating_sub(1); - let local_pos = (gid as u64 - segments[seg_idx].global_base) as u32; - per_segment.entry(seg_idx).or_default().insert(local_pos); +for &gid in &row_ids { + if gid < 0 { + return Err(DataFusionError::Execution(format!("Invalid row_id: {}", gid))); } + let seg_idx = segments + .partition_point(|s| s.global_base <= gid as u64) + .saturating_sub(1); + if seg_idx >= segments.len() { + return Err(DataFusionError::Execution(format!("row_id {} out of bounds", gid))); + } + let local_pos = (gid as u64 - segments[seg_idx].global_base) as u32; + per_segment.entry(seg_idx).or_default().insert(local_pos); +} Suggestion importance[1-10]: 9 __ Why: Critical bounds checking issue. If row_ids contains a negative value or a value less than the first segment's global_base, partition_point returns 0, then saturating_sub(1) wraps to usize::MAX, causing a panic when indexing segments[seg_idx]. The suggestion correctly adds validation for negative values and out-of-bounds checks.	High
	Fix race condition in context acquisition Race condition: between get() and markInUse(), another thread could close the context via freeContext(). The context would be removed from the map but markInUse() might still succeed on a stale reference. Use computeIfPresent with atomic mark-in-use to prevent this. sandbox/plugins/analytics-engine/src/main/java/org/opensearch/analytics/exec/ReaderContextStore.java [79-86] public ReaderContext acquireContext(String queryId) { - ReaderContext ctx = activeContexts.get(queryId); - if (ctx == null) return null; - if (ctx.markInUse()) { - return ctx; - } - return null; + return activeContexts.computeIfPresent(queryId, (k, ctx) -> { + return ctx.markInUse() ? ctx : null; + }); } Suggestion importance[1-10]: 8 __ Why: Valid race condition between get() and markInUse(). Using computeIfPresent with atomic mark-in-use prevents the context from being freed between the lookup and the mark operation. The improved code is more concise and thread-safe.	Medium
	Replace fragile byte search Using byte-level substring search on serialized Substrait plan bytes is fragile and may produce false positives if the column name appears in other contexts (e.g., as part of a longer identifier or in metadata). Consider parsing the plan structure to reliably detect row_id column references. sandbox/plugins/analytics-backend-datafusion/rust/src/ffm.rs [724] -let has_row_id = plan_bytes.windows(crate::ROW_ID_COLUMN_NAME.len()).any(|w| w == crate::ROW_ID_COLUMN_NAME.as_bytes()); +let has_row_id = crate::indexed_table::substrait_to_tree::plan_requests_row_ids_from_bytes(plan_bytes); Suggestion importance[1-10]: 8 __ Why: Critical correctness issue. The byte-level substring search on serialized Substrait bytes is fragile and could produce false positives. The suggestion to use proper plan parsing via plan_requests_row_ids is the correct approach, though the suggested function name doesn't exist in the PR.	Medium
	Enforce non-null threadPool parameter The conditional initialization of readerContextStore based on null check creates inconsistent behavior. If threadPool is null, executeFetchByRowIds and completeFetch will throw IllegalStateException at runtime. Either make threadPool required (non-null) or handle the null case gracefully in all methods that use readerContextStore. sandbox/plugins/analytics-engine/src/main/java/org/opensearch/analytics/exec/AnalyticsSearchService.java [93-103] public AnalyticsSearchService( Map<String, AnalyticsSearchBackendPlugin> backends, List<AnalyticsOperationListener> listeners, NamedWriteableRegistry namedWriteableRegistry, org.opensearch.threadpool.ThreadPool threadPool ) { this.backends = backends; this.listener = new AnalyticsOperationListener.CompositeListener(listeners); this.allocator = ArrowAllocatorProvider.newChildAllocator("analytics-search-service", Long.MAX_VALUE); this.namedWriteableRegistry = namedWriteableRegistry; - this.readerContextStore = threadPool != null ? new ReaderContextStore(threadPool) : null; + this.readerContextStore = new ReaderContextStore(Objects.requireNonNull(threadPool, "threadPool must not be null")); } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that conditional initialization of readerContextStore creates inconsistent behavior. However, the PR already has multiple constructors that delegate to the main constructor, and the null check is intentional to support backward compatibility. The improved code would break existing constructors that pass null. A better fix would be to validate in methods that use readerContextStore, which is already done.	Medium
General	Eliminate redundant type conversion The compute_row_ids function returns Vec which is then converted to i64 via an iterator chain. This creates an intermediate allocation and performs redundant iteration. Consider having compute_row_ids return Vec directly to eliminate the conversion overhead. sandbox/plugins/analytics-backend-datafusion/rust/src/indexed_table/row_id_injection.rs [57-68] let ids = compute_row_ids( &ctx.eval_mask, current_mask, mask_offset_before, batch_len, ctx.batch_offset, ctx.position_map.as_ref(), ctx.base, ); -Arc::new(Int64Array::from_iter_values(ids.into_iter().map(|id| id as i64))) +Arc::new(Int64Array::from(ids)) Suggestion importance[1-10]: 6 __ Why: Valid optimization suggestion. The intermediate Vec<u64> to Vec<i64> conversion via iterator mapping creates unnecessary overhead. Returning Vec<i64> directly from compute_row_ids would be more efficient.	Low
	Avoid unnecessary array cloning The eval_mask.clone() creates an unnecessary copy of potentially large boolean arrays on every batch. Since eval_mask is consumed later in the function, consider moving it into the context instead of cloning, or restructure to avoid the clone when row IDs aren't needed. sandbox/plugins/analytics-backend-datafusion/rust/src/indexed_table/stream.rs [585-595] let row_id_ctx = if self.row_id_output_index.is_some() { Some(super::row_id_injection::RowIdContext { batch_offset: self.batch_offset, position_map: self.current_position_map.as_ref().cloned(), base: self.global_base + self.current_rg_first_row as u64, - eval_mask: eval_mask.clone(), + eval_mask: eval_mask.as_ref().cloned(), }) } else { None }; Suggestion importance[1-10]: 5 __ Why: The suggestion correctly identifies a potential performance issue with cloning eval_mask, but the proposed fix (eval_mask.as_ref().cloned()) doesn't actually avoid the clone. A better approach would be to restructure the code to avoid cloning entirely when possible.	Low
	Add defensive error message The unwrap() on row_id_ctx assumes it's always Some when row_id_output_index is Some, but this invariant isn't enforced at compile time. If the earlier logic changes, this could panic. Use pattern matching or add an assertion with a descriptive error message. sandbox/plugins/analytics-backend-datafusion/rust/src/indexed_table/stream.rs [638-639] let output = if let Some(row_id_idx) = self.row_id_output_index { - let ctx = row_id_ctx.unwrap(); + let ctx = row_id_ctx.expect("row_id_ctx must be Some when row_id_output_index is set"); ... } else if output.num_columns() > self.schema.fields().len() { Suggestion importance[1-10]: 4 __ Why: Minor improvement. Replacing unwrap() with expect() and a descriptive message makes debugging easier if the invariant is violated, though the logic appears correct as written.	Low

Suggestions up to commit 3ede2af

Category	Suggestion	Impact
General	Validate strategy before expensive operations The error message correctly prevents misuse, but the check occurs after schema inference and table registration work has been done. Move this validation to the beginning of the function to fail fast and avoid unnecessary I/O operations. sandbox/plugins/analytics-backend-datafusion/rust/src/query_executor.rs [107-115] -RowIdStrategy::IndexedPredicateOnly => { +// Validate strategy early before any I/O +if query_config.row_id_strategy == RowIdStrategy::IndexedPredicateOnly { return Err(DataFusionError::Execution( "IndexedPredicateOnly strategy requires the indexed executor path \ (execute_indexed_with_context). It cannot be used via execute_query \ because it needs segment metadata and PositionMap for position-based \ row ID computation.".into(), )); } +// ... rest of function (schema inference, table registration, etc.) + Suggestion importance[1-10]: 7 __ Why: Good optimization to fail fast before schema inference and table registration. Moving the validation to the beginning of the function (after basic parameter setup) would avoid unnecessary I/O and improve error response time for misconfigured queries.	Medium
	Remove unused parameter in predicate-only mode The call_strategy parameter is unused when collector is None (predicate-only mode). Consider removing it from the signature or documenting why it's retained for future use. This prevents confusion about its purpose in collector-free execution paths. sandbox/plugins/analytics-backend-datafusion/rust/src/indexed_table/eval/single_collector.rs [146-165] pub fn predicate_only( page_pruner: Arc<PagePruner>, pruning_predicate: Option<Arc<PruningPredicate>>, residual_expr: Option<Arc<dyn datafusion::physical_expr::PhysicalExpr>>, page_prune_metrics: Option<PagePruneMetrics>, - call_strategy: CollectorCallStrategy, ) -> Self { Self { collector: None, page_pruner, pruning_predicate, residual_expr, page_prune_metrics, ffm_collector_calls: None, - call_strategy, + call_strategy: CollectorCallStrategy::FullRange, // Default unused value } } Suggestion importance[1-10]: 4 __ Why: The call_strategy parameter is indeed unused when collector is None, but removing it would break the calling code at line 530 in indexed_executor.rs which passes this parameter. The suggestion is valid but requires coordinated changes across multiple files.	Low
	Clarify full-scan behavior in predicate-only mode When FilterClass::None and emit_row_ids is true but extraction is None, the code proceeds without a pushdown predicate. Verify that full-scan behavior is intentional when no filter exists, as this could lead to unexpected performance characteristics in row ID emission mode. sandbox/plugins/analytics-backend-datafusion/rust/src/indexed_executor.rs [489-496] FilterClass::None if emit_row_ids => { // Predicate-only mode: no collectors, but there may be predicates. // Convert the entire BoolNode tree to a PhysicalExpr for pushdown. - // If no predicates exist, this is None and we get a full scan. + // If no predicates exist (extraction is None), this returns None → full scan. extraction.as_ref().and_then(|e| { residual_bool_to_physical_expr(&e.tree) }) } Suggestion importance[1-10]: 3 __ Why: The suggestion only adds a comment clarifying existing behavior without changing code. While helpful for documentation, the existing comment already explains this ("If no predicates exist, this is None and we get a full scan"), making the improvement marginal.	Low
Possible issue	Handle position map lookup failures explicitly The unwrap_or(delivered_idx) fallback when rg_position fails may produce incorrect row IDs if the position map is corrupted or incomplete. Consider logging a warning or returning an error instead of silently falling back to potentially wrong positions. sandbox/plugins/analytics-backend-datafusion/rust/src/indexed_table/stream.rs [598-608] let surviving_positions: Vec<u64> = match &eval_mask { Some(mask) => (0..batch_len) .filter(|&i| mask.is_valid(i) && mask.value(i)) .map(|i| { let delivered_idx = batch_start_delivered + i; let rg_pos = match pm { - Some(p) => p.rg_position(delivered_idx).unwrap_or(delivered_idx), + Some(p) => p.rg_position(delivered_idx).ok_or_else(|| { + DataFusionError::Internal(format!("position map lookup failed for idx {}", delivered_idx)) + })?, None => delivered_idx, }; - base + rg_pos as u64 + Ok(base + rg_pos as u64) }) - .collect(), + .collect::<Result<Vec<_>>>()?, ... Suggestion importance[1-10]: 6 __ Why: Valid concern about silent fallback behavior that could produce incorrect row IDs. However, the suggested fix changes the return type and requires propagating Result through the entire function, which is a significant refactoring. The current unwrap_or may be intentional for robustness.	Low

Suggestions up to commit ee15643

Category	Suggestion	Impact
Possible issue	Validate pointer before unsafe dereference The raw pointer is reconstructed into a Box without verifying its validity. If execute_query returns an invalid pointer or if the pointer type doesn't match the expected layout, this will cause undefined behavior. Add validation or use a safer abstraction to transfer ownership across the FFI boundary. sandbox/plugins/analytics-backend-datafusion/rust/benches/row_id_bench.rs [189-193] +if ptr == 0 { + panic!("execute_query returned null pointer"); +} let mut stream = unsafe { Box::from_raw(ptr as *mut datafusion::physical_plan::stream::RecordBatchStreamAdapter< opensearch_datafusion::cross_rt_stream::CrossRtStream, >) }; Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a potential safety issue with raw pointer dereferencing. Adding a null check before Box::from_raw is a good defensive practice, though the benchmark context may make this less critical than in production code.	Medium
	Prevent integer overflow in bitmap range *The cast (r_min as i64 - rg.first_row) as u32 can overflow if r_min < rg.first_row, producing incorrect bitmap ranges. Validate that r_min >= rg.first_row before casting, or use checked arithmetic to prevent silent overflow. sandbox/plugins/analytics-backend-datafusion/rust/src/indexed_table/eval/single_collector.rs [247-252] -let mut candidates = if let Some(ref collector) = self.collector { - ... -} else { - // No collector — candidates are page-pruned universe - match &page_ranges { - Some(r) if r.is_empty() => return Ok(None), - Some(r) => { - let mut bm = RoaringBitmap::new(); - for (r_min, r_max) in r { - let lo = (*r_min as i64 - rg.first_row) as u32; - let hi = (*r_max as i64 - rg.first_row) as u32; - bm.insert_range(lo..hi); - } - bm - } - None => { - let mut bm = RoaringBitmap::new(); - bm.insert_range(0..rg.num_rows as u32); - bm - } - } -}; +for (r_min, r_max) in r { + let lo = (*r_min as i64).checked_sub(rg.first_row) + .and_then(|v| u32::try_from(v).ok()) + .expect("r_min < rg.first_row or overflow"); + let hi = (*r_max as i64).checked_sub(rg.first_row) + .and_then(|v| u32::try_from(v).ok()) + .expect("r_max < rg.first_row or overflow"); + bm.insert_range(lo..hi); +} Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a potential integer overflow issue when computing bitmap ranges. Using checked arithmetic is a good defensive practice, though the expect panic may need to be replaced with proper error propagation in production code.	Medium
	Check pointer alignment before dereferencing The function creates slices from raw pointers without verifying alignment or memory validity. If the Java side passes misaligned or invalid pointers, this will cause undefined behavior. Consider adding alignment checks or documenting strict preconditions for callers. sandbox/plugins/analytics-backend-datafusion/rust/src/api.rs [175-197] pub unsafe fn decode_file_metadata(ptr: i64, count: usize) -> Option<Vec<FileRowMetadata>> { if ptr == 0 || count == 0 { return None; } - let wire_slice = std::slice::from_raw_parts(ptr as *const WireFileMetadata, count); - let mut result = Vec::with_capacity(count); - for wire in wire_slice { - let num_rgs = wire.num_row_groups as usize; - let rg_counts = if wire.row_group_row_counts_ptr == 0 || num_rgs == 0 { - Vec::new() - } else { - let counts_ptr = wire.row_group_row_counts_ptr as *const i64; - std::slice::from_raw_parts(counts_ptr, num_rgs) - .iter() - .map(|&c| c as u64) - .collect() - }; - ... + let wire_ptr = ptr as *const WireFileMetadata; + if wire_ptr.align_offset(std::mem::align_of::<WireFileMetadata>()) != 0 { + panic!("decode_file_metadata: misaligned pointer"); } + let wire_slice = std::slice::from_raw_parts(wire_ptr, count); ... } Suggestion importance[1-10]: 6 __ Why: The alignment check is a valid safety improvement for FFI boundaries. However, the suggestion uses panic! which may not be the best error handling strategy. A Result return type would be more appropriate for production code.	Low
General	Handle missing position map entries explicitly The unwrap_or fallback silently uses delivered_idx when rg_position returns None, which may produce incorrect row IDs if the position map is incomplete. This could lead to duplicate or wrong row IDs. Consider logging a warning or returning an error instead of silently falling back. sandbox/plugins/analytics-backend-datafusion/rust/src/indexed_table/stream.rs [598-608] -let surviving_positions: Vec<u64> = match &eval_mask { - Some(mask) => (0..batch_len) - .filter(|&i| mask.is_valid(i) && mask.value(i)) - .map(|i| { - let delivered_idx = batch_start_delivered + i; - let rg_pos = match pm { - Some(p) => p.rg_position(delivered_idx).unwrap_or(delivered_idx), - None => delivered_idx, - }; - base + rg_pos as u64 - }) - .collect(), - ... -} +.map(|i| { + let delivered_idx = batch_start_delivered + i; + let rg_pos = match pm { + Some(p) => p.rg_position(delivered_idx) + .expect("position map missing entry for delivered_idx"), + None => delivered_idx, + }; + base + rg_pos as u64 +}) Suggestion importance[1-10]: 5 __ Why: The suggestion raises a valid concern about silent fallback behavior. However, changing unwrap_or to expect may be too aggressive if the fallback is intentional. The suggestion would benefit from understanding whether None is a valid state or an error condition.	Low

Suggestions up to commit 11601fc

Category	Suggestion	Impact
Possible issue	Remove incomplete assertion statement The test contains an incomplete assertion statement with no arguments. This will cause a compilation error. Remove the dangling assertion or complete it with the intended comparison. sandbox/plugins/analytics-backend-datafusion/rust/src/row_id_benchmark.rs [98-100] -assert_eq!( - ); } Suggestion importance[1-10]: 10 __ Why: This is a critical compilation error. The incomplete assert_eq!() statement at lines 98-99 will prevent the code from compiling and must be fixed immediately.	High
	Validate negative row group count The function performs unchecked pointer arithmetic and casts without validating alignment or bounds. If num_row_groups is negative or excessively large, the cast to usize could cause memory corruption. Add bounds checking before the cast. sandbox/plugins/analytics-backend-datafusion/rust/src/api.rs [174-190] pub unsafe fn decode_file_metadata(ptr: i64, count: usize) -> Option<Vec<FileRowMetadata>> { if ptr == 0 || count == 0 { return None; } let wire_slice = std::slice::from_raw_parts(ptr as *const WireFileMetadata, count); let mut result = Vec::with_capacity(count); for wire in wire_slice { + if wire.num_row_groups < 0 { + return None; + } let num_rgs = wire.num_row_groups as usize; let rg_counts = if wire.row_group_row_counts_ptr == 0 || num_rgs == 0 { Vec::new() } else { let counts_ptr = wire.row_group_row_counts_ptr as *const i64; std::slice::from_raw_parts(counts_ptr, num_rgs) .iter() .map(|&c| c as u64) .collect() }; Suggestion importance[1-10]: 8 __ Why: The suggestion identifies a real safety issue where negative num_row_groups values could cause undefined behavior when cast to usize. Adding validation prevents potential memory corruption in FFM boundary code.	Medium
	Add null pointer validation The raw pointer cast is unsafe and error-prone. If the pointer type doesn't match exactly, this will cause undefined behavior. Consider adding runtime type validation or using a safer abstraction to verify the pointer's validity before dereferencing. sandbox/plugins/analytics-backend-datafusion/rust/benches/row_id_bench.rs [196-200] -let mut stream = unsafe { - Box::from_raw(ptr as *mut datafusion::physical_plan::stream::RecordBatchStreamAdapter< - opensearch_datafusion::cross_rt_stream::CrossRtStream, - >) -}; +let stream_ptr = ptr as *mut datafusion::physical_plan::stream::RecordBatchStreamAdapter< + opensearch_datafusion::cross_rt_stream::CrossRtStream, +>; +if stream_ptr.is_null() { + panic!("Null pointer returned from execute_query"); +} +let mut stream = unsafe { Box::from_raw(stream_ptr) }; Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies an unsafe pointer cast without validation. Adding null pointer checks before dereferencing improves safety, though the impact is moderate since this is benchmark code where panics are acceptable.	Medium
General	Handle position mapping failures explicitly The unwrap_or fallback silently uses delivered_idx when position mapping fails, which could produce incorrect row IDs. Consider propagating the error or logging a warning when position mapping unexpectedly returns None. sandbox/plugins/analytics-backend-datafusion/rust/src/indexed_table/stream.rs [600-611] let surviving_positions: Vec<u64> = match &eval_mask { Some(mask) => (0..batch_len) .filter(|&i| mask.is_valid(i) && mask.value(i)) .map(|i| { let delivered_idx = batch_start_delivered + i; let rg_pos = match pm { - Some(p) => p.rg_position(delivered_idx).unwrap_or(delivered_idx), - None => delivered_idx, - }; - base + rg_pos as u64 + Some(p) => p.rg_position(delivered_idx).ok_or_else(|| { + DataFusionError::Internal(format!("Position mapping failed for index {}", delivered_idx)) + })?, + None => Ok(delivered_idx), + }?; + Ok(base + rg_pos as u64) }) - .collect(), + .collect::<Result<Vec<_>>>()?; Suggestion importance[1-10]: 6 __ Why: The suggestion raises a valid concern about silent fallback behavior, but the unwrap_or pattern is intentional for handling edge cases. Converting to error propagation would change the API signature and may not be necessary if the fallback is correct.	Low

[github-actions]

✅ Gradle check result for 26dc464: SUCCESS

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 73.50%. Comparing base (878afa4) to head (1ce1d72).
⚠️ Report is 7 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #21563      +/-   ##
============================================
+ Coverage     73.42%   73.50%   +0.08%     
- Complexity    74538    74583      +45     
============================================
  Files          5978     5978              
  Lines        338734   338734              
  Branches      48842    48842              
============================================
+ Hits         248709   248994     +285     
+ Misses        70180    69866     -314     
- Partials      19845    19874      +29     

☔ View full report in Codecov by Sentry.
📢 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 1ce1d72

[github-actions]

✅ Gradle check result for 1ce1d72: SUCCESS

[github-actions]

PR Code Analyzer ❗

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

Path	Line	Severity	Description
sandbox/plugins/analytics-backend-datafusion/rust/src/api.rs	571	medium	SQL injection risk: column names passed via FFI are quoted but not sanitized. A column name containing a double-quote character (e.g. 'col"--') could escape the quoting and inject arbitrary SQL into the DataFusion query constructed in fetch_by_row_ids. Column names originate from Java callers and ultimately from user-supplied query fields.
server/src/main/java/org/opensearch/index/engine/exec/coord/CatalogSnapshotManager.java	113	medium	Anomalous constructor side-effect unrelated to the QTF feature: adds a loop in CatalogSnapshotManager's constructor that fires afterRefresh(true, latestCatalogSnapshot) on all registered lifecycle listeners before the IndexFileDeleter is built. This is in the server/ module with no connection to the analytics changes, could trigger unexpected index file deletion callbacks during shard initialization, and latestCatalogSnapshot may be null depending on the committed-snapshots list, causing NPE in listeners.
sandbox/plugins/analytics-backend-datafusion/rust/src/api.rs	197	low	decode_file_metadata accepts raw i64 pointers (row_group_row_counts_ptr) from the Java FFM layer with only a zero-value check. There is no bounds validation on num_row_groups, so a malformed or adversarially crafted WireFileMetadata could cause out-of-bounds reads when slice::from_raw_parts is called.
sandbox/plugins/analytics-engine/src/main/java/org/opensearch/analytics/exec/ReaderContextStore.java	85	low	ReaderContextStore keyed by queryId string with no authentication or ownership check. acquireContext(queryId) returns any stored reader to any caller who knows or guesses the queryId string. If queryIds are predictable (e.g. sequential integers or user-supplied), one query could access another query's open index reader.

The table above displays the top 10 most important findings.

Total: 4 | Critical: 0 | High: 0 | Medium: 2 | Low: 2

---

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.

[github-actions]

Persistent review updated to latest commit f77ae58

[github-actions]

❌ Gradle check result for f77ae58: 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 f79aa97

[github-actions]

❌ Gradle check result for f79aa97: 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 5b659c5

[github-actions]

❌ Gradle check result for 5b659c5: 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 afd06ef

[github-actions]

❌ Gradle check result for afd06ef: 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 5628ec1

[github-actions]

❌ Gradle check result for 5628ec1: 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 11601fc

[github-actions]

❌ Gradle check result for 11601fc: 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 ee15643

[github-actions]

Persistent review updated to latest commit 3ede2af

[github-actions]

❌ Gradle check result for 3ede2af: 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?

[alchemist51]

Will remove it once the #21553 get's merged

[github-actions]

Persistent review updated to latest commit c60165b

[github-actions]

❌ Gradle check result for c60165b: 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 c274c9b

[github-actions]

❌ Gradle check result for c274c9b: 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 449ed67

[github-actions]

❌ Gradle check result for 449ed67: 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?

[alchemist51]

Closing it, have added my changed in #21836