Add clippy check to sandbox CI

[alchemist51]

Description

Follow-up to #22369 (rustfmt). Wires cargo clippy --workspace --all-targets -- -D warnings into the sandbox native (Rust) workspace CI, and clears the workspace to zero clippy warnings so the gate passes.

Workflow (sandbox-check.yml):

• Adds the clippy component to the Rust toolchain setup.
• Adds a "Run Rust clippy" step. It runs with working-directory: sandbox/libs/dataformat-native/rust (not --manifest-path from the repo root) so the crate's .cargo/config.toml — which sets --cfg tokio_unstable, required by the tokio RuntimeMetrics calls in stats.rs / merge/metrics.rs — is honored. cargo only reads .cargo/config.toml relative to the working directory, so running from the repo root fails to compile.

Code changes (the bulk of the diff is mechanical clippy cleanup):

• Fix deny-by-default lints that were aborting the lint build before any warnings could be seen: not_unsafe_ptr_arg_deref (FFI extern "C" entry points), never_loop, absurd_extreme_comparisons, approx_constant, mut_from_ref.
• Migrate deprecated parquet/arrow APIs: with_page_index → with_page_index_policy, set_max_row_group_size → set_max_row_group_row_count.
• Repair stale benchmarks that no longer matched current API signatures (they already failed cargo check --benches on main).
• The remainder is unused-import/mut removal, doc-comment formatting, and idiomatic simplifications.

Intentional #[allow(deprecated)] (please note during review):
Two PartitionedFile::with_extensions calls — in indexed_table/parquet_bridge.rs and shard_table_provider.rs — are deliberately kept on the deprecated API. The with_extension replacement keys the extension by its concrete type, but DataFusion's Parquet opener retrieves the ParquetAccessPlan via the legacy insert_dyn keying; switching silently drops row selection and returns wrong rows (caught by the boolean_algebra e2e suite). Each site has a comment explaining the deferral.

Verification:

• cargo clippy --workspace --all-targets -- -D warnings — passes (0 warnings).
• cargo fmt --all -- --check — clean.
• Test suite passes. (Two pre-existing local_exec_test failures exist on main, unrelated to this change.)

Clippy config/enforcement is modeled on Apache DataFusion's and arrow-rs's lint setup.

Check List

• Functionality includes tests (N/A — CI/lint only; existing tests validate the code changes)
• Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit ac079b2)

Here are some key observations to aid the review process:

🧪 No relevant tests

🔒 No security concerns identified

📝 TODO sections sandbox/plugins/analytics-backend-datafusion/rust/src/cache/page_index/page_index_io.rs [26]: migrate to ParquetMetaDataReader once arrow-rs exposes page-index column/row-group projection (Pass options to Parquet metadata readers apache/arrow-rs#8643).

🔀 Multiple PR themes Sub-PR theme: Migrate deprecated parquet/arrow APIs (with_page_index, set_max_row_group_size) Relevant files: sandbox/plugins/analytics-backend-datafusion/rust/src/indexed_table/tests_e2e/multi_segment.rs sandbox/plugins/analytics-backend-datafusion/rust/src/indexed_table/page_pruner.rs sandbox/plugins/analytics-backend-datafusion/rust/src/cache/page_index/page_index_io.rs sandbox/plugins/analytics-backend-datafusion/rust/src/indexed_table/tests_e2e/page_pruning.rs sandbox/plugins/analytics-backend-datafusion/rust/src/indexed_table/tests_e2e/row_id_emission.rs Sub-PR theme: Clippy cleanup in parquet-data-format and block-cache-foyer tests Relevant files: sandbox/plugins/parquet-data-format/src/main/rust/src/tests/mod.rs sandbox/plugins/parquet-data-format/src/main/rust/tests/sort_types_tests.rs sandbox/plugins/block-cache-foyer/src/main/rust/src/tests.rs Sub-PR theme: Clippy cleanup in analytics-backend-datafusion core modules Relevant files: sandbox/plugins/analytics-backend-datafusion/rust/src/memory.rs sandbox/plugins/analytics-backend-datafusion/rust/src/query_tracker.rs sandbox/plugins/analytics-backend-datafusion/rust/src/api.rs

⚡ Recommended focus areas for review Broken assertion macro The rewrite from assert_eq! to assert! left several call sites where the boolean expression is followed by a bare string literal used as the message. In Rust, assert!(expr, "msg") expects a format string as the second positional arg — that still compiles — but in some hunks the assert!( line is not shown alongside its arguments, and where visible (e.g., around lines 2498-2501, 2510-2512, 2519-2522, 2598-2603, 2673-2676) the assert! takes cache.should_skip_sweep(), "..." which is valid. However, the for (i, &val) in out.iter().enumerate() block near line 1299-1301 removed the enclosing for i in 0..20 { ... } but retained only a single-statement body with a trailing } at 1301 — verify the loop still contains exactly one assert_eq! and the outer scope's closing brace matches. A mismatched brace here would fail to compile or silently change scoping. // A freshly created cache has no hits, misses, evictions, used bytes, or active reads. // Sections 0 and 1 are identical (Foyer is currently single-tier). for (i, &val) in out.iter().enumerate() { assert_eq!(val, 0, "out[{i}] should be 0 for a fresh cache, got {val}"); } Reservation mutability removed Multiple tests changed let mut reservation = ... to let reservation = ... while still calling reservation.try_grow(...). This only compiles if try_grow takes &self (or the underlying type provides interior mutability). If DataFusion's MemoryReservation::try_grow takes &mut self (which is the upstream signature), these tests will fail to compile. Please confirm the reservation type in use here truly permits shared-reference grow; otherwise the clippy cleanup has introduced compile errors across many test files (memory.rs, query_tracker.rs). fn test_try_grow_within_limit() { let (pool, _handle) = new_pool(1024); let consumer = MemoryConsumer::new("test"); let reservation = consumer.register(&pool); assert!(reservation.try_grow(512).is_ok()); assert_eq!(pool.reserved(), 512); } Behavior change in clamp value.max(1).min(32) as usize was replaced with value.clamp(1, 32) as usize. For negative i64 values, the old code first clamped via max(1) (yielding 1) before casting; the new clamp(1, 32) also yields 1 for negatives, so semantics match. However, clamp panics if min > max, which isn't an issue here with constants. No functional regression, but worth noting the cast now happens after clamp — behavior is equivalent. pub fn set_reduce_target_partitions(value: i64) { REDUCE_TARGET_PARTITIONS.store( value.clamp(1, 32) as usize, std::sync::atomic::Ordering::Release, ); Semantic change in predicate !(i32_i * 10 < 30) was rewritten as (i32_i * 10 >= 30). These are logically equivalent for finite integers, but for the test oracle this is fine. Similarly !((i as i32) * 10 < 40) → (i as i32) * 10 >= 40. No issue, just verify the comment above ("NOT is conservative in stats") still applies since the source predicate under test presumably still uses NOT — the oracle transformation is only valid because it mirrors the same set of rows. ]), ]); // NOT is conservative in stats (always true), so actual predicate filters. let expected = wide_oracle(&specs, |i| (i as i32) * 10 >= 40); for np in [1usize, 3, 5] {

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to ac079b2

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Remove misleading no-op expect on Some Some(x).expect(...) is a no-op that always succeeds and doesn't validate anything — the prior code presumably intended to guard against a truncated/empty tag but this rewrite silently drops that check. Either remove the misleading expect entirely or reinstate a real validation of annotation_id bounds before casting to u8. sandbox/plugins/analytics-backend-datafusion/rust/src/indexed_table/tests_e2e/streaming_at_scale.rs [296-297] -let tag = Some(*annotation_id as u8).expect("empty tag bytes"); +let tag = *annotation_id as u8; out.push(large_collector_for(tag)); Suggestion importance[1-10]: 6 __ Why: Correctly identifies that Some(x).expect(...) is a no-op check; removing it improves clarity in test harness code, though impact is limited since this is test-only.	Low
	Remove or wire up unused partitions value _num_partitions is computed but discarded, which suggests the partitioning value that used to flow into execution is now being dropped on the floor. If downstream code no longer needs it, remove the line entirely; otherwise wire it through so the target-partition setting from the query config is actually honored. sandbox/plugins/analytics-backend-datafusion/rust/src/indexed_executor.rs [917] let query_config = Arc::new(handle.query_config); -let _num_partitions = query_config.target_partitions.max(1); let aggregate_mode = handle.aggregate_mode; Suggestion importance[1-10]: 5 __ Why: Valid observation that the underscore-prefixed variable indicates dead code; either wiring it through or removing it would clean up the code, though the leading underscore already signals intentional non-use.	Low
	Guard reservation size against overflow mapping_bytes * 2 can overflow usize for very large total (e.g. > usize::MAX/16 on 64-bit targets is unlikely, but the multiplication itself is unchecked and would silently wrap under-budget). Use checked_mul and return a clean error, mirroring the checked arithmetic style used elsewhere in the writer. sandbox/plugins/parquet-data-format/src/main/rust/src/writer.rs [825-830] -reservation.request(mapping_bytes * 2)?; +let reserve_bytes = mapping_bytes + .checked_mul(2) + .ok_or("mapping reservation overflow")?; +reservation.request(reserve_bytes)?; let mut flat_mapping = vec![0i64; total]; for (i, slot) in flat_mapping.iter_mut().enumerate() { *slot = i as i64; } Suggestion importance[1-10]: 4 __ Why: Overflow guard is defensive; the existing code was not changed in this area regarding the multiplication, and this concern is largely theoretical for realistic mapping sizes.	Low
	Prevent overflow in disk space multiplication The multiplication can still overflow on 32-bit platforms where both f_bavail and f_frsize are u32, since the as u64 cast is applied to each operand but Rust evaluates the multiplication in the smaller type only if both are smaller — however, if one side is already u64 the cast is fine. To be safe and explicit against overflow on any platform, use u64::from on each operand or use checked_mul / saturating_mul. sandbox/plugins/analytics-backend-datafusion/rust/src/memory_guard.rs [366-367] #[allow(clippy::unnecessary_cast)] -let avail = stat.f_bavail as u64 * stat.f_frsize as u64; +let avail = (stat.f_bavail as u64).saturating_mul(stat.f_frsize as u64); Some(avail) Suggestion importance[1-10]: 4 __ Why: Using saturating_mul is a minor defensive improvement against theoretical overflow on 32-bit platforms, but the practical risk is low since f_bavail * f_frsize represents available disk bytes which rarely approaches u64::MAX.	Low
	Verify path-building semantics after simplification The simplification changes behavior for the case where the previous segment ended with ARRAY_MARKER — the original code appended . in that case (matching the "else if s.ends_with(ARRAY_MARKER)" branch), and the new code also does so via !s.is_empty(). However, if ARRAY_MARKER itself contains or ends with ., or if the intent was different (e.g. no dot after array marker), the semantics differ. Please confirm ARRAY_MARKER semantics match "always insert . before next field name when non-empty". sandbox/plugins/analytics-backend-datafusion/rust/src/udf/json_extract_all.rs [299-307] if segment == ARRAY_MARKER { s.push_str(ARRAY_MARKER); } else { - // Separate field names with `.`; the leading segment gets none. - // (Both a plain field boundary and an array-marker boundary need a - // separator, which together is exactly "s is non-empty".) if !s.is_empty() { s.push('.'); } s.push_str(segment); Suggestion importance[1-10]: 4 __ Why: The suggestion correctly identifies a potential semantic change: the original code did NOT add . when s.ends_with(ARRAY_MARKER) in one branch, while the new code always adds . when non-empty. This warrants verification but the improved_code is essentially identical to existing_code.	Low
	Avoid missing Fetch under unhandled rel types The is_some_and closure receives &Box and calling rel_has_fetch(r) on it relies on auto-deref; more importantly, these arms recurse into a single child but silently return false for other rel variants (Join, Set, Read with nested inputs, etc.) that may contain a Fetch. Consider explicitly handling remaining rel types or falling back to a recursive walk rather than returning false for unknown types, since the current default hides Fetch nodes buried in other operators. sandbox/plugins/analytics-backend-datafusion/rust/src/session_context.rs [554-557] -Some(RelType::Sort(s)) => s.input.as_ref().is_some_and(|r| rel_has_fetch(r)), -Some(RelType::Project(p)) => p.input.as_ref().is_some_and(|r| rel_has_fetch(r)), -Some(RelType::Filter(f)) => f.input.as_ref().is_some_and(|r| rel_has_fetch(r)), -Some(RelType::Aggregate(a)) => a.input.as_ref().is_some_and(|r| rel_has_fetch(r)), +Some(RelType::Sort(s)) => s.input.as_deref().is_some_and(rel_has_fetch), +Some(RelType::Project(p)) => p.input.as_deref().is_some_and(rel_has_fetch), +Some(RelType::Filter(f)) => f.input.as_deref().is_some_and(rel_has_fetch), +Some(RelType::Aggregate(a)) => a.input.as_deref().is_some_and(rel_has_fetch), Suggestion importance[1-10]: 3 __ Why: The as_deref change is a minor stylistic improvement; the broader concern about unhandled rel types is already acknowledged by an existing TODO in the code, so the suggestion adds limited value.	Low
	Keep explicit zero-guard for clarity checked_div on unsigned integers only returns None when the divisor is zero, which matches the original guard. However, if actual_batches or actual_rows are of a signed type or become negative in future refactors, the behavior differs. Consider keeping this explicit for clarity and add a debug assertion, or ensure both are usize. sandbox/plugins/analytics-backend-datafusion/rust/tests/budget_accuracy_test.rs [246-247] -let avg_batch_bytes = actual_total_bytes.checked_div(actual_batches).unwrap_or(0); -let actual_avg_row_bytes = actual_total_bytes.checked_div(actual_rows).unwrap_or(0); +let avg_batch_bytes = if actual_batches > 0 { actual_total_bytes / actual_batches } else { 0 }; +let actual_avg_row_bytes = if actual_rows > 0 { actual_total_bytes / actual_rows } else { 0 }; Suggestion importance[1-10]: 2 __ Why: The suggestion reverts a clean refactor for hypothetical future concerns. The current checked_div usage is idiomatic and correct for unsigned types.	Low
Possible issue	Preserve conditional counter increment semantics Replacing the previously incrementing token_count with (1usize..).zip(...) binds token_count to the iteration index unconditionally on every match, whereas the original code likely incremented it only under specific conditions (e.g., when producing a token). If token_count was being incremented conditionally (e.g., only on non-token gaps or only on tokens), this refactor changes semantics. Verify the loop body still increments token_count on the same iterations as before, or restore the manual counter. sandbox/plugins/analytics-backend-datafusion/rust/src/patterns/utils.rs [85] -for (token_count, m) in (1usize..).zip(compiled_pattern.find_iter(pattern)) { +let mut token_count = 1usize; +for m in compiled_pattern.find_iter(pattern) { let start = m.start(); let end = m.end(); if start > last_end { Suggestion importance[1-10]: 3 __ Why: The suggestion speculates about potential semantic changes but the diff clearly shows the original code had let mut token_count = 1usize; before the loop with no visible conditional increment; the refactor appears equivalent. The suggestion is largely speculative.	Low

---

Previous suggestions

Suggestions up to commit 7082f75

Category	Suggestion	Impact
Possible issue	Verify counter increment semantics preserved Replacing the manually-incremented token_count (starting at 1 and incremented at the end of the loop) with (1usize..).zip(...) changes semantics if the original code only incremented token_count on a certain branch inside the loop. Verify that token_count was incremented unconditionally per iteration; otherwise, the counter values will differ from the original logic and break downstream token-key generation. sandbox/plugins/analytics-backend-datafusion/rust/src/patterns/utils.rs [85] -for (token_count, m) in (1usize..).zip(compiled_pattern.find_iter(pattern)) { +let mut token_count = 1usize; +for m in compiled_pattern.find_iter(pattern) { + // ... use token_count, then increment only where the original code did +} Suggestion importance[1-10]: 6 __ Why: Valid concern: if the original token_count was incremented conditionally inside the loop, the zip-based rewrite changes semantics. Worth verifying to avoid a subtle bug.	Low
	Verify slice-box allocation invariant *Box::from_raw on a mut [T] produced by slice_from_raw_parts_mut requires the slice to have been allocated as a Box<[T]> with matching length; if the original allocation was a Vec that leaked via Box::into_raw(vec.into_boxed_slice()), this is correct — otherwise this is UB. Verify the allocation path in parquet_merge_files matches; the same concern applies to the gen_keys_ptr, gen_offsets_ptr, gen_sizes_ptr, and parquet_free_row_id_mapping cases below. sandbox/plugins/parquet-data-format/src/main/rust/src/ffm.rs [791-798] if mapping_ptr != 0 && mapping_len > 0 { let mapping_bytes = mapping_len as usize * std::mem::size_of::<i64>(); - // Java released merge mapping — free from pool crate::memory::merge_pool().shrink(mapping_bytes); + // SAFETY: mapping_ptr must have been produced by Box::into_raw of a Box<[i64]> + // of length exactly mapping_len (see parquet_merge_files). let _ = Box::from_raw(std::ptr::slice_from_raw_parts_mut( mapping_ptr as *mut i64, mapping_len as usize, )); } Suggestion importance[1-10]: 4 __ Why: The suggestion only asks to verify existing allocation invariants and add a SAFETY comment; it does not change behavior. The PR only replaced slice::from_raw_parts_mut with the equivalent std::ptr::slice_from_raw_parts_mut.	Low
Possible bug	Guard against silent u8 truncation *The previous code used Some(annotation_id as u8).expect("empty tag bytes") which, though awkward, would never panic; the new code silently truncates annotation_id to a u8. If annotation_id can exceed 255, large_collector_for will receive a wrong tag and produce incorrect results. Add a debug assertion or explicit try_into to catch out-of-range values. sandbox/plugins/analytics-backend-datafusion/rust/src/indexed_table/tests_e2e/streaming_at_scale.rs [296-297] BoolNode::Collector { annotation_id } => { - let tag = *annotation_id as u8; + let tag: u8 = (*annotation_id) + .try_into() + .expect("annotation_id must fit in u8"); out.push(large_collector_for(tag)); } Suggestion importance[1-10]: 5 __ Why: The old code Some(*annotation_id as u8).expect(...) already truncated via as u8, so behavior is unchanged; still, adding a try_into guard in test code could catch invalid tags earlier.	Low
	Confirm dropped partition count is intentional num_partitions was renamed to num_partitions, indicating it's unused. If the value truly should influence execution (e.g. partitioning of the resulting plan), silently dropping it may cause a regression relative to before this PR. Confirm the removal is intentional or wire it back into the executor; if truly dead, remove the binding entirely rather than prefixing with . sandbox/plugins/analytics-backend-datafusion/rust/src/indexed_executor.rs [917] let query_config = Arc::new(handle.query_config); -let num_partitions = query_config.target_partitions.max(1); +// Removed: num_partitions was unused; verify no downstream code relies on it. let aggregate_mode = handle.aggregate_mode; Suggestion importance[1-10]: 5 __ Why: Renaming to _num_partitions suggests it was unused; the suggestion reasonably asks to confirm this isn't a regression, which is worth verifying but is a minor concern.	Low
General	Guard against multiplication overflow On platforms where f_bavail/f_frsize are wider than u64 (some BSDs use fsblkcnt_t that could differ), or where they are signed, the as u64 cast can silently truncate/misinterpret. Prefer u64::from(stat.f_bavail).checked_mul(u64::from(stat.f_frsize)) when the types allow, or use saturating_mul to guard against overflow on very large filesystems. sandbox/plugins/analytics-backend-datafusion/rust/src/memory_guard.rs [366-367] -// clippy::unnecessary_cast — `statvfs` field widths are platform-dependent: `f_bavail` -// is u64 on Linux (cast is a no-op there, which is why clippy flags it) but u32/c_ulong -// on macOS/BSD, where the `as u64` is a real widening cast needed to avoid overflow in -// the multiply. Keep the cast for cross-platform correctness. #[allow(clippy::unnecessary_cast)] -let avail = stat.f_bavail as u64 * stat.f_frsize as u64; +let avail = (stat.f_bavail as u64).saturating_mul(stat.f_frsize as u64); Some(avail) Suggestion importance[1-10]: 4 __ Why: Using saturating_mul avoids theoretical overflow on very large filesystems, a minor robustness improvement, though overflow is unlikely in practice.	Low
	Add regression test for array-marker path The previous logic explicitly avoided pushing . when s ended with ARRAY_MARKER in the original path (!s.is_empty() && !s.ends_with(ARRAY_MARKER)), but the else-branch added . when it did end with ARRAY_MARKER. The simplification to !s.is_empty() now always adds a . — including immediately after ARRAY_MARKER, which was the original intent, but double-check: the original produced . after the marker too, so the two branches were indeed equivalent. Confirm by adding/keeping a test for a path segment following an array marker (e.g., a[].b producing a[].b). sandbox/plugins/analytics-backend-datafusion/rust/src/udf/json_extract_all.rs [302-307] -// Separate field names with `.`; the leading segment gets none. -// (Both a plain field boundary and an array-marker boundary need a -// separator, which together is exactly "s is non-empty".) if !s.is_empty() { s.push('.'); } s.push_str(segment); Suggestion importance[1-10]: 3 __ Why: Suggestion only asks to add/keep a regression test to confirm equivalence; low-to-moderate importance and largely a verification request.	Low
	Use closure form for consistent Box deref is_some_and(rel_has_fetch) passes &Box to a function expecting &Rel; this only compiles due to auto-deref of Box through a closure. Since the earlier arms use |r| rel_has_fetch(r), keep the same closure form here to avoid a type-inference/coercion mismatch across compiler versions. sandbox/plugins/analytics-backend-datafusion/rust/src/session_context.rs [575] plan.relations.iter().any(|pr| match pr.rel_type.as_ref() { Some(substrait::proto::plan_rel::RelType::Root(rr)) => { - rr.input.as_ref().is_some_and(rel_has_fetch) + rr.input.as_ref().is_some_and(|r| rel_has_fetch(r)) } Some(substrait::proto::plan_rel::RelType::Rel(r)) => rel_has_fetch(r), None => false, Suggestion importance[1-10]: 2 __ Why: is_some_and(rel_has_fetch) works fine because Option<&Box<Rel>> auto-derefs when calling the closure; the change is purely stylistic and not a correctness concern.	Low
	Preserve original zero-check semantics checked_div on usize only returns None when the divisor is 0, but the previous code guarded against actual_batches > 0 / actual_rows > 0. If actual_batches or actual_rows are of a signed type, checked_div also returns None on overflow (e.g., MIN / -1); ensure the types match usize semantics, otherwise the fallback masks a real bug. Consider keeping the explicit zero-check for clarity and correctness parity. sandbox/plugins/analytics-backend-datafusion/rust/tests/budget_accuracy_test.rs [246-247] -let avg_batch_bytes = actual_total_bytes.checked_div(actual_batches).unwrap_or(0); -let actual_avg_row_bytes = actual_total_bytes.checked_div(actual_rows).unwrap_or(0); +let avg_batch_bytes = if actual_batches > 0 { actual_total_bytes / actual_batches } else { 0 }; +let actual_avg_row_bytes = if actual_rows > 0 { actual_total_bytes / actual_rows } else { 0 }; Suggestion importance[1-10]: 2 __ Why: checked_div on usize returns None only for zero divisor, so the refactor is semantically equivalent. The suggestion is largely a stylistic revert without added correctness.	Low

Suggestions up to commit 9140427

Category	Suggestion	Impact
Possible issue	Avoid silent truncation of collector tag Casting annotation_id (likely u16/u32) directly to u8 silently truncates the high bits, so any collector tag ≥ 256 will alias to a wrong collector and produce incorrect results without any diagnostic. Use a checked conversion (u8::try_from) and panic/return with a clear message on overflow. sandbox/plugins/analytics-backend-datafusion/rust/src/indexed_table/tests_e2e/streaming_at_scale.rs [296-297] BoolNode::Collector { annotation_id } => { - let tag = *annotation_id as u8; + let tag = u8::try_from(*annotation_id) + .expect("collector annotation_id does not fit in u8"); out.push(large_collector_for(tag)); } Suggestion importance[1-10]: 5 __ Why: In test code, silent truncation of annotation_id to u8 could mask bugs if tags ≥ 256 are used; a checked conversion would surface issues, though impact is limited to tests.	Low
	Verify path formatting after simplification The simplification changes behavior when the previous segment was an ARRAY_MARKER: previously, only the case ending with ARRAY_MARKER inserted a . separately, and other non-empty cases also inserted .. The new logic inserts . in both cases uniformly. If ARRAY_MARKER itself already contains a trailing/leading separator (e.g. "[]"), the produced path may now contain an unintended extra . (e.g. "a[].b" vs previous "a[].b"). Verify the ARRAY_MARKER format and add a test for the array-followed-by-field path to ensure formatting is unchanged. sandbox/plugins/analytics-backend-datafusion/rust/src/udf/json_extract_all.rs [299-307] if segment == ARRAY_MARKER { s.push_str(ARRAY_MARKER); } else { - // Separate field names with `.`; the leading segment gets none. - // (Both a plain field boundary and an array-marker boundary need a - // separator, which together is exactly "s is non-empty".) if !s.is_empty() { s.push('.'); } s.push_str(segment); } Suggestion importance[1-10]: 5 __ Why: The suggestion correctly identifies a potentially subtle behavior change: previously a . was skipped when s ended with ARRAY_MARKER, but the new logic always adds . when s is non-empty. This could genuinely alter path formatting.	Low
	Guard against overflow in memory reservation Multiplying mapping_bytes * 2 can overflow usize on large inputs, silently wrapping and reserving too little memory. Use checked_mul (or saturating_mul) and return an error on overflow to keep the reservation accounting correct. sandbox/plugins/parquet-data-format/src/main/rust/src/writer.rs [828-830] - reservation.request(mapping_bytes * 2)?; + let reserve_bytes = mapping_bytes + .checked_mul(2) + .ok_or("mapping reservation size overflow")?; + reservation.request(reserve_bytes)?; let mut flat_mapping = vec![0i64; total]; for (i, slot) in flat_mapping.iter_mut().enumerate() { *slot = i as i64; } Suggestion importance[1-10]: 4 __ Why: Overflow of mapping_bytes * 2 is only realistic for extremely large total values (billions of i64s), so the practical impact is limited, but using checked_mul is a reasonable defensive improvement.	Low
	Ensure state reset on segment change The refactor changed the flush condition from chunk_seg.is_some() && chunk_seg != Some(rg.segment_idx) combined with a separate if !chunk_rg_indices.is_empty() (which previously always flushed when segment changed even if empty was checked) to a combined AND. Behaviorally, this is equivalent only if chunk_rg_indices.is_empty() implies no flush is needed regardless; if the surrounding code depended on resetting chunk_seg/other state even when indices are empty, that reset is now skipped. Verify no state (chunk_doc_min/max/seg) needs to be reset when segment changes with an empty index list. sandbox/plugins/analytics-backend-datafusion/rust/src/indexed_table/partitioning.rs [94-100] if let Some(seg) = chunk_seg { - if seg != rg.segment_idx && !chunk_rg_indices.is_empty() { - current_chunks.push(SegmentChunk { - segment_idx: seg, - doc_min: chunk_doc_min, - doc_max: chunk_doc_max, - row_group_indices: chunk_rg_indices.clone(), + if seg != rg.segment_idx { + if !chunk_rg_indices.is_empty() { + current_chunks.push(SegmentChunk { + segment_idx: seg, + doc_min: chunk_doc_min, + doc_max: chunk_doc_max, + row_group_indices: chunk_rg_indices.clone(), Suggestion importance[1-10]: 4 __ Why: Valid concern about whether state needs resetting when segment changes with empty indices, though the original code also gated the reset via the same conditions. The improved_code snippet is incomplete/truncated.	Low
	Reject negative lengths before pointer free mapping_len and gen_count are signed (i64); casting a negative value to usize produces an enormous length and Box::from_raw will then try to drop a slice of that size, causing undefined behavior. Guard against negative lengths before the cast, matching the > 0 check already used elsewhere. sandbox/plugins/parquet-data-format/src/main/rust/src/ffm.rs [794-817] - let _ = Box::from_raw(std::ptr::slice_from_raw_parts_mut( - mapping_ptr as *mut i64, - mapping_len as usize, - )); + if mapping_len > 0 { + let _ = Box::from_raw(std::ptr::slice_from_raw_parts_mut( + mapping_ptr as *mut i64, + mapping_len as usize, + )); + } +} +if gen_count <= 0 { + return; } let n = gen_count as usize; -if gen_keys_ptr != 0 && n > 0 { +if gen_keys_ptr != 0 { let _ = Box::from_raw(std::ptr::slice_from_raw_parts_mut( gen_keys_ptr as *mut i64, n, )); } Suggestion importance[1-10]: 3 __ Why: The existing code already checks mapping_len > 0 and n > 0 (where n = gen_count as usize); however the cast happens before the check for gen_count, so negative values would wrap. The suggestion has some merit but partly duplicates existing guards.	Low
	Preserve original counter increment semantics Zipping (1usize..) with the iterator changes the semantics of token_count: previously it was incremented only inside the loop body (conditionally), but now it advances on every iteration regardless of whether a token is actually pushed. If the original loop had any early continue or conditional increment, this refactor silently changes counts. Verify the original increment was unconditional before adopting this form; otherwise revert to an explicit counter. sandbox/plugins/analytics-backend-datafusion/rust/src/patterns/utils.rs [85] -for (token_count, m) in (1usize..).zip(compiled_pattern.find_iter(pattern)) { +let mut token_count = 1usize; +for m in compiled_pattern.find_iter(pattern) { Suggestion importance[1-10]: 3 __ Why: Looking at the diff, the original code had token_count incremented unconditionally per iteration (there's no visible early continue), so the refactor is likely semantically equivalent. The suggestion is a valid caution but low impact.	Low
General	Use as_deref for Box option traversal is_some_and takes ownership of the closure argument by value, but here r is &Box; the call rel_has_fetch(r) will auto-deref, however clippy/is_some_and semantics require FnOnce(T) -> bool. If Option::as_ref() yields Option<&Box>, prefer map(|r| rel_has_fetch(r)).unwrap_or(false) or dereference explicitly to avoid future breakage when signatures change. sandbox/plugins/analytics-backend-datafusion/rust/src/session_context.rs [554-557] fn rel_has_fetch(rel: &substrait::proto::Rel) -> bool { match rel.rel_type.as_ref() { Some(RelType::Fetch(f)) => f.count_mode.is_some(), - Some(RelType::Sort(s)) => s.input.as_ref().is_some_and(|r| rel_has_fetch(r)), - Some(RelType::Project(p)) => p.input.as_ref().is_some_and(|r| rel_has_fetch(r)), - Some(RelType::Filter(f)) => f.input.as_ref().is_some_and(|r| rel_has_fetch(r)), - Some(RelType::Aggregate(a)) => a.input.as_ref().is_some_and(|r| rel_has_fetch(r)), + Some(RelType::Sort(s)) => s.input.as_deref().is_some_and(rel_has_fetch), + Some(RelType::Project(p)) => p.input.as_deref().is_some_and(rel_has_fetch), + Some(RelType::Filter(f)) => f.input.as_deref().is_some_and(rel_has_fetch), + Some(RelType::Aggregate(a)) => a.input.as_deref().is_some_and(rel_has_fetch), Suggestion importance[1-10]: 2 __ Why: The existing is_some_and(|r| rel_has_fetch(r)) compiles and works correctly with auto-deref; the suggestion is a minor stylistic refactor, not a correctness fix.	Low
	Strengthen atomic orderings for cache visibility The collapsed if changes behavior: previously, when the interval had elapsed but the CAS lost the race, the function still fell through to load the cached value. With the new combined condition this is still correct, but note that if the CAS fails, another thread just updated CACHED_RESIDENT, so reading it is fine — however, ensure the load is not reordered before the concurrent store. Consider using Ordering::Acquire on the load (paired with Release on the store) to guarantee readers observe the updater's write. sandbox/plugins/analytics-backend-datafusion/rust/src/memory_guard.rs [60-69] if now_ms.wrapping_sub(last) >= RESIDENT_CACHE_INTERVAL_MS && LAST_CHECK_MS - .compare_exchange(last, now_ms, Ordering::Relaxed, Ordering::Relaxed) + .compare_exchange(last, now_ms, Ordering::AcqRel, Ordering::Relaxed) .is_ok() { let r = native_bridge_common::allocator::resident_bytes(); - CACHED_RESIDENT.store(r, Ordering::Relaxed); + CACHED_RESIDENT.store(r, Ordering::Release); return r; } -CACHED_RESIDENT.load(Ordering::Relaxed) +CACHED_RESIDENT.load(Ordering::Acquire) Suggestion importance[1-10]: 2 __ Why: This suggestion addresses ordering semantics unrelated to the PR change (the PR only collapsed nested ifs). The original code already used Relaxed, so this is a pre-existing design choice, not introduced by the PR.	Low

Suggestions up to commit 8944983

Category	Suggestion	Impact
General	Narrow overly broad lint suppressions Blanket-allowing unreachable_code, never_loop, unused_variables, unused_mut, and unused_labels on the sweep and persist task bodies masks real bugs (dead branches, forgotten mutability, unused restart state) rather than the single intended lint. Narrow the allow list to only clippy::never_loop (the actually-intended suppression) and fix or remove the truly-unreachable restart/backoff scaffolding so future regressions are caught. sandbox/plugins/block-cache-foyer/src/main/rust/src/foyer/foyer_cache.rs [431-437] -#[allow( - clippy::never_loop, - unused_variables, - unused_mut, - unreachable_code, - unused_labels -)] +#[allow(clippy::never_loop)] instance._runtime.spawn(async move { native_bridge_common::log_info!( "[block-cache] sweep task started: interval={}s", sweep_interval_secs ); Suggestion importance[1-10]: 6 __ Why: Valid concern — the broad allow list masks legitimate warnings beyond the intended never_loop suppression, which could hide real bugs in restart/backoff logic. However, the comment explicitly justifies the broad allow due to unreachable-code cascade effects.	Low
	Document or wire up dropped partition count Renaming num_partitions to _num_partitions silences the unused-variable warning but hides the fact that the computed target-partition value is being dropped on the floor in execute_indexed_with_context_inner. Either remove the computation entirely or wire it into the downstream execution config so partitioning behavior matches the non-indexed path. sandbox/plugins/analytics-backend-datafusion/rust/src/indexed_executor.rs [915] let query_config = Arc::new(handle.query_config); -let num_partitions = query_config.target_partitions.max(1); +// TODO: propagate to downstream execution config; currently unused. +let _num_partitions = query_config.target_partitions.max(1); let aggregate_mode = handle.aggregate_mode; Suggestion importance[1-10]: 4 __ Why: Reasonable observation that renaming to _num_partitions silently drops a computation that may be relevant, but the suggested improvement only adds a TODO comment without functional change.	Low
	Use Acquire/Release for cached value visibility Behavior is preserved, but note that under contention, if the interval elapsed but the CAS lost the race, the losing thread now falls through to CACHED_RESIDENT.load — same as before. However, if the CAS fails because another thread just updated it, the returned cached value may briefly be the old one until that thread's store completes. Consider loading CACHED_RESIDENT with Acquire (paired with a Release store) to ensure the fresh value is visible after the CAS winner stores it. sandbox/plugins/analytics-backend-datafusion/rust/src/memory_guard.rs [60-68] if now_ms.wrapping_sub(last) >= RESIDENT_CACHE_INTERVAL_MS && LAST_CHECK_MS .compare_exchange(last, now_ms, Ordering::Relaxed, Ordering::Relaxed) .is_ok() { let r = native_bridge_common::allocator::resident_bytes(); - CACHED_RESIDENT.store(r, Ordering::Relaxed); + CACHED_RESIDENT.store(r, Ordering::Release); return r; } +CACHED_RESIDENT.load(Ordering::Acquire) Suggestion importance[1-10]: 3 __ Why: Minor memory-ordering nit; the original code used Relaxed too, so this is not a regression introduced by the PR and impact is marginal for a resident-bytes cache.	Low
	Preserve original predicate form in oracle The clippy-driven rewrite of !(i32_i * 10 < 30) into (i32_i * 10 >= 30) changes semantics when the multiplication overflows: the original < and its logical negation >= behave identically only for defined arithmetic, but under Rust's debug-mode overflow checks the two expressions can panic vs. wrap differently, and more importantly this diverges from the production predicate being tested (which still uses <). Preserve the exact form the oracle is meant to mirror by keeping the negation, or explicitly document the equivalence. sandbox/plugins/analytics-backend-datafusion/rust/src/indexed_table/tests_e2e/multi_segment.rs [1280] let expected = wide_oracle(&specs, |i| { let i32_i = i as i32; - (i32_i * 10 >= 30) && i32_i % 7 > 2 && i % 3 != 2 + #[allow(clippy::nonminimal_bool)] + { !(i32_i * 10 < 30) && i32_i % 7 > 2 && i % 3 != 2 } }); Suggestion importance[1-10]: 2 __ Why: The clippy rewrite (x * 10 >= 30) is semantically equivalent to !(x * 10 < 30) in Rust (both use the same underlying comparison), and overflow behavior is identical. The suggestion's reasoning about overflow differences is incorrect.	Low
	Preserve original divide-by-zero semantics checked_div on usize only returns None when the divisor is 0, but the original code guarded against actual_batches > 0 and actual_rows > 0. This is equivalent only if actual_batches/actual_rows are usize; if either is a signed type, checked_div also returns None on MIN / -1 overflow. Verify the operand types are unsigned to preserve the original semantics. sandbox/plugins/analytics-backend-datafusion/rust/tests/budget_accuracy_test.rs [246-247] -let avg_batch_bytes = actual_total_bytes.checked_div(actual_batches).unwrap_or(0); -let actual_avg_row_bytes = actual_total_bytes.checked_div(actual_rows).unwrap_or(0); +let avg_batch_bytes = if actual_batches > 0 { actual_total_bytes / actual_batches } else { 0 }; +let actual_avg_row_bytes = if actual_rows > 0 { actual_total_bytes / actual_rows } else { 0 }; Suggestion importance[1-10]: 2 __ Why: actual_batches and actual_rows are usize counters from measure_actual_bytes, so checked_div semantics are equivalent to the original guard. The suggestion is speculative.	Low
Possible issue	Validate length before reconstructing boxed slice *Switching from slice::from_raw_parts_mut(..) (returning &mut [T]) to std::ptr::slice_from_raw_parts_mut(..) (returning mut [T]) and then wrapping in Box::from_raw is correct and actually preferable, but the same change should be reviewed to ensure mapping_len as usize cannot overflow the original allocation length used on the Rust side — a mismatched length here is UB. Consider asserting or documenting the invariant, and using usize::try_from(mapping_len) to defensively reject negative values before the cast. sandbox/plugins/parquet-data-format/src/main/rust/src/ffm.rs [791-797] if mapping_ptr != 0 && mapping_len > 0 { - let mapping_bytes = mapping_len as usize * std::mem::size_of::<i64>(); + let len = usize::try_from(mapping_len).expect("mapping_len must be non-negative"); + let mapping_bytes = len * std::mem::size_of::<i64>(); // Java released merge mapping — free from pool crate::memory::merge_pool().shrink(mapping_bytes); let _ = Box::from_raw(std::ptr::slice_from_raw_parts_mut( mapping_ptr as *mut i64, - mapping_len as usize, + len, )); } Suggestion importance[1-10]: 5 __ Why: Defensive validation of the length from FFI is a reasonable safety improvement, though the existing mapping_len > 0 check already guards against negative values (as i64). The suggestion mildly improves robustness but is not critical.	Low
	Verify token_count semantics preserved The rewrite changes the semantics of token_count: previously it started at 1 and was (presumably) incremented once per iteration, becoming the count after processing. Now token_count is 1 on the first iteration and increments before use, so any code inside the loop that referenced token_count as "count including current" will now be off by one relative to the original. Verify downstream uses expect this pre-increment value. sandbox/plugins/analytics-backend-datafusion/rust/src/patterns/utils.rs [85] let mut last_end = 0usize; -for (token_count, m) in (1usize..).zip(compiled_pattern.find_iter(pattern)) { +for (i, m) in compiled_pattern.find_iter(pattern).enumerate() { + let token_count = i + 1; Suggestion importance[1-10]: 5 __ Why: Legitimate concern about off-by-one semantics if token_count was incremented at end of loop originally; worth verifying though may be correct.	Low
	Verify path separator semantics unchanged The refactor changes behavior: previously, when s ended with ARRAY_MARKER, a . was inserted, but when s was empty or ended with ARRAY_MARKER in one branch and just non-empty in another, both cases added .. The new logic always inserts . before a non-marker segment when s is non-empty — including immediately after appending an ARRAY_MARKER in a prior iteration, which the original code also did (via the else if s.ends_with(ARRAY_MARKER) branch), so this appears equivalent. However, note that the original also skipped . when !s.is_empty() && s.ends_with(ARRAY_MARKER) was false only if s.is_empty(); double-check no path produces [].foo vs previous []foo. sandbox/plugins/analytics-backend-datafusion/rust/src/udf/json_extract_all.rs [299-307] if segment == ARRAY_MARKER { s.push_str(ARRAY_MARKER); } else { - // Separate field names with `.`; the leading segment gets none. - // (Both a plain field boundary and an array-marker boundary need a - // separator, which together is exactly "s is non-empty".) - if !s.is_empty() { + if !s.is_empty() && !s.ends_with(ARRAY_MARKER) { + s.push('.'); + } else if s.ends_with(ARRAY_MARKER) { s.push('.'); } s.push_str(segment); Suggestion importance[1-10]: 3 __ Why: The suggestion only asks to verify equivalence and its improved_code reverts to the original logic without concrete evidence of a bug. Low impact.	Low

Suggestions up to commit 570b2b1

Category	Suggestion	Impact
General	Remove dead partition computation or wire it up num_partitions was renamed to _num_partitions to suppress an unused-variable warning, but this indicates dead code from a partial refactor. If partition count is no longer needed here, remove the line entirely; if it's expected to be consumed downstream (as the name suggests, e.g. by the executor plan), the missing usage is likely a functional regression. sandbox/plugins/analytics-backend-datafusion/rust/src/indexed_executor.rs [915] let query_config = Arc::new(handle.query_config); -let num_partitions = query_config.target_partitions.max(1); let aggregate_mode = handle.aggregate_mode; Suggestion importance[1-10]: 5 __ Why: Prefixing with _ to silence the warning does suggest a possible partial refactor. Investigating whether num_partitions should be consumed downstream is worthwhile, since a missed usage could be a functional regression rather than merely dead code.	Low
	Guard against silent u8 truncation Truncating annotation_id (a wider integer) to u8 via as u8 silently discards upper bits and may collide with unrelated tags. The previous code used Some(...).expect("empty tag bytes") which, while awkward, was infallible for a u8. Add a bounds check (e.g. u8::try_from) and return/panic explicitly if the id exceeds u8::MAX to avoid silent aliasing bugs in tests. sandbox/plugins/analytics-backend-datafusion/rust/src/indexed_table/tests_e2e/streaming_at_scale.rs [296-297] BoolNode::Collector { annotation_id } => { - let tag = *annotation_id as u8; + let tag = u8::try_from(*annotation_id).expect("annotation_id must fit in u8"); out.push(large_collector_for(tag)); } Suggestion importance[1-10]: 4 __ Why: Adding a bounds check via u8::try_from would surface silent truncation bugs in test collector tag mapping. It's a reasonable defensive improvement, though this is test code and the practical risk is low.	Low
	Use explicit false default for clarity unwrap_or_default() returns bool::default() which is false, matching the previous Err() => false behavior. However, this relies on the return type being Result<bool, > and bool::default() == false. If the inner function's return type ever changes (e.g. to Result<Option, _>), unwrap_or_default() will silently produce a different default. Prefer .unwrap_or(false) to make the intent explicit and robust to type changes. sandbox/plugins/analytics-backend-datafusion/rust/src/indexed_table/bloom_pruner.rs [124-126] // On any error (missing bloom filter, I/O, parse) default to `false`: // conservative, meaning do not prune the row group. bloom_prune_rg_inner(store, path, metadata, arrow_schema, rg_idx, predicate) .await - .unwrap_or_default() + .unwrap_or(false) Suggestion importance[1-10]: 4 __ Why: A reasonable minor readability improvement — unwrap_or(false) is more explicit than unwrap_or_default() for a bool, making intent clearer and more robust to future return-type changes.	Low
	Verify path separator after array marker The simplified condition !s.is_empty() changes behavior versus the original: previously, when s ended with ARRAY_MARKER, the code pushed . (matches new behavior), but when s was non-empty and did NOT end with ARRAY_MARKER, it also pushed . (also matches). However, the original had a subtle case where s.ends_with(ARRAY_MARKER) implicitly meant s is non-empty. Double-check that pushing . immediately after an ARRAY_MARKER like [] produces the intended path syntax (e.g. foo[].bar vs foo[]bar); this refactor now always inserts . after [], which appears intentional but should be verified against callers. sandbox/plugins/analytics-backend-datafusion/rust/src/udf/json_extract_all.rs [302-306] -// Separate field names with `.`; the leading segment gets none. -// (Both a plain field boundary and an array-marker boundary need a -// separator, which together is exactly "s is non-empty".) if !s.is_empty() { s.push('.'); } s.push_str(segment); Suggestion importance[1-10]: 3 __ Why: The suggestion raises a valid concern about behavioral semantics of the refactor, but doesn't propose an actual code change. The original code branches (ends_with(ARRAY_MARKER) also pushes .) do collapse to !s.is_empty(), so behavior is preserved.	Low
	Verify unconditional counter increment semantics Using (1usize..).zip(...) changes semantics if token_count was previously incremented conditionally within the loop body. The original let mut token_count = 1usize; may have been incremented only on certain branches, whereas zip now increments on every match iteration unconditionally. Verify the previous code always incremented token_count per iteration; otherwise this is a behavior change. sandbox/plugins/analytics-backend-datafusion/rust/src/patterns/utils.rs [85] +let mut last_end = 0usize; +for (token_count, m) in (1usize..).zip(compiled_pattern.find_iter(pattern)) { Suggestion importance[1-10]: 2 __ Why: Verification-only suggestion with no code change; the diff shows token_count was declared but its increment logic is not visible in the hunk, so the concern is speculative.	Low
	Verify division argument order preserved checked_div on unsigned integers only returns None when the divisor is 0, but the arguments here are usize. Ensure the argument order is dividend.checked_div(divisor) — this matches the original intent, but confirm actual_batches/actual_rows are the divisors (they are), so this is correct. However, if either is usize::MAX there's no issue. The refactor is fine, but note that the original also handled 0 correctly; verify no semantic drift for very large denominators. sandbox/plugins/analytics-backend-datafusion/rust/tests/budget_accuracy_test.rs [246-247] +let avg_batch_bytes = actual_total_bytes.checked_div(actual_batches).unwrap_or(0); +let actual_avg_row_bytes = actual_total_bytes.checked_div(actual_rows).unwrap_or(0); - Suggestion importance[1-10]: 1 __ Why: The suggestion only asks to verify the change without proposing a code modification (existing_code equals improved_code), providing minimal value.	Low
Possible issue	Verify Box allocation origin for slice reconstruction *Box::from_raw on a mut [T] produced by slice_from_raw_parts_mut requires that the pointer originally came from Box<[T]> allocation. If the array was created via Vec::into_boxed_slice/Box::new([...]) this is correct, but ensure the allocator/layout matches (previously slice::from_raw_parts_mut returned &mut [T] and boxing that was equivalent). Double-check that all four freed pointers were originally allocated as Box<[T]> with matching lengths, otherwise this is UB. sandbox/plugins/parquet-data-format/src/main/rust/src/ffm.rs [794-817] +let _ = Box::from_raw(std::ptr::slice_from_raw_parts_mut( + mapping_ptr as *mut i64, + mapping_len as usize, +)); - Suggestion importance[1-10]: 3 __ Why: The suggestion asks the author to verify existing allocation semantics without providing a concrete code improvement (existing_code equals improved_code). The concern is valid but not actionable as presented.	Low
	Use closure to preserve deref semantics is_some_and(rel_has_fetch) passes the &Box (or similar smart pointer) to rel_has_fetch, whose signature expects &Rel. This may fail to compile or invoke via auto-deref inconsistently across types; use a closure with explicit deref to match the sibling Rel(r) arm and avoid ambiguity. sandbox/plugins/analytics-backend-datafusion/rust/src/session_context.rs [575] plan.relations.iter().any(|pr| match pr.rel_type.as_ref() { Some(substrait::proto::plan_rel::RelType::Root(rr)) => { - rr.input.as_ref().is_some_and(rel_has_fetch) + rr.input.as_ref().is_some_and(|r| rel_has_fetch(r)) } Some(substrait::proto::plan_rel::RelType::Rel(r)) => rel_has_fetch(r), None => false, }) Suggestion importance[1-10]: 2 __ Why: is_some_and accepts FnOnce(T) where T is the inner type of the Option, and Rust's auto-deref handles &Box<Rel> to &Rel coercion when passing to rel_has_fetch. The change is essentially a style preference and the compiler accepts both forms.	...

[github-actions]

Persistent review updated to latest commit 5b92528

[github-actions]

Persistent review updated to latest commit cf7c9be

[github-actions]

✅ Gradle check result for cf7c9be: SUCCESS

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 73.52%. Comparing base (830ff52) to head (cb4217d).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #22402      +/-   ##
============================================
+ Coverage     73.45%   73.52%   +0.07%     
+ Complexity    76232    76228       -4     
============================================
  Files          6076     6076              
  Lines        345793   345793              
  Branches      49763    49763              
============================================
+ Hits         254004   254248     +244     
+ Misses        71598    71344     -254     
- Partials      20191    20201      +10     

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

[github-actions]

✅ Gradle check result for d7f2056: SUCCESS

[github-actions]

Persistent review updated to latest commit cb4217d

[github-actions]

✅ Gradle check result for cb4217d: SUCCESS

[github-actions]

Persistent review updated to latest commit 094ddf2

[github-actions]

❌ Gradle check result for 094ddf2: 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 570b2b1

[github-actions]

❌ Gradle check result for 570b2b1: 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 8944983

[github-actions]

❌ Gradle check result for 8944983: 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 9140427

[github-actions]

❌ Gradle check result for 9140427: 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]

❌ Gradle check result for 9140427: 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 7082f75

[github-actions]

❌ Gradle check result for 7082f75: 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 ac079b2

[github-actions]

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