imbajin commented on code in PR #360: URL: https://github.com/apache/hugegraph-computer/pull/360#discussion_r3768831301
########## computer-rust/src/ffi/c_api.rs: ########## @@ -0,0 +1,175 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You me obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +use crate::kernel::csr::CsrGraph; +use crate::kernel::pagerank::PageRankKernel; +use crate::kernel::sssp::SsspKernel; +use crate::RUST_KERNEL_VERSION; +use std::ffi::CString; +use std::os::raw::c_char; +use std::ptr; Review Comment: ⚠️ important — `std::ptr` is unused. The added Rust workflow runs `cargo clippy --all-targets -- -D warnings`, so this import is promoted to an error and prevents the Rust CI job from reaching its tests. Remove the import or use it deliberately. ########## computer-rust/src/kernel/csr.rs: ########## @@ -0,0 +1,123 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#[derive(Debug, Clone, Default)] +pub struct Edge { + pub target: u32, + pub weight: f64, +} + +#[derive(Debug, Clone)] +pub struct CsrGraph { + num_vertices: u32, + row_offsets: Vec<usize>, + column_indices: Vec<u32>, + edge_weights: Vec<f64>, +} + +impl CsrGraph { + pub fn new(num_vertices: u32) -> Self { + Self { + num_vertices, + row_offsets: vec![0; (num_vertices + 1) as usize], + column_indices: Vec::new(), + edge_weights: Vec::new(), + } + } + + pub fn from_edges(num_vertices: u32, edges: &[(u32, u32, f64)]) -> Self { + let mut degree = vec![0; num_vertices as usize]; + for &(src, _dst, _weight) in edges { + if src < num_vertices { Review Comment: ⚠️ important — The degree pass counts every edge with a valid source, while the fill pass skips destinations outside `num_vertices`. For an edge such as `(1, 99, 1.0)`, `row_offsets` reserves a slot that remains the default `(target=0, weight=0)`, so PageRank and SSSP process a fabricated edge. Count only edges with both endpoints valid, or reject invalid endpoints at the API boundary. ########## computer-rust/src/ffi/c_api.rs: ########## @@ -0,0 +1,175 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You me obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +use crate::kernel::csr::CsrGraph; +use crate::kernel::pagerank::PageRankKernel; +use crate::kernel::sssp::SsspKernel; +use crate::RUST_KERNEL_VERSION; +use std::ffi::CString; +use std::os::raw::c_char; +use std::ptr; +use std::slice; + +pub struct GraphBuilder { + num_vertices: u32, + edges: Vec<(u32, u32, f64)>, + csr: Option<CsrGraph>, +} + +#[no_mangle] +pub extern "C" fn computer_graph_create(num_vertices: u32) -> *mut GraphBuilder { + let builder = Box::new(GraphBuilder { + num_vertices, + edges: Vec::new(), + csr: None, + }); + Box::into_raw(builder) +} + +#[no_mangle] +pub extern "C" fn computer_graph_add_edge( + handle: *mut GraphBuilder, + src: u32, + dst: u32, + weight: f64, +) -> i32 { + if handle.is_null() { + return -1; + } + let builder = unsafe { &mut *handle }; + builder.edges.push((src, dst, weight)); Review Comment: ⚠️ important — `computer_graph_add_edge` accepts arbitrary `f64` weights, but `SsspKernel` uses Dijkstra. Negative weights can return incorrect shortest paths and a reachable negative cycle can keep lowering distances and growing the heap; non-finite weights are also unbounded input. Reject non-finite and negative weights here, or change the algorithm and document the supported weight domain. ########## vermeer/apps/compute/rust_bridge.go: ########## @@ -0,0 +1,107 @@ +/* +Licensed to the Apache Software Foundation (ASF) under one or more +contributor license agreements. See the NOTICE file distributed with this +work for additional information regarding copyright ownership. The ASF +licenses this file to You under the Apache License, Version 2.0 (the +"License"); you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +License for the specific language governing permissions and limitations +under the License. +*/ + +package compute + +import ( + "fmt" + "math" +) + +// RustKernelBridge manages interaction with high-performance Rust computing kernels. +type RustKernelBridge struct { + available bool + version string +} + +func NewRustKernelBridge() *RustKernelBridge { + return &RustKernelBridge{ + available: false, Review Comment: ⚠️ important — `available` is hard-coded to `false`, and this package contains no cgo declaration, library loading, or Rust C-ABI call. Vermeer therefore always executes the Go fallback even when the Rust library is deployed, making the new native bridge unreachable. Implement initialization and native calls, or remove the native-bridge claim and keep this as an explicitly fallback-only implementation. ########## computer/computer-core/src/main/java/org/apache/hugegraph/computer/core/rust/RustKernelBridge.java: ########## @@ -0,0 +1,120 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.hugegraph.computer.core.rust; + +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +public class RustKernelBridge { + + private static final Logger LOG = LoggerFactory.getLogger(RustKernelBridge.class); + private static final boolean NATIVE_AVAILABLE; + private static final String LIB_NAME = "hugegraph_computer_rust"; + + static { + boolean loaded = false; + try { + System.loadLibrary(LIB_NAME); + loaded = true; + LOG.info("Successfully loaded Rust graph computing native library: {}", LIB_NAME); + } catch (UnsatisfiedLinkError e) { + LOG.info("Native library '{}' not available on system PATH; using pure Java fallback", + LIB_NAME); + } catch (Throwable t) { + LOG.warn("Failed to load native Rust graph computing library: {}", t.getMessage()); + } + NATIVE_AVAILABLE = loaded; + } + + public static boolean isAvailable() { + return NATIVE_AVAILABLE; + } + + public static String getVersion() { + if (NATIVE_AVAILABLE) { + try { + return nativeGetVersion(); + } catch (Throwable t) { + LOG.warn("Error calling nativeGetVersion: {}", t.getMessage()); + } + } + return "1.5.0-java-fallback"; + } + + public static double[] computePageRank(double[][] adjMatrix, double dampingFactor, + int maxIterations, double tolerance) { + if (adjMatrix == null || adjMatrix.length == 0) { + return new double[0]; + } + + int n = adjMatrix.length; + double[] ranks = new double[n]; + double initialRank = 1.0 / n; + for (int i = 0; i < n; i++) { + ranks[i] = initialRank; + } + + double[] nextRanks = new double[n]; + double teleport = (1.0 - dampingFactor) / n; + + for (int iter = 0; iter < maxIterations; iter++) { + java.util.Arrays.fill(nextRanks, 0.0); + double danglingSum = 0.0; + + for (int i = 0; i < n; i++) { + int outDegree = 0; + for (int j = 0; j < n; j++) { + if (adjMatrix[i][j] > 0.0) { + outDegree++; + } + } + + if (outDegree == 0) { + danglingSum += ranks[i]; + } else { + double share = ranks[i] / outDegree; + for (int j = 0; j < n; j++) { + if (adjMatrix[i][j] > 0.0) { + nextRanks[j] += share; + } + } + } + } + + double danglingShare = dampingFactor * (danglingSum / n); + double maxDiff = 0.0; + + for (int i = 0; i < n; i++) { + double newRank = teleport + danglingShare + dampingFactor * nextRanks[i]; + double diff = Math.abs(newRank - ranks[i]); + if (diff > maxDiff) { + maxDiff = diff; + } + ranks[i] = newRank; + } + + if (maxDiff < tolerance) { + break; + } + } + + return ranks; + } + + private static native String nativeGetVersion(); Review Comment: ⚠️ important — This Java native declaration does not match the library's exported ABI: Rust exports the C function `computer_kernel_version`, but no JNI symbol for `nativeGetVersion`, and `computePageRank` never calls a native function. If the library loads, `isAvailable()` becomes true while version lookup falls back after `UnsatisfiedLinkError` and computation still runs in Java. Add JNI/C-ABI bindings for the required calls and make availability reflect callable symbols. ########## .github/workflows/release-notes.yml: ########## @@ -0,0 +1,40 @@ +# +# Licensed to the Apache Software Foundation (ASF) under one or more +# contributor license agreements. See the NOTICE file distributed with +# this work for additional information regarding copyright ownership. +# The ASF licenses this file to You under the Apache License, Version 2.0 +# (the "License"); you may not use this file except in compliance with +# the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +# +name: "Release Notes Generator" + +on: + push: + tags: + - 'v*' Review Comment: ⚠️ important — This workflow triggers only for `v*` tags, while the repository's existing release tags use the `1.0.0`/`1.5.0`/`1.7.0` form. A normal version tag will not run this workflow and will not produce draft release notes. Align the trigger with the repository's release convention or update the release process consistently. ########## .github/workflows/rust-ci.yml: ########## @@ -0,0 +1,75 @@ +# +# Licensed to the Apache Software Foundation (ASF) under one or more +# contributor license agreements. See the NOTICE file distributed with +# this work for additional information regarding copyright ownership. +# The ASF licenses this file to You under the Apache License, Version 2.0 +# (the "License"); you may not use this file except in compliance with +# the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +# +name: "Rust CI" + +on: + push: + branches: + - master + - /^release-.*$/ Review Comment: ⚠️ important — GitHub Actions branch filters use glob patterns, not regular expressions. `^/release-.*$/` therefore does not match normal `release-*` branches, so Rust CI is skipped for release-branch pushes. Replace it with a supported glob such as `release-*` and verify the trigger. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
