Skip to content

Add rustfmt check to sandbox CI - #22369

Merged
bharath-techie merged 1 commit into
opensearch-project:mainfrom
alchemist51:clean-code
Jul 7, 2026
Merged

Add rustfmt check to sandbox CI#22369
bharath-techie merged 1 commit into
opensearch-project:mainfrom
alchemist51:clean-code

Conversation

@alchemist51

Copy link
Copy Markdown
Contributor

Description

Adds Rust formatting enforcement to the sandbox native (Rust) workspace and wires it into CI.

  • rustfmt.toml at the workspace root (sandbox/libs/dataformat-native/rust/) pinned to max_width = 140, matching the OpenSearch Java code style (see CLAUDE.md). Edition is intentionally left unset so rustfmt honors each crate's own edition = 2021. Testing showed 140 also produces the least reformatting churn, i.e. it is closest to how the code was already written.
  • One-time cargo fmt --all normalization across the workspace (186 files). Prior to this the sources had never been run through rustfmt.
  • sandbox-check.yml: adds the rustfmt component to the Rust toolchain setup and a cargo fmt --all -- --check step placed before the protobuf/build steps so it fails fast on formatting regressions.

The bulk of the diff is the mechanical cargo fmt reformat; the meaningful changes are rustfmt.toml and the workflow edit.

Clippy enforcement (cargo clippy -D warnings) is deliberately not in this PR — it requires fixing pre-existing correctness-lint errors and warnings first, and will follow as a separate change.

Check List

  • Functionality includes tests (N/A — CI/formatting only)
  • 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.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@alchemist51
alchemist51 requested review from a team, jed326 and peternied as code owners July 1, 2026 08:36
@github-actions

github-actions Bot commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

PR Code Analyzer ❗

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

'Diff too large, requires skip by maintainers after manual review'


Pull Requests Author(s): Please update your Pull Request according to the report above.

Repository Maintainer(s): You can bypass diff analyzer by adding label skip-diff-analyzer after reviewing the changes carefully, then re-run failed actions. To re-enable the analyzer, remove the label, then re-run all actions.


⚠️ Note: The Code-Diff-Analyzer helps protect against potentially harmful code patterns. Please ensure you have thoroughly reviewed the changes beforehand.

Thanks.

@alchemist51 alchemist51 added the skip-diff-analyzer Maintainer to skip code-diff-analyzer check, after reviewing issues in AI analysis. label Jul 1, 2026
@alchemist51 alchemist51 closed this Jul 1, 2026
@alchemist51 alchemist51 reopened this Jul 1, 2026
@github-actions

github-actions Bot commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

PR Reviewer Guide 🔍

(Review updated until commit 38be390)

Here are some key observations to aid the review process:

🧪 No relevant tests
🔒 No security concerns identified
✅ No TODO sections
🔀 No multiple PR themes
⚡ No major issues detected

@github-actions

github-actions Bot commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

PR Code Suggestions ✨

Latest suggestions up to 38be390

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Avoid identity multiplication lint warning

The expression 1 * 1024 * 1024 will trigger a clippy identity_op warning, which can
fail strict lint checks. Since this PR adds formatting/lint CI, replace it with the
plain literal 1024 * 1024 to avoid CI breakage.

sandbox/plugins/analytics-backend-datafusion/rust/src/tiered_storage_integration_tests.rs [721]

 let metadata_cache = Arc::new(FoyerCache::new(
-    1 * 1024 * 1024,
+    1024 * 1024,
     meta_dir.path(),
     BLOCK_SIZE,
     BUFFER_POOL,
     SUBMIT_QUEUE,
     "auto",
     0,
     0.0,
     0,
     false,
 ));
Suggestion importance[1-10]: 3

__

Why: The suggestion correctly identifies that 1 * 1024 * 1024 would trigger clippy's identity_op lint, but this is a minor style issue in test code, and the pre-existing code already used this expression. Low impact.

 
Human:

</details></details></td><td align=center>Low

</td></tr></tr></tbody></table>

___

#### Previous suggestions


<details><summary>Suggestions up to commit b3fe422</summary>
<br><table><thead><tr><td><strong>Category</strong></td><td align=left><strong>Suggestion&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </strong></td><td align=center><strong>Impact</strong></td></tr><tbody><tr><td rowspan=1>General</td>
<td>



<details><summary>Avoid identity multiplication triggering lint</summary>

___


**The <code>1 * 1024 * 1024</code> expression triggers Clippy's <code>identity_op</code> lint and may cause the <br>rustfmt/clippy CI check to fail. Since this PR adds a rustfmt/lint CI check, <br>simplify to <code>1024 * 1024</code> to avoid the lint warning. This occurs in multiple places in <br>the file.**

