perf(db): migrate Repository read queries to NativeDatabase rusqlite (6.14)#671
perf(db): migrate Repository read queries to NativeDatabase rusqlite (6.14)#671carlos-alm merged 5 commits intomainfrom
Conversation
…(6.14) Implement all 40 Repository read methods as Rust napi-rs methods on NativeDatabase so every query runs via rusqlite when the native engine is available. openRepo() now returns NativeRepository (delegating to NativeDatabase.openReadonly) with automatic fallback to SqliteRepository. Rust side: read_types.rs defines 14 napi return-type structs, read_queries.rs implements 40 query methods including BFS for getClassHierarchy and dynamic SQL for triage/fan-in queries. TypeScript side: NativeRepository maps napi camelCase rows back to the snake_case field names the Repository interface expects.
|
Claude finished @carlos-alm's task —— View job PR Review Task List
Root Cause AnalysisProblem Being Solved: Performance optimization by migrating all 40 Repository read queries from JavaScript/better-sqlite3 to native Rust/rusqlite via napi-rs, reducing JS-Rust boundary crossings. Why This Approach: Dual-engine architecture requires parity. Native queries run via optimized rusqlite when available, with automatic fallback to better-sqlite3. Risk Assessment: Medium - This is a significant change (1993 lines added) that reimplements core database operations in Rust. Risk mitigated by comprehensive fallback mechanisms. Backlog Compliance
Critical ConcernsNone identified. This is a well-implemented performance optimization with proper error handling, security measures, and backwards compatibility. Code Quality Highlights:
Final Recommendation
|
Greptile SummaryThis PR completes Phase 6.14 by implementing all 40 Key changes:
The main finding in this review is a consistency issue in Confidence Score: 5/5Safe to merge — all findings are P2 style/defensive concerns with no correctness impact on the normal execution path. The prior P0 concern (error-swallowing in has_cfg_tables/has_embeddings/has_dataflow_table) is resolved. The two remaining findings are P2: a silent-error pattern in the BFS that would only affect results under a rare low-level I/O failure, and a style inconsistency in get_callable_nodes. All parameterized queries are correctly bound, the escape_like helper mirrors the TypeScript original, and the camelCase→snake_case mapping in NativeRepository is thorough and accurate. crates/codegraph-core/src/read_queries.rs — specifically get_class_hierarchy (line 904) and get_callable_nodes (line 1014). Important Files Changed
Sequence DiagramsequenceDiagram
participant TS as TypeScript caller
participant OR as openRepo()
participant NR as NativeRepository
participant ND as NativeDatabase (Rust)
participant RQ as rusqlite
participant FB as SqliteRepository (fallback)
TS->>OR: openRepo(dbPath?)
OR->>OR: isNativeAvailable()?
alt native available
OR->>ND: NativeDatabase.openReadonly(dbPath)
ND->>RQ: open readonly connection
OR->>NR: new NativeRepository(ndb)
OR-->>TS: { repo: NativeRepository, close() }
TS->>NR: repo.findCallees(nodeId)
NR->>ND: ndb.findCallees(nodeId)
ND->>RQ: prepare_cached + query_map
RQ-->>ND: Vec<NativeRelatedNodeRow>
ND-->>NR: camelCase JS objects
NR->>NR: toRelatedNodeRow() (camelCase to snake_case)
NR-->>TS: RelatedNodeRow[]
else native unavailable or engine error (non-DbError)
OR->>FB: new SqliteRepository(better-sqlite3 db)
OR-->>TS: { repo: SqliteRepository, close() }
end
Reviews (2): Last reviewed commit: "fix: distinguish table-not-found from I/..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| } | ||
| } | ||
|
|
||
| /// Get complexity metrics for a node. | ||
| #[napi] | ||
| pub fn get_complexity_for_node( | ||
| &self, | ||
| node_id: i32, |
There was a problem hiding this comment.
has_cfg_tables swallows all errors indiscriminately
The current implementation catches any rusqlite error (including I/O errors or a closed connection) and silently returns false. A real connection error would be misreported as "CFG tables don't exist". The same applies to has_embeddings and has_dataflow_table. Consider distinguishing between "table doesn't exist" and genuine I/O failures:
pub fn has_cfg_tables(&self) -> napi::Result<bool> {
let conn = self.conn()?;
match conn.prepare("SELECT 1 FROM cfg_blocks LIMIT 0") {
Ok(_) => Ok(true),
Err(rusqlite::Error::SqliteFailure(_, _)) => Ok(false),
Err(e) => Err(napi::Error::from_reason(format!("has_cfg_tables: {e}"))),
}
}There was a problem hiding this comment.
Fixed in 3d337b7 — has_cfg_tables, has_embeddings, and has_dataflow_table now match on rusqlite::Error::SqliteFailure (and QueryReturnedNoRows for embeddings) and return false only for those. Genuine I/O or connection errors are propagated as napi errors.
…ow, NativeAdjacentEdgeRow (#671) The nodes table allows NULL for line, but these three Rust structs declared line as non-optional i32. Any node with a NULL line (e.g. file-kind nodes) would trigger a rusqlite type-mismatch error. Change to Option<i32> in Rust and number | null in TypeScript, with ?? 0 fallback in the converter functions to match existing Repository interface contracts.
Re-throw DbError (e.g. DB not found) instead of silently falling back to better-sqlite3, which would produce a misleading debug message. Also wrap the ndb setup block in try/catch to close the handle if an exception occurs after openReadonly, preventing resource leaks.
has_cfg_tables, has_embeddings, and has_dataflow_table previously caught all rusqlite errors and returned false. Now only SqliteFailure (and QueryReturnedNoRows for embeddings) map to false — genuine I/O or connection errors propagate as napi errors.
|
Addressed all Greptile review findings: P1: Non-optional P1: DbError for missing DB silently swallowed by native catch block — Fixed in 3418539. The catch block now re-throws P2: P2: |
|
Claude finished @carlos-alm's task —— View job Review of Greptile Finding Fixes ✅Task: Review the fixes applied to address all Greptile review findings Todo List
✅ All Greptile Findings Successfully AddressedP1 Fix Verified: ✅ Non-optional line fields corrected
P1 Fix Verified: ✅ DbError handling corrected
P2 Fix Verified: ✅ Resource leak prevented
P2 Fix Verified: ✅ Table check error handling improved
🎯 AssessmentStatus: All critical review findings have been comprehensively addressed. The implementation now properly handles:
The PR is ready for merge once CI confirms Rust compilation succeeds. |
The nodes table allows NULL for line, but these three Rust structs declared line as non-optional i32. Any node with a NULL line (e.g. file-kind nodes) would trigger a rusqlite type-mismatch error. Change to Option<i32> in Rust and number | null in TypeScript, with ?? 0 fallback in the converter functions to match existing Repository interface contracts.
Re-throw DbError (e.g. DB not found) instead of silently falling back to better-sqlite3, which would produce a misleading debug message. Also wrap the ndb setup block in try/catch to close the handle if an exception occurs after openReadonly, preventing resource leaks.
has_cfg_tables, has_embeddings, and has_dataflow_table previously caught all rusqlite errors and returned false. Now only SqliteFailure (and QueryReturnedNoRows for embeddings) map to false — genuine I/O or connection errors propagate as napi errors.
carlos-alm
force-pushed
the
feat/native-read-queries
branch
from
3d337b7 to
0269b08
Compare
1 participant
Summary
NativeDatabase, so every query runs viarusqlitewhen the native engine is availableopenRepo()now returnsNativeRepository(delegating toNativeDatabase.openReadonly) with automatic fallback toSqliteRepositorywhen native is unavailableread_types.rsdefines 14 napi return-type structs;read_queries.rsimplements 40 query methods including BFS forgetClassHierarchyand dynamic SQL for triage/fan-in queriesNativeRepositorymaps napi camelCase rows back to the snake_case field names the Repository interface expectsTest plan
tsc --noEmit)codegraph build && codegraph statsproduces identical results on both engines