Skip to content

HelixGraphEngine refactor #136

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 14 commits into from
Closed

HelixGraphEngine refactor #136

wants to merge 14 commits into from

Conversation

lukasnxyz
Copy link
Contributor

@lukasnxyz lukasnxyz commented Jun 1, 2025

Description

  • Removed need for HelixGraphEngine just directly access everything through HelixGraphStorage. This was old code from like Feb

  • Tested running helix-bench and everything still works

  • versions to 1.0.90

Related Issues

closes #110, closes #103

@lukasnxyz lukasnxyz marked this pull request as ready for review June 2, 2025 20:07
@lukasnxyz lukasnxyz requested review from Copilot and xav-db June 2, 2025 20:07
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors the codebase to eliminate the legacy HelixGraphEngine type and consistently use HelixGraphStorage throughout, plus cleans up various module exports, test code, and build profiles.

  • Replaced all references to HelixGraphEngine with HelixGraphStorage
  • Removed or updated outdated module exports and commented-out tests
  • Simplified error conversions and updated build profile entries

Reviewed Changes

Copilot reviewed 21 out of 21 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
helixdb/src/helixc/generator/generator_types.rs Updated clone target from .storage to the graph itself
helixdb/src/helix_gateway/thread_pool/thread_pool.rs Swapped HelixGraphEngineHelixGraphStorage
helixdb/src/helix_gateway/router/router.rs Swapped HelixGraphEngineHelixGraphStorage
helixdb/src/helix_gateway/gateway.rs Swapped HelixGraphEngineHelixGraphStorage & removed commented tests
helixdb/src/helix_gateway/connection/connection.rs Swapped HelixGraphEngineHelixGraphStorage
helixdb/src/helix_engine/vector_core/vector.rs Tweaked panic message format
helixdb/src/helix_engine/types.rs Consolidated std::io::Error variants and changed imports
helixdb/src/helix_engine/graph_core/ops/vectors/search.rs Streamlined error matching
helixdb/src/helix_engine/graph_core/ops/vectors/insert.rs Streamlined error matching and iterator handling
helixdb/src/helix_engine/graph_core/mod.rs Removed pub mod graph_core
helixdb/Cargo.toml Stripped out individual crate release profiles
helix-container/src/queries.rs New handler functions added under helixdb::{…} imports
helix-container/src/main.rs Swapped to HelixGraphStorage constructor
helix-container/Cargo.toml Commented out per-crate release profile
helix-cli/src/utils.rs Updated query syntax
helix-cli/src/types.rs Removed unused error variant
helix-cli/src/main.rs Renamed unused del_prompt variable
helix-cli/Cargo.toml Commented out per-crate release profile
get_routes/Cargo.toml Added edition = "2021"
Cargo.toml Centralized release profile settings
Comments suppressed due to low confidence (6)

helixdb/src/helix_engine/graph_core/mod.rs:2

  • The public graph_core module was removed, which will break imports of graph_core. Re-add pub mod graph_core; to restore module exports.
pub mod graph_core;

helixdb/src/helix_gateway/gateway.rs:31

  • All gateway tests were commented out. Consider restoring or rewriting these tests to maintain coverage for connection and routing logic.
// #[cfg(test)]

helix-container/src/queries.rs:58

  • [nitpick] Rust naming conventions prefer CamelCase for type names. Rename scan_recordsInput to ScanRecordsInput for consistency.
pub struct scan_recordsInput {

helixdb/src/helix_engine/types.rs:17

  • [nitpick] The Io variant is ambiguous; consider renaming to IoError for clarity.
Io(Error),

helixdb/src/helix_engine/graph_core/ops/vectors/search.rs:56

  • This code calls once(...) but std::iter::once is not imported. Add use std::iter::once; at the top to fix the compilation error.
Err(e) => once(Err(GraphError::from(e))).collect::<Vec<_>>().into_iter()

helix-container/src/queries.rs:70

  • ResponseRemapping is referenced but not imported. Add the proper import (e.g., use helixdb::protocol::remapping::ResponseRemapping;) to resolve this.
let mut remapping_vals: RefCell<HashMap<u128, ResponseRemapping>> = RefCell::new(HashMap::new());

@@ -163,7 +163,7 @@ impl HVector {
let other_len = other.data.len();

if len != other_len {
println!("mis-match in vector dimensions!\n{} != {}", len, other_len);
println!("mis-match in vector dimensions! {} != {}", len, other_len);
Copy link
Preview

Copilot AI Jun 2, 2025

Choose a reason for hiding this comment

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

[nitpick] Using println! for error conditions is not ideal. Consider using eprintln!, a logging macro, or a debug_assert! for mismatched dimensions.

Suggested change
println!("mis-match in vector dimensions! {} != {}", len, other_len);
eprintln!("mis-match in vector dimensions! {} != {}", len, other_len);

Copilot uses AI. Check for mistakes.

@@ -51,8 +57,7 @@ impl ConnectionHandler {

let active_connections = Arc::clone(&self.active_connections);
let thread_pool_sender = self.thread_pool.sender.clone();
let address = self.address.clone();

let _address = self.address.clone(); // TODO: not needed
Copy link
Preview

Copilot AI Jun 2, 2025

Choose a reason for hiding this comment

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

This variable is unused. Remove the line to clean up dead code.

Suggested change
let _address = self.address.clone(); // TODO: not needed

Copilot uses AI. Check for mistakes.

@suxatcode
Copy link

suxatcode commented Jun 8, 2025

What's the current status on this PR here? Will it be merged soon?
TL;DR: For the deterministic simulation engine #146 I see a few conflicts incoming. Would like to resolve those rather sooner than later if this PR is supposed to be merged! :)

WIP here: #146 - I either need to abstract the LMDB directly in front of the heed3-api (replicate that API), or a bit higher level to abstract away also e.g. entity id retrieval, if I go for the later one (which would be a better architecture IMO), then I see a few conflicts incoming, since I need to do quite some refactorings to get rid of all heed3-api calls in favor of a helix_storage module.

@lukasnxyz
Copy link
Contributor Author

Hey @suxatcode, this pr isn't going to be merged, but there a few things in it that I changed that I'm going to push in a different pr soon

@lukasnxyz lukasnxyz closed this Jun 10, 2025
@xav-db xav-db deleted the helixgraphengine-refactor branch June 21, 2025 19:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Task]: Condense HelixGraphEngine to just HelixGraphStorage Move optimization flags to the root Cargo.toml
3 participants