[sandbox/plugins/analytics-backend-datafusion/rust/src/tiered_storage_integration_tests.rs [721]](https://github.com/opensearch-project/OpenSearch/pull/22369/files#diff-454c0952f2964d9d5cd77d745beacd1a2d639fefaf0ff78c152ff1e80f60920fR721-R721)

```diff
 let data_cache = Arc::new(FoyerCache::new(
-    1 * 1024 * 1024,
+    1024 * 1024,
     meta_dir.path(),
Suggestion importance[1-10]: 3

__

Why: The suggestion correctly identifies that 1 * 1024 * 1024 triggers Clippy's identity_op lint, which is a minor style/lint improvement in test code. Impact is low since this is only in tests.



</details></details></td><td align=center>Low

</td></tr></tr></tbody></table>

</details>
<details><summary>Suggestions up to commit e1ec540</summary>
<br><table><thead><tr><td><strong>Category</strong></td><td align=left><strong>Suggestion&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </strong></td><td align=center><strong>Impact</strong></td></tr><tbody><tr><td rowspan=1>General</td>
<td>



<details><summary>Remove identity multiplication to satisfy lints</summary>

___


**The expression <code>1 * 1024 * 1024</code> triggers the clippy <code>identity_op</code> lint which may fail <br>strict CI checks (especially with <code>-D warnings</code>). Since this PR adds rustfmt/lint <br>checks, replace the identity multiplication with just <code>1024 * 1024</code> to keep the code <br>lint-clean.**

[sandbox/plugins/analytics-backend-datafusion/rust/src/tiered_storage_integration_tests.rs [721]](https://github.com/opensearch-project/OpenSearch/pull/22369/files#diff-454c0952f2964d9d5cd77d745beacd1a2d639fefaf0ff78c152ff1e80f60920fR721-R721)

```diff
 let metadata_cache = Arc::new(FoyerCache::new(
-    1 * 1024 * 1024,
+    1024 * 1024,
     meta_dir.path(),
     BLOCK_SIZE,
     BUFFER_POOL,
     SUBMIT_QUEUE,
     "auto",
     0,
     0.0,
     0,
     false,
 ));
Suggestion importance[1-10]: 2

__

Why: The suggestion is technically valid as 1 * 1024 * 1024 triggers clippy's identity_op lint, but this is a very minor stylistic change in test code and the existing 1 * 1024 * 1024 pattern was not introduced by this PR (it's a formatting-only PR).

Low

@github-actions

github-actions Bot commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

✅ Gradle check result for e1ec540: SUCCESS

@codecov

codecov Bot commented Jul 1, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 73.46%. Comparing base (b99229e) to head (38be390).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff              @@
##               main   #22369      +/-   ##
============================================
- Coverage     73.52%   73.46%   -0.07%     
+ Complexity    76254    76199      -55     
============================================
  Files          6076     6076              
  Lines        345790   345793       +3     
  Branches      49762    49763       +1     
============================================
- Hits         254236   254023     -213     
- Misses        71371    71587     +216     
  Partials      20183    20183              

☔ 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

github-actions Bot commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit b2fe0a0

@github-actions

github-actions Bot commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

✅ Gradle check result for b2fe0a0: SUCCESS

@github-actions

github-actions Bot commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 1342ab4

@github-actions

github-actions Bot commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

❌ Gradle check result for 1342ab4: 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

github-actions Bot commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 0223f88

@github-actions

github-actions Bot commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit ec5ba14

@github-actions

github-actions Bot commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

✅ Gradle check result for ec5ba14: SUCCESS

@github-actions

github-actions Bot commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit b3fe422

Apply `cargo fmt --all` across the sandbox native (Rust) workspace and wire a
`cargo fmt --all -- --check` step into the sandbox-check GitHub Action so
formatting regressions are caught in CI.

Formatting uses rustfmt's default style (100-char width). No rustfmt.toml is
added: rustfmt resolves config by walking up from each file's directory, and
the workspace pulls in plugin crates from sibling paths (sandbox/plugins/...)
that a config under dataformat-native/rust/ would not cover — so a width
override there would apply only to the three lib crates and silently leave the
plugin crates on the default. Relying on the default keeps every sandbox crate
on one consistent style.

The rustfmt component is added to the Rust toolchain setup and the format
check runs before the build steps so it fails fast. Clippy enforcement will
follow in a separate change.

Signed-off-by: Arpit Bandejiya <abandeji@amazon.com>
@github-actions

github-actions Bot commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 38be390

@github-actions

github-actions Bot commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

❕ Gradle check result for 38be390: UNSTABLE

Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

@bharath-techie bharath-techie left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@alchemist51

Copy link
Copy Markdown
Contributor Author

@peternied @jed326 @peterzhuamazon folks please review

@bharath-techie
bharath-techie merged commit 830ff52 into opensearch-project:main Jul 7, 2026
16 checks passed
@alchemist51 alchemist51 mentioned this pull request Jul 7, 2026
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

skip-diff-analyzer Maintainer to skip code-diff-analyzer check, after reviewing issues in AI analysis.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants