Skip to content

Conversation

@ishaksebsib
Copy link
Contributor

@ishaksebsib ishaksebsib commented Oct 21, 2025

Description

This PR fixes a compilation error in plural AddE queries within FOR loops, where ID variables are references.

Problem

Queries like:

QUERY AddEdges (edges: [{from_id: ID, to_id: ID, relationship_name: String, properties: String}]) =>
	FOR {from_id, to_id, relationship_name, properties} IN edges {
		AddE<MyEdge>({
			relationship_name: relationship_name,
			properties: properties
		})::From(from_id)::To(to_id)
	}
	RETURN "Edges are added successfully"

failed with "no method named id found for &ID" because the generator produced from_id.id(), but ID (references from iterating &data.edges) has no .id() method, and references don't dereference automatically here.

Solution

Added .id() method to the ID struct in utils/id.rs.

Now both work seamlessly:

  • ID.id() returns u128
  • &ID.id() works through Rust's auto-deref mechanism

This allows the code generator to use .id() consistently for AddE identifier references in FOR loops.

Related Issues

None

Checklist when merging to main

  • No compiler warnings (if applicable)
  • Code is formatted with rustfmt
  • No useless or dead code (if applicable)
  • Code is easy to understand
  • Doc comments are used for all functions, enums, structs, and fields (where appropriate)
  • All tests pass
  • Performance has not regressed (assuming change was not to fix a bug)
  • Version number has been updated in helix-cli/Cargo.toml and helixdb/Cargo.toml

Additional Notes

None

Greptile Overview

Updated On: 2025-10-26 13:17:58 UTC

Greptile Summary

This PR elegantly fixes a compilation error where AddE queries with borrowed ID references in FOR loops failed because &ID didn't have an .id() method.

Key Changes:

  • Added .id() method to the ID struct in utils/id.rs that returns u128
  • Leverages Rust's auto-deref mechanism so both ID.id() and &ID.id() work seamlessly
  • This allows the code generator to consistently use .id() for all ID accessor patterns
  • Minor formatting cleanup in test code (whitespace fixes)

Solution Quality:
This is a cleaner approach compared to the previous attempt (commit 95e1a17) which changed the generator to use .inner() instead. By adding the .id() method, the fix maintains consistency with the existing .inner() method while providing a more intuitive API that works with both owned and borrowed values through auto-deref.

Impact:
The fix enables plural AddE queries within FOR loops to compile correctly when ID variables are references, as shown in the example query in the PR description.

Important Files Changed

File Analysis

Filename Score Overview
helix-db/src/utils/id.rs 5/5 Added .id() method to ID struct for unified accessor interface, enabling both owned and borrowed IDs to use the same method call. Also includes minor formatting fixes.
helix-db/src/helixc/analyzer/utils.rs 5/5 No functional changes in this file - the PR only adds the .id() method to the ID struct to support the existing code generator that calls .id() on identifier references.

Sequence Diagram

sequenceDiagram
    participant User as User Query
    participant Parser as HQL Parser
    participant Analyzer as Semantic Analyzer
    participant CodeGen as Code Generator
    participant ID as ID Struct
    
    User->>Parser: AddE query with FOR loop
    Note over User: FOR {from_id, to_id} IN edges<br/>AddE::From(from_id)::To(to_id)
    
    Parser->>Analyzer: Parse AddE with identifiers
    Analyzer->>Analyzer: Validate identifiers in scope
    Analyzer->>CodeGen: gen_id_access_or_param(from_id)
    Note over CodeGen: Generates: from_id.id()
    
    CodeGen->>ID: Call .id() on &ID
    Note over ID: Before: &ID had no .id() method<br/>Compiler error: "no method named id"
    
    ID-->>CodeGen: Now: auto-deref to ID.id()
    Note over ID: Rust auto-deref: &ID -> ID<br/>Returns u128
    
    CodeGen-->>Analyzer: GeneratedValue with u128
    Analyzer-->>User: Compiled query code
Loading

@ishaksebsib
Copy link
Contributor Author

@greptile

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

1 file reviewed, no comments

Edit Code Review Agent Settings | Greptile

@ishaksebsib ishaksebsib marked this pull request as ready for review October 26, 2025 13:15
Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

2 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

xav-db added a commit that referenced this pull request Oct 29, 2025
This integrates the fix from PR #667 (#667) which adds an .id() method to the ID struct to resolve compilation errors when using AddE with borrowed IDs in FOR loops.

Changes:
- Added ID.id() method returning u128 in helix-db/src/utils/id.rs
- Added test case for AddE with borrowed IDs from FOR loop destructuring

The helixc code generator consistently uses .id() to access ID values. Before this change, calling .id() on &ID (borrowed ID) would fail. The new method leverages Rust's auto-deref so both ID.id() and &ID.id() work seamlessly.

This fixes queries like:
  FOR {from_id, to_id, ...} IN edges {
    AddE<MyEdge>::From(from_id)::To(to_id)
  }

Co-Authored-By: ishaksebsib <[email protected]>
xav-db added a commit that referenced this pull request Oct 29, 2025
## Summary

This PR integrates the fix from #667 into the arena-implementation
branch. It adds an .id() method to the ID struct to resolve compilation
errors when using AddE with borrowed IDs in FOR loops.

## Changes

- Added ID.id() method in helix-db/src/utils/id.rs returning u128
directly
- Added test case in hql-tests/tests/add_e_borrowed_ids/ validating the
fix

