-
-
Notifications
You must be signed in to change notification settings - Fork 148
feat(hql + core): f32 and f16 vector precision #655
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
base: dev
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additional Comments (5)
-
helix-db/src/helixc/parser/utils.rs, line 46 (link)style: Remove debug println statement before merging
-
helix-db/src/helix_engine/tests/traversal_tests/vector_traversal_tests.rs, line 162 (link)logic: This brute_force_search_v call still uses raw array - should be updated to use VectorData for consistency
-
helix-db/src/helix_engine/vector_core/vector_distance.rs, line 184 (link)style: Debug println statement should be removed or wrapped in debug_assert! for production code
-
helix-db/src/helixc/parser/schema_parse_methods.rs, line 241-260 (link)logic: Missing F16 case in default value parsing for float literals
-
helix-db/src/helixc/analyzer/types.rs, line 312-329 (link)logic: The
is_numeric()method is missing F16 in its pattern matching. This could cause type validation issues when F16 values are used in numeric contexts.
38 files reviewed, 10 comments
| let original = HVector::new(VectorData::F64(vec![1.0, 2.0, 3.0])); | ||
| let bytes = original.to_bytes(); | ||
| let reconstructed = HVector::from_bytes(original.get_id(), 0, &bytes).unwrap(); | ||
| assert_eq!(original.get_data(), reconstructed.get_data()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: This test validates the new serialization format with type markers but only tests f64. Consider adding tests for f16/f32 precision roundtrip to ensure the new functionality works correctly.
Prompt To Fix With AI
This is a comment left during a code review.
Path: helix-db/src/helix_engine/tests/vector_tests.rs
Line: 46:49
Comment:
**style:** This test validates the new serialization format with type markers but only tests f64. Consider adding tests for f16/f32 precision roundtrip to ensure the new functionality works correctly.
How can I resolve this? If you propose a fix, please make it concise.| } | ||
| let mut vec = Vec::with_capacity(data_bytes.len() / std::mem::size_of::<f64>()); | ||
| for chunk in data_bytes.chunks_exact(std::mem::size_of::<f64>()) { | ||
| let value = f64::from_be_bytes(chunk.try_into().expect("Invalid chunk")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: Using expect() here could cause panics on corrupted data. Consider returning VectorError instead for better error handling.
Prompt To Fix With AI
This is a comment left during a code review.
Path: helix-db/src/helix_engine/vector_core/vector.rs
Line: 218:218
Comment:
**style:** Using expect() here could cause panics on corrupted data. Consider returning VectorError instead for better error handling.
How can I resolve this? If you propose a fix, please make it concise.| let d = cosine_similarity(v.get_data(), query).unwrap(); | ||
| let v_data = v.get_data(); | ||
| let d = | ||
| cosine_similarity_mixed(v_data, &VectorData::F64(query.to_vec())).unwrap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: Converting query to VectorData::F64 creates unnecessary allocations for each vector comparison. Consider passing the query slice directly or caching the VectorData wrapper.
Prompt To Fix With AI
This is a comment left during a code review.
Path: helix-db/src/helix_engine/traversal_core/ops/vectors/brute_force_search.rs
Line: 64:64
Comment:
**style:** Converting query to VectorData::F64 creates unnecessary allocations for each vector comparison. Consider passing the query slice directly or caching the VectorData wrapper.
How can I resolve this? If you propose a fix, please make it concise.## 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 -->
Description
Related Issues
Closes #650
Checklist when merging to main
rustfmthelix-cli/Cargo.tomlandhelixdb/Cargo.tomlAdditional Notes
Greptile Overview
Updated On: 2025-10-08 17:38:49 UTC
Summary
This PR implements comprehensive support for multiple floating-point precisions (f16, f32, f64) in HelixDB's vector operations, addressing issue #650. The changes introduce a new `VectorData` enum that wraps different precision types, enabling more memory-efficient vector storage and operations while maintaining backward compatibility with existing f64 vectors.Key architectural changes include:
VectorDataenum: Provides type-safe abstraction over f64, f32, and f16 vectors with proper serialization supportV::MyVector<F32>for vector definitionsThe implementation follows a systematic approach, updating every layer from the grammar parser through to the storage engine. The
VectorDataenum serves as the central abstraction, allowing the same APIs to work with different precision types while enabling precision-specific optimizations for distance calculations.PR Description Notes:
VectorDataenum or the comprehensive nature of the changes across multiple system layersImportant Files Changed
Changed Files
helix-db/src/helix_engine/vector_core/vector_data.rshelix-db/src/helix_engine/vector_core/vector.rshelix-db/src/helix_engine/vector_core/vector_distance.rshelix-db/src/grammar.pesthelix-db/src/helixc/parser/types.rshelix-db/src/helixc/analyzer/methods/traversal_validation.rshelix-db/src/helixc/generator/utils.rshelix-db/src/helix_engine/vector_core/hnsw.rshelix-db/src/helix_engine/vector_core/vector_core.rshelix-db/src/helix_engine/traversal_core/ops/vectors/search.rshelix-db/src/helix_engine/traversal_core/ops/vectors/insert.rshelix-db/src/helixc/parser/schema_parse_methods.rshelix-db/src/helixc/analyzer/types.rshelix-db/Cargo.tomlhelix-container/Cargo.tomlhql-tests/tests/vector_precision/schema.hxhql-tests/tests/vector_precision/queries.hxhql-tests/tests/vector_precision/config.hx.jsonhql-tests/tests/vector_precision/helix.tomlhelix-db/src/helix_gateway/mcp/tools.rshelix-db/src/helix_engine/tests/traversal_tests/util_tests.rshelix-db/src/protocol/value.rshelix-db/src/helix_engine/bm25/bm25.rshelix-db/src/helixc/parser/utils.rshelix-db/src/helix_engine/tests/traversal_tests/edge_traversal_tests.rshelix-db/src/helix_engine/tests/traversal_tests/drop_tests.rshelix-db/src/helix_engine/tests/traversal_tests/vector_traversal_tests.rshelix-db/src/helix_engine/bm25/bm25_tests.rshelix-db/src/helix_gateway/mcp/tools_tests.rshelix-db/src/helixc/generator/traversal_steps.rshelix-db/src/helix_engine/tests/hnsw_tests.rshelix-db/src/helix_engine/tests/vector_tests.rshelix-db/src/helixc/analyzer/methods/infer_expr_type.rshelix-db/src/helixc/analyzer/methods/graph_step_validation.rshelix-db/benches/hnsw_benches.rshelix-db/src/helixc/generator/source_steps.rshelix-db/src/helix_engine/vector_core/mod.rshelix-db/src/helix_engine/traversal_core/ops/vectors/brute_force_search.rsSequence Diagram
sequenceDiagram participant User participant Parser as "HQL Parser" participant Analyzer as "Semantic Analyzer" participant Generator as "Code Generator" participant VectorCore as "Vector Core" participant Storage as "HNSW Storage" User->>Parser: "V::Embedding<F16> { content: String }" Parser->>Parser: "Parse precision specification" Parser->>Analyzer: "VectorSchema with Precision::F16" Analyzer->>Analyzer: "Validate precision type" Analyzer->>Generator: "Generate VectorData::F16 operations" User->>Parser: "AddV<Embedding>(vector, {content: content})" Parser->>Analyzer: "Vector creation with F16 data" Analyzer->>Generator: "Generate F16-specific VectorData" Generator->>VectorCore: "VectorData::F16(vec)" VectorCore->>VectorCore: "Create HVector with F16 precision" VectorCore->>Storage: "Store vector with type marker" Storage-->>VectorCore: "Vector stored with F16 metadata" User->>Parser: "SearchV<Embedding>(query_vector, 10)" Parser->>Analyzer: "Vector search operation" Analyzer->>Generator: "Generate F16 search operation" Generator->>VectorCore: "search() with VectorData::F16" VectorCore->>VectorCore: "Perform cosine similarity with F16" Note over VectorCore: "Uses native F16 arithmetic or converts to F32 for calculations" VectorCore->>Storage: "Query HNSW index" Storage-->>VectorCore: "Return matching F16 vectors" VectorCore-->>Generator: "Search results" Generator-->>User: "Vector search results"