feat(rust): establish Rust graph computing modernization framework (#355)

[KHARSHAVARDHAN-eng]

Summary

This PR establishes the initial Phase 1 foundation for the incremental Rust modernization roadmap described in #355.

The scope is intentionally kept small and isolated. It introduces a standalone Rust crate for graph-computing experiments, together with the initial C-ABI boundary, correctness fixtures, benchmarks, documentation, and CI support.

Included in this PR

• Introduces the computer-rust crate
• Adds an initial CSR graph representation
• Adds an initial PageRank kernel
• Defines an explicit C-ABI boundary and error-code contract
• Adds correctness fixtures and tolerance utilities
• Adds an initial PageRank benchmark
• Adds cargo fmt, Clippy, tests, release build, and benchmark compilation to Rust CI
• Adds the Rust modernization roadmap and proposed follow-up task boundaries

Intentionally out of scope

To keep this first step small and reviewable, this PR does not include:

• Java/JNI host integration
• Go/CGO host integration
• SSSP implementation
• Atomic aggregators
• Changes to the existing Java or Go runtime paths
• Production migration or replacement of existing implementations

Those areas are intended for dedicated follow-up tasks after the core Rust slice, interoperability boundary, and acceptance criteria are reviewed.

Validation

The implementation is scoped to an isolated Rust slice and does not modify existing Java or Go runtime behavior.

The Rust CI workflow includes:

• cargo fmt --check
• cargo clippy --all-targets -- -D warnings
• cargo test --all-targets --verbose
• cargo build --release
• cargo bench --no-run

Related Issue

Part of the incremental Rust modernization roadmap tracked in #355.

[imbajin]

Blocking: yes. The Rust framework currently has correctness and delivery blockers: invalid endpoints can corrupt the CSR, negative-weight SSSP can fail to terminate, and the new Rust CI/license/integration path is not passing or connected; the exact head has failed checks. Evidence: actionlint on .github/workflows/rust-ci.yml; gh run view 31351599313 --log-failed; computer-rust/src/kernel/{csr,sssp}.rs; Java/Go bridge sources.

[imbajin]

‼️ GitHub Actions branch filters use glob syntax, but /^release-.*$/ is rejected as an invalid branch name/pattern (actionlint reports the leading /, ^, and trailing / as invalid); the exact-head Rust CI run 31351599715 ended in startup_failure, so formatting, clippy, tests, and release build never ran. Please use a valid glob such as release-* and rerun the workflow.

[imbajin]

⚠️ The Apache header contains You me obtain a copy, which makes the exact-head check-license-header job fail on this file. Please correct the standard license text to You may obtain a copy and rerun the license check.

[imbajin]

‼️ degree counts every edge whose source is in range, but the fill loop skips an out-of-range destination. For example, from_edges(2, &[(0, 99, 1.0)]) allocates one slot and leaves it as the default 0 -> 0 edge, so PageRank/SSSP consume a topology that was never supplied. Please validate both endpoints when counting and filling, and return an error from the C API for invalid vertices.

[imbajin]

‼️ This Dijkstra loop accepts negative weights and has no negative-cycle detection. A graph containing 0 -> 1 = -1 and 1 -> 0 = -1 keeps lowering both distances and pushing new heap entries, so the exported SSSP call can run without termination and exhaust CPU/memory. Please reject negative/non-finite weights at the API boundary or use an algorithm that detects negative cycles.

[imbajin]

⚠️ The new Rust library is not reachable from the advertised Vermeer path: NewRustKernelBridge hard-codes available: false, and ComputePageRank always executes the Go fallback. The Java bridge likewise computes in Java and only declares nativeGetVersion, which does not match Rust's computer_kernel_version export. Please implement and test the JNI/CGO bindings and native-path selection, or document this PR as fallback-only instead of presenting an active Rust integration.

[imbajin]

Blocking: yes. Summary: Independent gaps remain in the Go fallback's input validation, the C-ABI graph builder lifecycle, and the new correctness tests' ability to catch invalid output. Evidence: exact-head sources under computer-rust/, vermeer/apps/compute/, and the Maven/Go test wiring; the existing exact-head review already covers the branch filter, license header, CSR corruption, negative SSSP, and native bridge reachability findings.

[imbajin]

‼️ The Go fallback counts an edge in outDegree when only src is valid, but the propagation loop later requires both endpoints to be valid. With numVertices=2 and an edge (0, 99), vertex 0 divides its rank by an edge that contributes nothing, so the fallback result loses mass and diverges from the Rust path. Please validate both endpoints before counting, or reject invalid edges with an error.

[imbajin]

‼️ computer_graph_add_edge() still returns success after computer_graph_finalize() has populated builder.csr. Subsequent edges are appended to edges, but both compute functions keep reading the old CSR, so the C caller silently computes an obsolete graph. Please reject additions after finalization or invalidate/rebuild the CSR before allowing computation.

[imbajin]

⚠️ An out-of-range source_vertex is passed to SsspKernel::compute(), which returns an all-INFINITY vector, and the FFI function still returns 0. This is indistinguishable from a valid graph whose vertices are all unreachable. Please validate the source at the C boundary and return a documented error code.

[imbajin]

⚠️ NaN > epsilon is false, so assert_parity([f64::NAN], [0.0], epsilon) returns Ok(()); l1_distance() likewise returns Ok(NaN). A non-finite kernel result can therefore pass the differential fixture. Please reject non-finite inputs/differences and add NaN/Infinity regression cases.

[imbajin]

⚠️ PageRankKernel::new() accepts non-finite or out-of-range parameters without validation. A NaN damping factor produces NaN ranks, and a NaN tolerance prevents convergence because every comparison is false; damping values outside [0, 1] also violate the probability contract. Please validate finite damping/tolerance at the API boundary and return an error for invalid input.

[imbajin]

⚠️ The PageRank test uses only a symmetric three-cycle, whose expected vector is uniform even if edge propagation is broken or the topology is ignored. Please add an asymmetric graph with a dangling vertex and assert a fixed reference result so transition and dangling-node handling are actually exercised.

[imbajin]

⚠️ This new Java test is not included by the module's unit-test execution: computer-test/pom.xml includes only **/UnitTestSuite.java, and UnitTestSuite does not reference RustKernelBridgeTest. The class can compile while its fallback regression never runs in CI. Please add it to the suite or configure an explicit Surefire include, then verify the test count.

[imbajin]

⚠️ The added Go bridge test is not exercised by the repository CI workflow, which builds Vermeer but does not run go test. Please add at least go test ./apps/compute (and a native-path job when bindings exist) so fallback behavior is continuously verified.

[imbajin]

‼️ std::ptr is unused in this file, while the new workflow runs cargo clippy --all-targets -- -D warnings. Once the workflow startup issue is fixed, this import will fail the quality gate. Please remove it and rerun Clippy.

[imbajin]

⚠️ computer_kernel_version() returns a pointer into a thread-local CString; that pointer becomes invalid when the calling thread exits, and the header does not document the borrowed lifetime or provide a copy/free contract. A C caller that stores the pointer or passes it across threads can use freed memory. Please return process-lifetime static storage or expose an explicit copy API and document ownership.

[KHARSHAVARDHAN-eng]

Hi @imbajin,

Thanks for the detailed review. I’ve addressed the requested issues in the latest commit 9e72b0e.

The updates include the CI/license fixes, CSR/FFI validation, SSSP/PageRank safety checks, aggregator synchronization, test coverage improvements, CI test wiring, and documentation updates.

I also verified the final diff and kept the changes scoped to the review feedback. Could you please take another look when you have a chance?

Thanks!

[imbajin]

Blocking: yes. Summary: The exact head still cannot execute Vermeer CI, and the advertised parity contract is not met: PageRank stops on a per-vertex maximum error instead of the documented L1 threshold while the Java/Go fallbacks accept invalid parameters. Evidence: actionlint on .github/workflows/vermeer-ci.yml; gh run view 31675719210 and 31675719706; computer-rust/src/kernel/pagerank.rs:83-95; computer/computer-core/.../RustKernelBridge.java:61-73; vermeer/apps/compute/rust_bridge.go:47-67.

[imbajin]

‼️ The workflow containing this new test step is still unexecutable on the exact head: actionlint rejects the existing push branch filter /^release-.*$/ at line 23, and run 31675719210 finished startup_failure with no jobs. Please replace the filter with a GitHub Actions glob such as release-*, then rerun and require a successful Vermeer CI run so this added test actually executes.

[imbajin]

⚠️ This fallback only rejects numVertices == 0; dampingFactor, tolerance (including NaN/Inf/negative/out-of-range) and invalid endpoints are otherwise accepted or ignored, while the Rust C-ABI returns -4/-1 for those inputs and the roadmap promises identical validation. Please validate and return errors consistently, or revise the contract, and add regression tests.

[imbajin]

⚠️ This fallback has no validation for dampingFactor or tolerance; NaN or out-of-range values flow into arithmetic and can return NaN or invalid ranks, while the Rust C-ABI rejects them with -4 and the roadmap promises parity. Please validate finite damping in [0,1] and finite non-negative tolerance, define the error behavior, and add regression tests.

[imbajin]

‼️ The loop terminates on the maximum single-vertex difference (max_diff < tolerance), but the roadmap declares an L1 error bound. With N vertices, this permits aggregate L1 error up to N*tolerance, so the advertised parity guarantee is not met. Please accumulate the L1 difference for convergence, or change the contract and tests to match.

[imbajin]

⚠️ epsilon itself is never validated. With epsilon = NaN, diff > epsilon is false, so finite mismatched vectors can return Ok; this lets an invalid tolerance bypass the differential check. Please reject non-finite or negative epsilon before the loop and add a NaN regression case.

[imbajin]

⚠️ This is labeled as the standard Karate Club fixture, but it contains only 35 directed edges, all sourced from vertices 0-3; vertices 4-33 have no outgoing edges. The current test only checks non-empty data, so benchmarks and parity inputs are materially truncated. Please add the complete dataset and assert edge count/key adjacency, or rename and document this as a reduced fixture.

[imbajin]

⚠️ Despite the powerlaw name, this generator gives every vertex an out-degree of only avg_degree + (src % 5), i.e. 10-14 for the benchmark input, with no heavy tail. The benchmark therefore does not exercise power-law hotspots or memory behavior. Please generate a reproducible heavy-tailed distribution or rename the fixture to match its regular topology.

[imbajin]

Please pause further coding for now. This PR has been marked as Draft. Before continuing, please submit and get approval for a complete review plan covering at least the objectives and scope, implementation steps, API and compatibility impact, testing and validation, risks and rollback strategy, and acceptance criteria. Until the plan is approved, the previous review process will remain paused; follow-up review can resume after the complete plan is approved.

[KHARSHAVARDHAN-eng]

Thanks for the detailed review, @imbajin. I’ve paused further implementation as requested.

I’ll prepare a complete review/implementation plan covering the scope, implementation steps, API/compatibility impact, testing and validation, risks/rollback, and acceptance criteria, and will wait for approval before making further code changes.

[KHARSHAVARDHAN-eng]

Hi @imbajin, I’ve updated the PR according to the agreed Phase 1 scope. The premature Java/Go bridges and secondary Rust components have been removed, and the PR is now focused on the isolated Rust crate, C-ABI boundary, correctness fixtures/benchmark, CI, and the roadmap/RFC documentation.

The PR is now ready for review. Thank you!

[imbajin]

Blocking: no. Summary: The exact head still has two non-duplicate contract issues: PageRank ignores supplied edge weights, and the parity helper checks per-element error instead of the documented L1 bound. Evidence: computer-rust/src/kernel/pagerank.rs:79-82 and computer-rust/src/fixtures/tolerance.rs:48-70; exact-head Rust CI is startup_failure and license/CodeQL are action_required.

[imbajin]

⚠️ The C API accepts a weight and CsrGraph stores it, but this binding explicitly discards the weights (let (neighbors, _)) and always sends ranks[v] / out_degree to every neighbor. Consequently, edges with weights 1.0 and 100.0 produce the same PageRank result, despite the exported API and CSR carrying edge weights. Please either use normalized outgoing weights in the transition or remove/document the argument as topology-only, and add an asymmetric-weight regression test. Evidence: computer-rust/src/ffi/c_api.rs:45-64, computer-rust/src/kernel/csr.rs:60-73, and this line.

[imbajin]

⚠️ assert_parity checks each element against epsilon, but the roadmap promises an L1-distance bound. For two elements whose absolute differences are each 0.75 * epsilon, this function returns Ok while the L1 distance is 1.5 * epsilon; a caller can therefore accept a result outside the advertised contract. Please compare l1_distance(actual, expected) with a validated epsilon, or rename/document this as a per-element check, and add a multi-element regression test. Evidence: the per-element diff > epsilon condition at this line and the L1 contract in docs/rust-modernization-roadmap.md.

[bitflicker64]

Blocking: no. Summary: The biggest simplification here is deleting PageRankKernel::new and the copy of its validation that c_api.rs carries only to avoid that constructor's panic, replacing both with a single try_new match; the rest is a unit struct used as a namespace, a fixture that is not the dataset it is named after, and three dead public symbols. Scope-wise this PR is fine: #355 explicitly asks for an isolated kernel prototype, correctness fixtures, and Rust CI, so the crate's existence and its size are not findings. Evidence: reviewed the full diff at c1fc10a and the crate as checked out at that SHA; grep -rn 'PageRankKernel::new' computer-rust/ returns the definition plus one bench and two tests, grep -rn 'DifferentialTolerance' computer-rust/ returns only its own file, grep -rn '\bEdge\b' computer-rust/ returns only the declaration at csr.rs:19, CsrGraph::new has zero call sites, and the karate_club edge list counts 35 edges over vertices {0..13,17,19,21,27,28,30,31,32}.

[bitflicker64]

⚠️ These six lines are a character-for-character copy of the two checks already inside PageRankKernel::try_new (pagerank.rs:33-44). They only exist because line 108 calls PageRankKernel::new, which is try_new(..).expect(..), so without the pre-check a bad parameter panics instead of returning -4.

Worth collapsing now rather than in Phase 3, for a reason beyond line count: Cargo.toml sets panic = "abort" on the release profile. A pub extern "C" function that can reach a panic! in a panic = "abort" cdylib does not unwind and does not return an error code, it takes the host process with it. The roadmap puts a JVM and a Go runtime on the other side of this boundary, so the panicking constructor is the thing to delete before that lands.

Drop lines 101-106 and line 108 in favour of:

let kernel = match PageRankKernel::try_new(damping_factor, max_iterations, tolerance) {
    Ok(k) => k,
    Err(_) => return -4,
};

Then delete PageRankKernel::new (pagerank.rs:52-55) entirely. Its only other callers are benches/kernel_bench.rs:25 and two unit tests, all of which become try_new(..).unwrap(). The duplicate is also the usual drift bug: add a third constraint to try_new and the C-ABI quietly stops enforcing it.

[bitflicker64]

⚠️ DifferentialTolerance is a fieldless struct whose only job is to be a namespace for two functions, and the module is already that namespace. grep -rn 'DifferentialTolerance' computer-rust/ returns the declaration, the impl, and six calls, all six inside this file's own #[cfg(test)] block. Nothing in kernel/, ffi/, or benches/ touches it.

Not asking you to drop the file, since #355 lists correctness fixtures as a work item. Just drop the wrapper:

pub fn l1_distance(actual: &[f64], expected: &[f64]) -> Result<f64, String> { .. }
pub fn assert_parity(actual: &[f64], expected: &[f64], epsilon: f64) -> Result<(), String> { .. }

Callers become tolerance::assert_parity(..), which reads better than DifferentialTolerance::assert_parity(..) anyway. While you are in here: assert_parity (lines 48-60) re-implements the length-mismatch check and the per-element finite check from l1_distance (lines 21-33), copied format! strings and all. Have one call into the other's guard instead of carrying a second copy.

[bitflicker64]

⚠️ This is not Zachary's karate club. The real graph has 34 vertices and 78 edges; this list has 35, and every one of them has source 0, 1, 2, or 3, stopping part-way through vertex 3's row. Counting at head, the vertices appearing at either endpoint are {0..13, 17, 19, 21, 27, 28, 30, 31, 32}, so twelve vertices have no incident edge, including vertex 33, which is one of the two hubs the dataset is known for.

That matters given what the fixture is for. #355 requires differential parity against ground truth, and docs/rust-modernization-roadmap.md in this PR promises L1 distance <= 1e-6 against ground-truth outputs. A parity test comparing this kernel's PageRank against a Java or NetworkX run on the real karate club will disagree on every vertex, and whoever hits it first will go hunting for a bug in the kernel.

Either paste the full 78-edge list, or rename to something that does not claim to be the canonical dataset (karate_club_subset, or just fold it into synthetic_*).

[bitflicker64]

🧹 Three dead public symbols, each verified by grep at head as returning only its own definition. dead_code will not warn on any of them because they are pub in a library crate, so CI stays green while they rot.

• Edge (this line): exactly one occurrence in the crate. out_edges returns (&[u32], &[f64]) and never constructs it.
• CsrGraph::new (lines 33-41): zero call sites. from_edges is the only constructor anything uses and builds row_offsets itself.
• GraphFixture.name (fixtures/dataset.rs:21): written by both constructors, including a format! allocation in synthetic_powerlaw, never read.

Deleting all three is a smaller crate with no behaviour change. Add them back in the PR that has a caller.

[bitflicker64]

Blocking: no. Summary: The Rust slice is well isolated from the Java and Go runtime paths, but the C-ABI boundary is not sound on the Rust side yet: the four pointer-taking exports are safe extern "C" fn that dereference caller-supplied pointers behind a module-wide clippy::not_unsafe_ptr_arg_deref allow, and nothing bounds num_vertices, so CsrGraph::from_edges can be asked for tens of GiB and wraps outright at u32::MAX. Three smaller items follow: the exported RUST_KERNEL_VERSION hand-copies the crate version, libc is declared but never used, and .licenserc.yaml gains a Cargo.lock exclusion for a lockfile that is never committed. Evidence: sources read at head c1fc10a; pub extern "C" fn at c_api.rs:35,45,69,80,117,128 with raw-pointer derefs at 54,73,91,111,120 under the allow at c_api.rs:18; Cargo.toml:39-43 sets no overflow-checks while Cargo.toml:43 sets panic = "abort"; git grep -n libc c1fc10a -- computer-rust matches only Cargo.toml:30; git ls-tree -r c1fc10a -- computer-rust lists twelve files and no Cargo.lock. Findings already raised on this head are omitted. Two caveats: no Rust toolchain was available here, so the crate was not compiled and the new gates were not run locally; and gh api repos/apache/hugegraph-computer/commits/c1fc10a/check-runs returns total_count: 0 with a pending combined status, so the Rust CI workflow this PR adds has never executed against its own code.

[bitflicker64]

⚠️ This module-wide allow silences the lint that is correctly reporting an unsound boundary. computer_graph_add_edge (45), computer_graph_finalize (69), computer_graph_compute_pagerank (80) and computer_graph_free (117) are declared pub extern "C" fn, not pub unsafe extern "C" fn, yet each dereferences a pointer the caller supplies: &mut *handle (54, 73), &*handle (91), slice::from_raw_parts_mut (111), Box::from_raw (120).

The null checks do not recover safety. computer_graph_free(0x1 as *mut GraphBuilder) passes !handle.is_null() and reaches Box::from_raw on a bogus address. Because these functions are safe, no call site needs an unsafe block to do that, and the crate's own tests (143-158, 169-180, 186-203) already call them from safe Rust today.

Please declare those four pub unsafe extern "C" fn and drop this allow. The exported symbols and the C ABI are unchanged, since unsafe only constrains Rust callers. The same change needs unsafe blocks around the three test call sites above.

[bitflicker64]

⚠️ Nothing bounds num_vertices on the way in, and from_edges allocates 8 * num_vertices bytes three times over: degree (43), row_offsets (this line), and the current_pos clone (58). computer_graph_create (ffi/c_api.rs:35) validates nothing and computer_graph_finalize (ffi/c_api.rs:74) calls straight in, so computer_graph_create(1 << 31) followed by a finalize with zero edges asks for roughly 48 GiB from two C calls.

The sharp edge is at u32::MAX. num_vertices + 1 is computed in u32 before the widening cast (same pattern at line 36), and [profile.release] (Cargo.toml:39-43) never sets overflow-checks, so release keeps the wrapping default and this becomes vec![0; 0]. Line 52 then panics with index out of bounds on the first iteration, and panic = "abort" at Cargo.toml:43 turns that into a dead host process rather than a return code. This is a separate path from the PageRankKernel::new panic already raised at ffi/c_api.rs:108.

Please use num_vertices as usize + 1 at lines 36 and 50, and bound num_vertices in computer_graph_create by returning NULL. Neither computer_rust_c_api.h:34 nor docs/rust-modernization-roadmap.md:71 states a limit today.

[bitflicker64]

🧹 This hand-copies version = "1.5.0" from Cargo.toml:18, and it is the string that computer_kernel_version() (src/ffi/c_api.rs:128) exports across the C ABI. docs/rust-modernization-roadmap.md:77 documents that export as part of the stable boundary, and the Phase 3 JNI and CGO hosts (lines 33, 91-92) are the consumers. A routine version bump touches Cargo.toml only, so the exported version goes stale silently. The test at c_api.rs:215 cannot catch the drift either: it asserts the exported string equals this same constant.

Please derive it from the manifest:

pub const RUST_KERNEL_VERSION: &str = env!("CARGO_PKG_VERSION");

[bitflicker64]

🧹 libc is declared but never used. git grep -n libc c1fc10a -- computer-rust returns this line and nothing else; the FFI layer uses std::ffi::CString, std::os::raw::c_char and std::slice (src/ffi/c_api.rs:23-25). Neither cargo build nor the new cargo clippy --all-targets -- -D warnings gate reports an unused dependency, so this will not surface on its own.

Please drop it, or switch c_api.rs to libc::c_char if a future no_std path is the intent. Declaring it while using std::os::raw gets the cost without the benefit.

[bitflicker64]

🧹 This exclusion is added for a file the PR never commits. git ls-tree -r --name-only c1fc10a -- computer-rust lists twelve files and no Cargo.lock, and .gitignore has no Cargo entry, so the entry is dead config today and the crate builds unpinned.

That matters for the new gate. .github/workflows/rust-ci.yml:69 runs cargo clippy --all-targets -- -D warnings on a floating dtolnay/rust-toolchain@stable against freshly resolved dependencies, so a new clippy lint or an upstream minor release turns the gate red on the next unrelated Rust PR, with no lockfile to bisect against. The cache key hashFiles('computer-rust/Cargo.toml') (rust-ci.yml:62) cannot see resolution changes that a lockfile would capture either.

Please commit computer-rust/Cargo.lock, which is what this exclusion and the neighbouring committed go.mod/go.sum entries imply was intended. A rust-toolchain.toml would pin the other half.