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 8944983)

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 (page_index / 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 sandbox/plugins/analytics-backend-datafusion/rust/src/indexed_table/tests_e2e/null_columns.rs sandbox/plugins/analytics-backend-datafusion/rust/src/indexed_table/eval/bitmap_tree.rs Sub-PR theme: Add clippy step to sandbox CI workflow Relevant files: .github/workflows/sandbox-check.yml Sub-PR theme: Mechanical clippy cleanups in tests (struct-init, needless-mut, assert!) 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 sandbox/plugins/analytics-backend-datafusion/rust/src/query_tracker.rs sandbox/plugins/analytics-backend-datafusion/rust/src/memory.rs

⚡ Recommended focus areas for review Broken assertion messages Several assert_eq!(cond, true/false, "msg") were rewritten to assert!(cond, "msg") but the closing ); and message argument were left on separate lines from the old macro, producing calls like assert!(cache.should_skip_sweep(), "usage=0% < threshold=75%: sweep must be skipped");. Verify these still compile — if the message ended up outside the macro invocation it becomes a dangling string expression. Also test_ffm_snapshot_stats_valid_ptr_returns_zero_and_fills_buffer shows a stray closing } after the for loop, suggesting the diff may have mismatched braces. assert!( !cache.should_skip_sweep(), "threshold=0.0: should never skip even when cache is empty" ); Test lost coverage test_concurrent_writes_different_files was rewritten from for i in 0..file_count to for filename in &filenames, but the schema_ptrs vector (populated in a prior loop) is no longer indexed by i, and the thread body appears unchanged. If the thread previously referenced schema_ptrs[i] or i for indexing, the loop variable rename may have silently broken per-thread assignment. Confirm the closure body no longer references i or an indexed schema. schema_ptrs.push(schema_ptr); } for filename in &filenames { let filename = filename.clone(); let success_count = Arc::clone(&success_count); let handle = thread::spawn(move || { Semantics change in test predicate Clippy's nonminimal_bool rewrite changed !(i32_i * 10 < 30) to i32_i * 10 >= 30. These are equivalent for i32 only if i32_i * 10 does not overflow; for values near i32::MAX/10 the wrapped-around result flips sign and the two expressions diverge. Not a real concern for the small test inputs here, but note the general pattern — the same rewrite is applied in stats_prune_multi_segment_multi_partition_with_not and elsewhere. (i32_i * 10 >= 30) && i32_i % 7 > 2 && i % 3 != 2 }); Removed `mut` on Reservation Many reservations were changed from let mut reservation = … to let reservation = …. Calls like reservation.try_grow(1000).unwrap() presumably take &mut self. If DataFusion's MemoryReservation::try_grow requires &mut self, these tests will fail to compile. Verify that try_grow takes &self in the version in use, or restore the mut bindings. let reservation = make_reservation(&pool, "test"); reservation.try_grow(1000).unwrap(); assert_eq!(qp.current_bytes(), 1000);

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 8944983

Explore these optional code suggestions:

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

---

Previous suggestions

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

Suggestions up to commit 094ddf2

Category	Suggestion	Impact
Possible issue	Preserve i64 type consistency in match The None branch now returns rg.num_rows directly, but if rg.num_rows is not already i64 the match arms will have mismatched types and fail to compile (previously it was cast via as i64). Confirm the field type or restore the cast to keep both arms i64. sandbox/plugins/analytics-backend-datafusion/rust/src/indexed_table/eval/single_collector.rs [261-263] -let surviving_rows = match page_ranges { - None => rg.num_rows, +let surviving_rows: i64 = match page_ranges { + None => rg.num_rows as i64, Some(ranges) => ranges.iter().map(|(lo, hi)| (hi - lo) as i64).sum::<i64>(), }; Suggestion importance[1-10]: 6 __ Why: Valid observation about potential type mismatch between the None arm returning rg.num_rows and the Some arm summing to i64. If rg.num_rows is not already i64, this would cause a compile error. However, since the PR presumably compiles, rg.num_rows is likely already i64, but the explicit type annotation improves clarity.	Low
	Verify path-building equivalence after simplification The simplification changes behavior when s ends with ARRAY_MARKER: previously, the code hit the else if s.ends_with(ARRAY_MARKER) branch and pushed ., but the original outer if !s.is_empty() && !s.ends_with(ARRAY_MARKER) guard already excluded that case — so the original code pushed . after an array marker via the else if, but ALSO would have pushed . unconditionally in the new code. Verify that paths like arr[].field produce the same string as before; if ARRAY_MARKER itself contains or ends with ., the new code may double-dot. Consider adding a regression test covering an array-marker-preceded segment. 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]: 5 __ Why: This raises a valid concern about behavioral equivalence: the original code had a separate branch when s.ends_with(ARRAY_MARKER), and the simplified version might change output for that case. However, the suggestion only asks to verify and doesn't propose a concrete fix.	Low
	Ensure allocation layout matches boxed-slice free Box::from_raw on mut [T] created via slice_from_raw_parts_mut only correctly reclaims memory that was originally allocated as a boxed slice with the same layout. If the arrays were allocated via Vec::into_boxed_slice this is fine, but ensure the allocation path matches; otherwise the deallocator may free with the wrong layout, causing UB. Verify each gen_ and mapping_ptr was produced by Box<[T]>::into_raw (not Vec::as_mut_ptr with capacity != len). sandbox/plugins/parquet-data-format/src/main/rust/src/ffm.rs [794-817] 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); + // Must have been produced by Box::<[i64]>::into_raw with exactly mapping_len elements. 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 user to verify allocation layout compatibility, which is a valid concern for FFI safety but only asks for verification rather than proposing a concrete fix. The improved_code is essentially identical to the existing code with just added comments.	Low
	Verify counter semantics after zip refactor The refactor changes token_count semantics: it was previously incremented only after matches were processed (starting at 1 and rising with each iteration body), but zip now assigns token_count = 1 for the first match, 2 for the second, etc. Verify this matches the original behavior — if the original code incremented token_count conditionally inside the loop (not shown), the zip-based rewrite will produce different values. Consider using .enumerate() with an explicit + 1 if the intent was strictly sequential 1-based numbering, or preserve the original mutable counter if increments were conditional. sandbox/plugins/analytics-backend-datafusion/rust/src/patterns/utils.rs [85] let mut last_end = 0usize; +let mut token_count = 1usize; -for (token_count, m) in (1usize..).zip(compiled_pattern.find_iter(pattern)) { +for m in compiled_pattern.find_iter(pattern) { Suggestion importance[1-10]: 3 __ Why: The suggestion asks to verify semantics but the zip-based rewrite with (1usize..) produces identical 1-based sequential values as the original mutable counter would. The concern is largely speculative without evidence of conditional increment in the original.	Low
	Fix closure deref for recursive fetch check is_some_and takes ownership of a closure receiving the inner value by value; passing |r| rel_has_fetch(r) where r: &Box will not auto-deref to &Rel. Use .as_deref() or dereference explicitly to ensure the recursion compiles and matches the previous map_or(false, ...) semantics. 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 suggestion incorrectly claims a compilation issue. In prost-generated code, input is typically Option<Box<Rel>>, and is_some_and(|r| rel_has_fetch(r)) where r: &Box<Rel> auto-derefs to &Rel via Deref coercion. The code compiles as-is; the suggestion is a minor stylistic improvement at best.	Low
General	Guard truncation of annotation_id to u8 *The previous code used Some(annotation_id as u8).expect("empty tag bytes"), which was a no-op but documented the invariant. The replacement silently truncates annotation_id to a u8; if any annotation_id exceeds 255, this will now silently produce the wrong collector rather than surfacing the mismatch. Add a debug assertion or bounds check to preserve the original safety intent. sandbox/plugins/analytics-backend-datafusion/rust/src/indexed_table/tests_e2e/streaming_at_scale.rs [296-297] BoolNode::Collector { annotation_id } => { + debug_assert!(*annotation_id <= u8::MAX as _, "annotation_id out of u8 range"); let tag = *annotation_id as u8; out.push(large_collector_for(tag)); } Suggestion importance[1-10]: 4 __ Why: The original code Some(*annotation_id as u8).expect("empty tag bytes") was a no-op (Some is never None), so the "safety intent" claim is misleading. Adding a debug_assert for the u8 range is a reasonable defensive improvement for test code but has limited impact.	Low
	Confirm zero-sentinel is intentional checked_div on usize only returns None when the divisor is 0, but it also panics-free-guards against nothing else. This is functionally equivalent to the old code, but note that if actual_batches/actual_rows are ever usize::MAX or other unusual values the behavior is unchanged — verify that the callers actually want 0 as the sentinel (rather than skipping the assertion or reporting an error) when there are zero batches/rows, since a zero here will silently propagate into downstream ratio comparisons. 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]: 2 __ Why: The suggestion only asks to verify behavior and provides an improved_code identical to the existing_code, offering no actionable change.	Low
	Document/handle CAS-loser stale-read race The collapsed if preserves original semantics, but note that on compare_exchange failure (another thread won the race), the code now falls through to CACHED_RESIDENT.load(...) — same as before. However, if the CAS fails, CACHED_RESIDENT may not yet have been updated by the winner (the winner is between CAS and store). This race existed before the refactor too, but consider whether the caller can tolerate reading a stale CACHED_RESIDENT value in that narrow window; if not, spin/retry on CAS failure. 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); + return r; +} +CACHED_RESIDENT.load(Ordering::Relaxed) - Suggestion importance[1-10]: 2 __ Why: The suggestion acknowledges the race existed before the refactor and provides identical improved_code, offering no concrete change to the PR.	Low

Suggestions up to commit cb4217d

Category	Suggestion	Impact
General	Narrow overly broad lint suppressions on task Broadly allowing unreachable_code and unused_variables on the entire spawned task hides real bugs beyond just the never-looping outer scaffold. If the intent is to keep restart/backoff scaffolding for future use, gate only the specific unreachable region with a narrower #[allow] or a #[cfg(...)] block; otherwise remove the dead scaffolding entirely to avoid masking genuine issues introduced later in this closure. 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]: 5 __ Why: Reasonable concern that broad #[allow] attributes over an entire async block can mask genuine issues in future code. However, the PR author already documented the intent explicitly in a comment, so this is a moderate maintainability suggestion.	Low
	Use explicit false instead of default Relying on bool::default() (which is false) to encode "conservative: don't prune" is fragile — if the return type ever changes to something whose default means "prune", this silently becomes a correctness bug. Prefer an explicit .unwrap_or(false) to make the safety-critical intent tied to the value, not to the type's default. sandbox/plugins/analytics-backend-datafusion/rust/src/indexed_table/bloom_pruner.rs [122-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]: 5 __ Why: Reasonable minor readability/robustness improvement: unwrap_or(false) makes the safety-critical intent explicit rather than relying on bool::default().	Low
	Guard against silent u8 truncation of tag **The previous code used Some(x).expect("empty tag bytes") which was a no-op assertion but semantically implied a validation. The new code silently truncates annotation_id (a wider integer) to u8 via as, which will wrap around for values ≥ 256 and route to the wrong collector. Add an explicit range check (e.g., assert!(annotation_id <= u8::MAX as _, ...)) or use u8::try_from(annotation_id).expect(...) to preserve fail-fast behavior on out-of-range tags. 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 out of u8 range"); out.push(large_collector_for(tag)); } Suggestion importance[1-10]: 4 __ Why: Valid observation that as u8 silently truncates, but this is test harness code and the original Some(x).expect(...) was also a no-op that never validated range. Minor improvement to fail-fast behavior.	Low
	Confirm checked_div argument order checked_div on usize only returns None when the divisor is 0, so this is equivalent to the original guards — but the previous code used actual_batches / actual_rows as divisors, while checked_div here is called on actual_total_bytes as receiver. Verify argument order: the intent is actual_total_bytes / actual_batches, which matches actual_total_bytes.checked_div(actual_batches). This is correct, but ensure the same holds for the row-bytes line and that no future refactor swaps them. 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]: 2 __ Why: The suggestion merely asks to verify correctness without proposing a change; the existing_code and improved_code are identical.	Low
	Confirm merged branch preserves semantics The merged condition slightly changes short-circuit behavior: previously the wildcard branch only ran when tokens were unequal, now it may also match when both are equal AND both happen to satisfy the prefixes (harmless, still +1). More importantly, if the original else if also handled the case where an equal-but-wildcard pair should still count once, this is preserved. Behavior is equivalent — no change needed, just confirm no third branch was dropped. sandbox/plugins/analytics-backend-datafusion/rust/src/patterns/eval.rs [155-158] for (preprocessed_token, candidate_token) in tokens.iter().zip(candidate.iter()) { - // A position matches when the tokens are equal, or when a wildcard - // input (`<*…>`) lines up with a numbered candidate token (`<token…>`). if preprocessed_token == candidate_token || (preprocessed_token.starts_with("<*") && candidate_token.starts_with("<token")) { score += 1; } } Suggestion importance[1-10]: 1 __ Why: Suggestion is purely a verification request with identical existing_code and improved_code; the merged condition is logically equivalent to the original.	Low
Possible issue	Verify token_count semantics after refactor The refactor changes token_count from a running mutable counter that was likely incremented conditionally (e.g. only for actual tokens, not for gap segments) into a value bound one-to-one with each regex match via zip. If the original code incremented token_count on every iteration unconditionally this is fine, but if it advanced only after producing a token, downstream indexing/labels using token_count may now be off. Verify the semantics match the pre-refactor behavior. 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)) { + let start = m.start(); + let end = m.end(); + if start > last_end { Suggestion importance[1-10]: 4 __ Why: Only asks to verify semantics without proposing a concrete fix; potential concern is valid but speculative without evidence of behavior change.	Low
	Ensure boxed-slice reclaim matches allocation type **Box::from_raw expects a *mut T, not a *mut [T] produced by slice_from_raw_parts_mut. The correct construction for reclaiming a boxed slice is Box::from_raw(std::ptr::slice_from_raw_parts_mut(...)) only if the value has type mut [T]—which it does here—but the previous slice::from_raw_parts_mut produced a &mut [T] reference that Box::from_raw cannot accept. Verify the new form actually compiles and correctly reclaims the allocation; if not, use Box::from_raw(std::ptr::slice_from_raw_parts_mut(ptr, len) as mut [i64]) explicitly, or revert to constructing a Vec::from_raw_parts for deallocation to avoid undefined behavior on free. sandbox/plugins/parquet-data-format/src/main/rust/src/ffm.rs [794-797] 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); - let _ = Box::from_raw(std::ptr::slice_from_raw_parts_mut( + let _ = Vec::from_raw_parts( mapping_ptr as *mut i64, mapping_len as usize, - )); + mapping_len as usize, + ); } Suggestion importance[1-10]: 3 __ Why: The suggestion is largely incorrect: Box::from_raw does accept *mut [T] produced by std::ptr::slice_from_raw_parts_mut, which is idiomatic Rust for reclaiming a boxed slice. The old code used slice::from_raw_parts_mut returning &mut [T] which coerces to *mut [T], so both forms are equivalent. Switching to Vec::from_raw_parts would actually be incorrect if the allocation originated as Box<[T]>.	Low

Suggestions up to commit d7f2056

Category	Suggestion	Impact
Possible issue	Preserve original token counter semantics Using (1usize..).zip(find_iter) changes semantics from the previous code: previously token_count started at 1 and was (presumably) incremented per iteration inside the loop, but now it is bound to 1, 2, 3, ... for every match unconditionally. If the loop originally only incremented token_count when a token was actually pushed (e.g. after handling the pre-match gap), this refactor silently changes the numbering. Verify the original increment placement and, if needed, use enumerate() with an explicit +1 only where a token is emitted. sandbox/plugins/analytics-backend-datafusion/rust/src/patterns/utils.rs [85] -for (token_count, m) in (1usize..).zip(compiled_pattern.find_iter(pattern)) { +for m in compiled_pattern.find_iter(pattern) { + let token_count = token_order.len() + 1; let start = m.start(); let end = m.end(); if start > last_end { Suggestion importance[1-10]: 6 __ Why: Valid concern: the refactor binds token_count to the match index unconditionally, but if the original code only incremented it in specific branches, semantics could differ. Worth verifying, though the improved_code fix using token_order.len() + 1 is speculative.	Low
	Verify path separator refactor equivalence The refactored condition changes behavior when s is non-empty but does NOT end with ARRAY_MARKER and the branch was previously the else fall-through: previously only two cases added a . (non-empty and not ending with marker, OR ending with marker). Now every non-empty case adds ., which is equivalent — but only if the original two branches truly covered the same set. Double-check that ARRAY_MARKER never contains a . and that the original code did not intentionally skip a separator in some path (e.g. when the previous segment was itself an array marker followed immediately by a field, which would now produce [].field vs previously []field). sandbox/plugins/analytics-backend-datafusion/rust/src/udf/json_extract_all.rs [302-306] 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('.'); } s.push_str(segment); Suggestion importance[1-10]: 5 __ Why: The refactor genuinely changes behavior: previously when s ended with ARRAY_MARKER, a . was still added, but the branch structure was different. The suggestion correctly identifies a potential subtle equivalence issue worth verifying.	Low
	Use as_deref for boxed Rel traversal is_some_and takes FnOnce(T) -> bool (by value), but rel_has_fetch takes &Rel. Passing |r| rel_has_fetch(r) where r: &Box will not auto-deref through the closure into &Rel cleanly in all compiler versions and can cause a type mismatch. Prefer .as_deref().is_some_and(rel_has_fetch) or keep the original map_or(false, ...) pattern to avoid subtle regressions. 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]: 3 __ Why: The original is_some_and(|r| rel_has_fetch(r)) form typically compiles fine because &Box<Rel> derefs to &Rel at call sites. Using as_deref is a minor stylistic improvement, not a correctness fix.	Low
	Verify control-flow equivalence after refactor This refactor subtly changes control flow: the original if chunk_seg.is_some() && chunk_seg != Some(rg.segment_idx) guarded the flush, and any subsequent code in the outer block presumably ran regardless. With if let Some(seg) = chunk_seg { if seg != rg.segment_idx && !chunk_rg_indices.is_empty() { ... } }, the case where chunk_seg == Some(rg.segment_idx) (same segment) now enters the outer if let but skips the inner block — which is fine — however make sure no else branch that handled chunk_seg.is_none() was inadvertently dropped or reordered. Verify the rest of the loop body still executes for all prior code paths. 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(), - Suggestion importance[1-10]: 3 __ Why: The suggestion only asks to verify equivalence without identifying a concrete bug; the refactor appears semantically equivalent to the original nested condition.	Low
General	Fix or remove dead restart scaffolding Broadly allowing unreachable_code and never_loop masks real bugs — if the inner loop always returns, the documented restart/backoff scaffolding never executes, meaning the task will silently die on unexpected exit rather than restarting as intended. Either implement the restart behavior correctly (e.g., use continue on error paths) or remove the dead scaffolding so future readers aren't misled. 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 -)] +// TODO: implement restart-on-unexpected-exit or remove dead scaffolding below. +#[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: Broadly suppressing unreachable_code and never_loop does mask potential real issues, and the concern that the documented restart-on-exit behavior never executes is legitimate and worth addressing.	Low
	Guard against multiplication overflow in reservation mapping_bytes * 2 can overflow usize for very large mappings, and the identity initialization is redundant work that will be overwritten by the subsequent per-chunk assignment. Use checked_mul for the reservation size and avoid the initial loop to reduce overhead. sandbox/plugins/parquet-data-format/src/main/rust/src/writer.rs [827-830] -reservation.request(mapping_bytes * 2)?; +let double_bytes = mapping_bytes + .checked_mul(2) + .ok_or("mapping size overflow")?; +reservation.request(double_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 protection on mapping_bytes * 2 is a reasonable defensive measure, though practically unlikely for realistic mapping sizes. The suggestion is valid but low impact.	Low
	Use explicit false for conservative default bool::default() is false, which matches the previous behavior, but unwrap_or_default() obscures intent and is fragile: if the return type of bloom_prune_rg_inner ever changes (e.g. to Result<Option> or a newtype whose default is true), the conservative "don't prune" contract silently breaks. Prefer unwrap_or(false) to make the conservative default explicit and type-stable. sandbox/plugins/analytics-backend-datafusion/rust/src/indexed_table/bloom_pruner.rs [124-126] 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: Reasonable style/robustness improvement making the conservative default explicit, but low impact since bool::default() is well-defined as false.	Low
	Remove unused partition clamp computation _num_partitions is computed but never used; if target_partitions must be at least 1 for downstream logic, the clamped value should either be propagated back into query_config or the line removed entirely. Leaving unused clamping suggests a latent bug where target_partitions may still be 0 elsewhere in the pipeline. 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]: 3 __ Why: The prefixed _num_partitions indicates intentional retention (possibly for future use). Removing it is a minor cleanup but flagging a potential latent bug is speculative.	Low

Suggestions up to commit cf7c9be

Category	Suggestion

[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?