## Technical Details

The helixc code generator consistently generates .id() calls to access
ID values. Before this change, calling .id() on &ID (borrowed ID) would
fail. This fix leverages Rust's auto-deref so both ID.id() and &ID.id()
work seamlessly.

## Testing

✅ cargo check --workspace passes
✅ All 27 ID struct unit tests pass  
✅ New test case compiles successfully

Co-Authored-By: ishaksebsib
@xav-db xav-db mentioned this pull request Oct 29, 2025
8 tasks
xav-db added a commit that referenced this pull request Nov 7, 2025
## Description
<!-- Provide a brief description of the changes in this PR -->

## Related Issues
<!-- Link to any related issues using #issue_number -->

Closes #670 #666 #667 #672  #668 #661 #655 #654 #652 #436 

## Checklist when merging to main
<!-- Mark items with "x" when completed -->

- [ ] No compiler warnings (if applicable)
- [ ] Code is formatted with `rustfmt`
- [ ] No useless or dead code (if applicable)
- [ ] Code is easy to understand
- [ ] Doc comments are used for all functions, enums, structs, and
fields (where appropriate)
- [ ] All tests pass
- [ ] Performance has not regressed (assuming change was not to fix a
bug)
- [ ] Version number has been updated in `helix-cli/Cargo.toml` and
`helixdb/Cargo.toml`

## Additional Notes
<!-- Add any additional information that would be helpful for reviewers
-->

<!-- greptile_comment -->

<h2>Greptile Overview</h2>

Updated On: 2025-11-07 00:19:04 UTC

<h3>Greptile Summary</h3>


This PR implements arena-based memory allocation for graph traversals
and refactors the worker pool's channel selection mechanism.

**Key Changes:**
- **Arena Implementation**: Introduced `'arena` lifetime parameter
throughout traversal operations (`in_e.rs`), replacing owned data with
arena-allocated references for improved memory efficiency
- **Worker Pool Refactor**: Replaced `flume::Selector` with a
parity-based `try_recv()`/`recv()` pattern to handle two channels
(`cont_rx` and `rx`) across multiple worker threads
- **Badge Addition**: Added Manta Graph badge to README

**Issues Found:**
- **Worker Pool Channel Handling**: The new parity-based approach
requires an even number of workers (≥2) and uses non-blocking
`try_recv()` followed by blocking `recv()` on alternating channels.
While this avoids a true busy-wait (since one `recv()` always blocks),
the asymmetry means channels are polled at different frequencies,
potentially causing channel starvation or unfair scheduling compared to
the previous `Selector::wait()` approach.

The arena implementation appears solid and follows Rust lifetime best
practices. The worker pool change seems to be addressing a specific
issue with core affinity (per commit `7437cf0f`), but the trade-off in
channel fairness should be monitored.

<details><summary><h3>Important Files Changed</h3></summary>



File Analysis



| Filename | Score | Overview |
|----------|-------|----------|
| README.md | 5/5 | Added Manta Graph badge to README - cosmetic
documentation change with no functional impact |
| helix-db/src/helix_engine/traversal_core/ops/in_/in_e.rs | 5/5 |
Refactored to use arena-based lifetimes ('arena) instead of owned data,
replacing separate InEdgesIterator struct with inline closures for
better memory management |
| helix-db/src/helix_gateway/worker_pool/mod.rs | 3/5 | Replaced flume
Selector with parity-based try_recv/recv pattern requiring even worker
count, but implementation has potential busy-wait issues that could
cause high CPU usage |

</details>


</details>


<details><summary><h3>Sequence Diagram</h3></summary>

```mermaid
sequenceDiagram
    participant Client
    participant WorkerPool
    participant Worker1 as Worker (parity=true)
    participant Worker2 as Worker (parity=false)
    participant Router
    participant Storage

    Client->>WorkerPool: process(request)
    WorkerPool->>WorkerPool: Send request to req_rx channel
    
    par Worker1 Loop (parity=true)
        loop Every iteration
            Worker1->>Worker1: try_recv(cont_rx) - non-blocking
            alt Continuation available
                Worker1->>Worker1: Execute continuation function
            else Empty
                Worker1->>Worker1: Skip (no busy wait here)
            end
            Worker1->>Worker1: recv(rx) - BLOCKS until request
            alt Request received
                Worker1->>Router: Route request to handler
                Router->>Storage: Execute graph operation
                Storage-->>Router: Return result
                Router-->>Worker1: Response
                Worker1->>WorkerPool: Send response via ret_chan
            end
        end
    end
    
    par Worker2 Loop (parity=false)
        loop Every iteration
            Worker2->>Worker2: try_recv(rx) - non-blocking
            alt Request available
                Worker2->>Router: Route request to handler
                Router->>Storage: Execute graph operation
                Storage-->>Router: Return result
                Router-->>Worker2: Response
                Worker2->>WorkerPool: Send response via ret_chan
            else Empty
                Worker2->>Worker2: Skip (no busy wait here)
            end
            Worker2->>Worker2: recv(cont_rx) - BLOCKS until continuation
            alt Continuation received
                Worker2->>Worker2: Execute continuation function
            end
        end
    end

    WorkerPool-->>Client: Response
```
</details>


<!-- greptile_other_comments_section -->

<!-- /greptile_comment -->
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.

1 participant