diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 6d95807..46b2983 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -32,7 +32,7 @@ jobs: - name: Run tests run: | - nim c -d:ssl --threads:on --path:src -r tests/test_all.nim > test_output.log 2>&1 + nimble test > test_output.log 2>&1 EXIT=$? tail -n 200 test_output.log exit $EXIT @@ -52,12 +52,6 @@ jobs: - name: Compile benchmarks run: nim c -d:release --threads:on benchmarks/bench_all.nim - - name: Compile stress test - run: nim c -d:ssl --threads:on --path:src tests/stress_test.nim - - - name: Run stress test - run: ./tests/stress_test - - name: Check for unused declarations and imports run: | nim c -d:ssl --threads:on --path:src tests/test_all.nim 2>&1 | tee build.log || true diff --git a/baradadb.nimble b/baradadb.nimble index 2be7758..6b28c75 100644 --- a/baradadb.nimble +++ b/baradadb.nimble @@ -24,7 +24,14 @@ task build_release, "Build release version": exec "nim c -d:release --opt:speed -o:build/baramcp src/baramcp.nim" task test, "Run all tests": - exec "nim c -r tests/test_all.nim" + # Smoke test talks to ./build/baradadb over TCP — build it first. + exec "nim c -o:build/baradadb src/baradadb.nim" + # Quick embedded suites first, heavy fuzz/stress suites last. + for t in ["test_minimal", "test_all", "bugfix_test", "join_tests", "test_lock", + "test_schema_persist", "test_storage_hardening", "tla_faithfulness", + "nimforum_smoke_test", "fuzz_test", "prop_test", + "test_wire_insert_stress", "stress_test"]: + exec "nim c -r tests/" & t & ".nim" task bench, "Run embedded micro-benchmarks (in-process)": exec "nim c -d:release -r benchmarks/bench_all.nim" diff --git a/docs/superpowers/plans/2026-07-30-executor-split.md b/docs/superpowers/plans/2026-07-30-executor-split.md new file mode 100644 index 0000000..25328fa --- /dev/null +++ b/docs/superpowers/plans/2026-07-30-executor-split.md @@ -0,0 +1,566 @@ +# Executor.nim Split Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Split the 5,398-line `src/barabadb/query/executor.nim` monolith into focused, layered modules under `src/barabadb/query/exec/` without changing any behavior or public API. + +**Architecture:** Strictly layered real Nim modules (no circular imports — Nim forbids them). The mutually recursive core (eval ↔ executePlan ↔ dispatcher) is split via two typed proc-var hooks: `eval.executePlanHook` (subqueries) and `triggers.executeQueryHook` (trigger bodies). `executor.nim` becomes the top layer: the `executeQueryImpl` dispatcher + `executeQuery` wrapper, importing and re-exporting everything so existing consumers are untouched. + +**Tech Stack:** Nim 2.2.10, ARC (forced by `nim.cfg`), unittest via `tests/test_all.nim` + full `nimble test`. + +## Global Constraints + +- Build/test command per task: `nim c -d:ssl --threads:on --path:src -o:tests/test_all tests/test_all.nim && ./tests/test_all` (must exit 0; 461+ `[OK]`). +- Final gate (last task only): `nimble test` must exit 0 with 650 `[OK]`. +- Public API freeze: `import barabadb/query/executor` must keep working with every currently exported symbol (`executeQuery`, `executePlan`, `newExecutionContext`, `cloneForConnection`, `evalExpr`, `evalExprOld`, `lowerExpr`, `lowerSelect`, `execInsert`, `execDelete`, `execUpdateRow`, `validateType`, `fireTriggers`, `validateConstraints`, `applyDefaultValues`, `computeWindowValues`, `bindParams`, `extractJoinEquality`, `parseVectorString`). Achieve this with `import exec/x; export x` in `executor.nim` (established pattern, executor.nim:59-64). +- No behavior changes. Pure code motion + import/export plumbing + the two hooks. +- No new dependencies. No `include` files — real modules only. +- Line numbers below are from the pre-split file (5,398 lines). After each extraction they shift — **always relocate procs by name** (`grep -n '^proc name' src/barabadb/query/executor.nim`), never by line number. +- Git commits: only after explicit user confirmation (session rule). Batch `git add` per task, commit when the user approves. + +## Layer map (dependency order, bottom → top) + +``` +L0 exec/types.nim, exec/values.nim, exec/schema.nim (existing, untouched) +L1 exec/context.nim Task 1 — newExecutionContext, cloneForConnection, exprToSql, selectToSql +L1 exec/helpers.nim Task 2 — cmpMax/cmpMin, extractJoinEquality, chooseJoinStrategy, parseVectorString, collectCorrelatedTables* +L1 exec/params.nim Task 3 — doBindParams, bindParams, getSelectColumns, isDDL +L1 exec/migrations.nim Task 4 — migration storage helpers (228–299) +L2 exec/eval.nim Task 5 — evalExpr, evalExprOld, row conversions, hybrid search; hooks: executePlanHook, execScanHook +L3 exec/lower.nim Task 6 — lowerExpr, lowerSelect, evalNodeToString +L4 exec/rls.nim Task 7 — hasPrivilege, passesPolicy, checkInsertPolicy +L5 exec/scan.nim Task 8 — execScan, execPointRead +L6 exec/dml.nim Task 9 — execInsert, execDelete, execUpdateRow +L7 exec/fk.nim Task 10 — enforceFkOn*, findReferencingRows +L8 exec/triggers.nim Task 11 — fireTriggers (hook: executeQueryHook), validateConstraints, applyDefaultValues, validateType +L9 exec/window.nim Task 12 — partitionKey, compareRowsByOrder, resolveFrameBounds, computeWindowValues, expandStarRow +L10 exec/plan_exec.nim Task 13 — executePlan +L11 executor.nim Task 14 — executeQueryImpl dispatcher, executeQuery, executeMigrationSql, hook wiring, re-exports + cleanup + docs Task 15 +``` + +--- + +### Task 1: exec/context.nim + +**Files:** +- Create: `src/barabadb/query/exec/context.nim` +- Modify: `src/barabadb/query/executor.nim` + +**Interfaces:** +- Consumes: `exec/types.nim` (ExecutionContext, ChangeEvent), `exec/values.nim`, `exec/schema.nim`, `query/ast` (Node), `query/lexer`/`query/parser` only if exprToSql needs them (check imports at executor.nim:1-68 and copy the needed ones). +- Produces: `newExecutionContext*(...)` (copy exact signatures from executor.nim:72 and its overloads), `cloneForConnection*(ctx: ExecutionContext): ExecutionContext`, `exprToSql*(...)`, `selectToSql*(...)`. + +- [ ] **Step 1: Move the procs** + +Create `src/barabadb/query/exec/context.nim` starting with the module doc comment, then the imports executor.nim uses that these procs need (from executor.nim:1-68 — copy the import block and trim unused ones at the end of the task), then move, from executor.nim: the forward-decl block lines that belong to these procs, `newExecutionContext` (was ~line 72), `exprToSql`, `selectToSql`, `cloneForConnection` (was ~line 201). Every proc called from outside the module keeps its `*` export marker; private helpers stay private. + +- [ ] **Step 2: Wire executor.nim** + +In executor.nim: delete the moved code; add `import exec/context` + `export context` next to the existing `import exec/types; export types` lines (59-64). Delete now-unneeded forward declarations of the moved procs. + +- [ ] **Step 3: Compile and test** + +Run: `nim c -d:ssl --threads:on --path:src -o:tests/test_all tests/test_all.nim && ./tests/test_all` +Expected: compile clean (fix missing imports/exports until it is), exit 0, 461+ `[OK]`. + +- [ ] **Step 4: Stage for commit** + +```bash +git add src/barabadb/query/exec/context.nim src/barabadb/query/executor.nim +# commit only after user confirmation: git commit -m "refactor(exec): extract context management into exec/context.nim" +``` + +--- + +### Task 2: exec/helpers.nim + +**Files:** +- Create: `src/barabadb/query/exec/helpers.nim` +- Modify: `src/barabadb/query/executor.nim` + +**Interfaces:** +- Consumes: `exec/types`, `exec/values`, `query/ast`, `query/ir` (FromPlan for collectCorrelatedTables), stdlib. +- Produces: `cmpMax`, `cmpMin` (private or exported as currently), `extractJoinEquality*`, `chooseJoinStrategy*`, `parseVectorString*`, `collectCorrelatedTablesFromPlan*` (and any sibling collectCorrelatedTables overloads — keep their current export status). + +- [ ] **Step 1: Move the procs** + +Create `exec/helpers.nim`; move `cmpMax`/`cmpMin` (top of executor.nim, ~65-69) and everything in the 300–436 region: `extractJoinEquality`, `chooseJoinStrategy`, `parseVectorString`, `collectCorrelatedTables*` overloads, plus their forward decls. Copy needed imports (query/ir, query/ast, std/strutils, etc.). + +- [ ] **Step 2: Wire executor.nim** + +Delete moved code; add `import exec/helpers` + `export helpers`. + +- [ ] **Step 3: Compile and test** + +Run: `nim c -d:ssl --threads:on --path:src -o:tests/test_all tests/test_all.nim && ./tests/test_all` +Expected: exit 0, 461+ `[OK]`. + +- [ ] **Step 4: Stage for commit** + +```bash +git add src/barabadb/query/exec/helpers.nim src/barabadb/query/executor.nim +# commit after user confirmation: git commit -m "refactor(exec): extract join/vector helpers into exec/helpers.nim" +``` + +--- + +### Task 3: exec/params.nim + +**Files:** +- Create: `src/barabadb/query/exec/params.nim` +- Modify: `src/barabadb/query/executor.nim` + +**Interfaces:** +- Consumes: `exec/types`, `exec/values`, `exec/context` (`exprToSql` — called by doBindParams, was executor.nim:3878), `query/ast`. +- Produces: `bindParams*`, `getSelectColumns`, `isDDL`, `doBindParams` (private if currently private). + +- [ ] **Step 1: Move the procs** + +Create `exec/params.nim`; move the 3739–3906 region: `doBindParams`, `bindParams`, `getSelectColumns`, `isDDL` (+ related forward decls). Import `exec/context` for `exprToSql`. + +- [ ] **Step 2: Wire executor.nim** + +Delete moved code; add `import exec/params` + `export params`. + +- [ ] **Step 3: Compile and test** + +Run: `nim c -d:ssl --threads:on --path:src -o:tests/test_all tests/test_all.nim && ./tests/test_all` +Expected: exit 0, 461+ `[OK]`. + +- [ ] **Step 4: Stage for commit** + +```bash +git add src/barabadb/query/exec/params.nim src/barabadb/query/executor.nim +# commit after user confirmation: git commit -m "refactor(exec): extract param binding into exec/params.nim" +``` + +--- + +### Task 4: exec/migrations.nim + +**Files:** +- Create: `src/barabadb/query/exec/migrations.nim` +- Modify: `src/barabadb/query/executor.nim` + +**Interfaces:** +- Consumes: `exec/types`, `exec/values`, `storage/lsm`, `checksums/sha2` (computeChecksum — check current import), std/locks or sync primitives as currently used. +- Produces: `acquireMigrationLock`, `releaseMigrationLock`, `isMigrationApplied`, `getMigrationRecord`, `setMigrationRecord`, `computeChecksum`, `getMigrationBody`, `migrationAppliedKey`, `listMigrations` — keep each proc's current export status (they are private today but used by the dispatcher in executor.nim, so they now need `*`; export them but do NOT re-export migrations from executor.nim — dispatcher imports it directly). + +- [ ] **Step 1: Move the procs** + +Create `exec/migrations.nim`; move the 228–299 region (all migration storage helpers + their lock globals if any — check for module-level `var` in that range; there is none per analysis, but verify before moving). + +- [ ] **Step 2: Wire executor.nim** + +Delete moved code; add `import exec/migrations` (NO `export` — internal). + +- [ ] **Step 3: Compile and test** + +Run: `nim c -d:ssl --threads:on --path:src -o:tests/test_all tests/test_all.nim && ./tests/test_all` +Expected: exit 0, 461+ `[OK]`. + +- [ ] **Step 4: Stage for commit** + +```bash +git add src/barabadb/query/exec/migrations.nim src/barabadb/query/executor.nim +# commit after user confirmation: git commit -m "refactor(exec): extract migration storage into exec/migrations.nim" +``` + +--- + +### Task 5: exec/eval.nim (with hybrid search + hooks) + +**Files:** +- Create: `src/barabadb/query/exec/eval.nim` +- Modify: `src/barabadb/query/executor.nim` + +**Interfaces:** +- Consumes: `exec/types`, `exec/values`, `exec/schema`, `exec/helpers` (parseVectorString — called by evalExprOld), `query/ast`, `query/ir` (IRPlan for the hook type), FTS/vector engine imports used by the hybrid region (copy from executor.nim imports: `fts/engine`, `vector/engine`, etc.). +- Produces: + - `evalExpr*` (all current overloads — Row and Table[string,string] variants), `evalExprOld*` (all overloads), `rowToStringTable`, `stringTableToValueRow`, `reciprocalRankFusion`, `realIdFromKey`, `findRealIdByDocId`, `doHybridSearch`, `doHybridSearchFiltered` (keep current export status). + - Two hook vars (new, the ONLY non-code-motion change): + ```nim + ## Wired by executor.nim at module load. Breaks the eval <-> executePlan / + ## execScan module cycle (subqueries, hybrid search). + var executePlanHook*: proc(ctx: ExecutionContext, plan: IRPlan): ExecResult + var execScanHook*: proc(ctx: ExecutionContext, tableName: string): seq[Row] + ``` + Exact hook signatures MUST be copied from the real `executePlan` / `execScan` signatures in executor.nim before moving (check `proc executePlan*` and `proc execScan` — including all parameters, e.g. filters/RLS args execScan takes; if execScan has more params, the hook type gets all of them). + +- [ ] **Step 1: Move eval + hybrid** + +Create `exec/eval.nim`; move: `evalExpr` (587-736), `rowToStringTable`/`stringTableToValueRow` (737-755), `evalExprOld` (756-1512), and the hybrid region (437-582: `reciprocalRankFusion`, `realIdFromKey`, `findRealIdByDocId`, `doHybridSearch`, `doHybridSearchFiltered`) including the `{.gcsafe.}` closure if it lives there (~line 542 — move verbatim). Move their forward decls too. + +- [ ] **Step 2: Redirect the two back-edges through hooks** + +In the moved code: replace every call to `executePlan(...)` inside evalExpr/evalExprOld (was at 889, 909, 1503) with `executePlanHook(...)`; replace the two `execScan(...)` calls in the hybrid procs (was 468, 562) with `execScanHook(...)`. Add the hook var declarations with a nil-guard: first line of each call site region stays a plain call; add at module bottom: +```nim +proc requireExecutePlanHook(): proc(ctx: ExecutionContext, plan: IRPlan): ExecResult = + if executePlanHook == nil: + raise newException(ValueError, "executePlanHook not wired (import barabadb/query/executor)") + executePlanHook +``` +and use `requireExecutePlanHook()(...)` at call sites (same pattern for execScanHook). Keep it minimal: direct `executePlanHook(...)` calls are acceptable if the nil raise is added once inside a tiny wrapper. + +- [ ] **Step 3: Wire executor.nim** + +Delete moved code; add `import exec/eval` + `export eval`. In executor.nim at module scope (bottom, after all procs are defined — or wire in Task 14 if executePlan/execScan are already moved; if still local, wire now): +```nim +eval.executePlanHook = executePlan +eval.execScanHook = execScan +``` + +- [ ] **Step 4: Compile and test** + +Run: `nim c -d:ssl --threads:on --path:src -o:tests/test_all tests/test_all.nim && ./tests/test_all` +Expected: exit 0, 461+ `[OK]` (the correlated-subquery and hybrid-search tests exercise both hooks). + +- [ ] **Step 5: Stage for commit** + +```bash +git add src/barabadb/query/exec/eval.nim src/barabadb/query/executor.nim +# commit after user confirmation: git commit -m "refactor(exec): extract expression evaluation + hybrid search into exec/eval.nim" +``` + +--- + +### Task 6: exec/lower.nim + +**Files:** +- Create: `src/barabadb/query/exec/lower.nim` +- Modify: `src/barabadb/query/executor.nim` + +**Interfaces:** +- Consumes: `exec/types`, `exec/values`, `exec/context` (`exprToSql` — called by lowerExpr, was 2530), `query/ast`, `query/ir`. +- Produces: `lowerExpr*`, `lowerSelect*`, `evalNodeToString` (keep export status). + +- [ ] **Step 1: Move the procs** + +Create `exec/lower.nim`; move the 2155–2557 region: `lowerExpr` (~222 lines), `evalNodeToString`, `lowerSelect` (~174 lines) + forward decls. + +- [ ] **Step 2: Wire executor.nim** + +Delete moved code; add `import exec/lower` + `export lower`. + +- [ ] **Step 3: Compile and test** + +Run: `nim c -d:ssl --threads:on --path:src -o:tests/test_all tests/test_all.nim && ./tests/test_all` +Expected: exit 0, 461+ `[OK]`. + +- [ ] **Step 4: Stage for commit** + +```bash +git add src/barabadb/query/exec/lower.nim src/barabadb/query/executor.nim +# commit after user confirmation: git commit -m "refactor(exec): extract AST->IR lowering into exec/lower.nim" +``` + +--- + +### Task 7: exec/rls.nim + +**Files:** +- Create: `src/barabadb/query/exec/rls.nim` +- Modify: `src/barabadb/query/executor.nim` + +**Interfaces:** +- Consumes: `exec/types` (PolicyDef, UserDef), `exec/eval` (`evalExpr`), `exec/lower` (`lowerExpr`) — both called in passesPolicy/checkInsertPolicy. +- Produces: `hasPrivilege`, `passesPolicy`, `checkInsertPolicy` (export all three with `*` — used by scan.nim and dml.nim next; do NOT re-export from executor unless they were exported before). + +- [ ] **Step 1: Move the procs** + +Create `exec/rls.nim`; move the 1520–1567 region + forward decls (there is a forward-decl block at ~1513 — move what belongs to these procs). + +- [ ] **Step 2: Wire executor.nim** + +Delete moved code; add `import exec/rls` (+ `export rls` only if any proc was previously exported). + +- [ ] **Step 3: Compile and test** + +Run: `nim c -d:ssl --threads:on --path:src -o:tests/test_all tests/test_all.nim && ./tests/test_all` +Expected: exit 0, 461+ `[OK]` (RLS/policy tests in test_all exercise this). + +- [ ] **Step 4: Stage for commit** + +```bash +git add src/barabadb/query/exec/rls.nim src/barabadb/query/executor.nim +# commit after user confirmation: git commit -m "refactor(exec): extract RLS/privileges into exec/rls.nim" +``` + +--- + +### Task 8: exec/scan.nim + +**Files:** +- Create: `src/barabadb/query/exec/scan.nim` +- Modify: `src/barabadb/query/executor.nim` + +**Interfaces:** +- Consumes: `exec/types`, `exec/values`, `exec/rls` (`passesPolicy` — was 1589), `exec/helpers` (`collectCorrelatedTablesFromPlan` — was 1600), storage imports as needed. +- Produces: `execScan`, `execPointRead` — exact current signatures; export both with `*` (needed by fk.nim, plan_exec.nim, and the eval execScanHook wiring). + +- [ ] **Step 1: Move the procs** + +Create `exec/scan.nim`; move the 1568–1624 region + forward decls. + +- [ ] **Step 2: Wire executor.nim** + +Delete moved code; add `import exec/scan` + `export scan` (export needed: eval.execScanHook assignment references execScan from executor.nim scope — importing is enough for the wiring line; re-export only if previously exported). + +- [ ] **Step 3: Compile and test** + +Run: `nim c -d:ssl --threads:on --path:src -o:tests/test_all tests/test_all.nim && ./tests/test_all` +Expected: exit 0, 461+ `[OK]`. + +- [ ] **Step 4: Stage for commit** + +```bash +git add src/barabadb/query/exec/scan.nim src/barabadb/query/executor.nim +# commit after user confirmation: git commit -m "refactor(exec): extract table scans into exec/scan.nim" +``` + +--- + +### Task 9: exec/dml.nim + +**Files:** +- Create: `src/barabadb/query/exec/dml.nim` +- Modify: `src/barabadb/query/executor.nim` + +**Interfaces:** +- Consumes: `exec/types`, `exec/values`, `exec/schema`, `exec/rls` (`hasPrivilege`, `checkInsertPolicy`), storage/lsm. +- Produces: `execInsert*`, `execDelete*`, `execUpdateRow*` (already exported today; keep signatures). + +- [ ] **Step 1: Move the procs** + +Create `exec/dml.nim`; move the 1625–1925 region: `execInsert` (~176 lines), `execDelete`, `execUpdateRow` + their private helpers + forward decls. Do NOT move validateType (belongs to triggers task). + +- [ ] **Step 2: Wire executor.nim** + +Delete moved code; add `import exec/dml` + `export dml`. + +- [ ] **Step 3: Compile and test** + +Run: `nim c -d:ssl --threads:on --path:src -o:tests/test_all tests/test_all.nim && ./tests/test_all` +Expected: exit 0, 461+ `[OK]`. + +- [ ] **Step 4: Stage for commit** + +```bash +git add src/barabadb/query/exec/dml.nim src/barabadb/query/executor.nim +# commit after user confirmation: git commit -m "refactor(exec): extract DML row operations into exec/dml.nim" +``` + +--- + +### Task 10: exec/fk.nim + +**Files:** +- Create: `src/barabadb/query/exec/fk.nim` +- Modify: `src/barabadb/query/executor.nim` + +**Interfaces:** +- Consumes: `exec/types` (ForeignKeyDef), `exec/values`, `exec/scan` (`execScan` — called by findReferencingRows, was 1928). +- Produces: `findReferencingRows`, `enforceFkOnDelete`, `enforceFkOnUpdate`, `enforceFkOnChildUpdate` (export with `*` for the dispatcher; NOT validateType — that moves in Task 11). + +- [ ] **Step 1: Move the procs** + +Create `exec/fk.nim`; move the 1926–2015 region (FK enforcement) — stop before `validateType` (~2016). + +- [ ] **Step 2: Wire executor.nim** + +Delete moved code; add `import exec/fk` (+ `export fk` only if previously exported). + +- [ ] **Step 3: Compile and test** + +Run: `nim c -d:ssl --threads:on --path:src -o:tests/test_all tests/test_all.nim && ./tests/test_all` +Expected: exit 0, 461+ `[OK]` (FK enforcement suite in test_all exercises this). + +- [ ] **Step 4: Stage for commit** + +```bash +git add src/barabadb/query/exec/fk.nim src/barabadb/query/executor.nim +# commit after user confirmation: git commit -m "refactor(exec): extract FK enforcement into exec/fk.nim" +``` + +--- + +### Task 11: exec/triggers.nim (with executeQueryHook) + +**Files:** +- Create: `src/barabadb/query/exec/triggers.nim` +- Modify: `src/barabadb/query/executor.nim` + +**Interfaces:** +- Consumes: `exec/types` (TriggerDef, CheckDef), `exec/values`, `exec/eval` (`evalExpr`), `exec/lower` (`lowerExpr`). +- Produces: `validateType*`, `fireTriggers*`, `validateConstraints*`, `applyDefaultValues*`, plus one new hook var: + ```nim + ## Wired by executor.nim at module load. fireTriggers executes trigger + ## action statements via the dispatcher; the hook breaks the module cycle. + var executeQueryHook*: proc(ctx: ExecutionContext, ast: Node): ExecResult + ``` + The signature MUST match how fireTriggers calls executeQueryImpl today (was 2064 — copy the exact call: argument count/types; if it passes params, include them). + +- [ ] **Step 1: Move the procs + hook** + +Create `exec/triggers.nim`; move `validateType` (~2016-2052), the 2056–2154 region (`fireTriggers`, `validateConstraints`, `applyDefaultValues`) + forward decls (block at ~2053). In `fireTriggers`, replace the `executeQueryImpl(...)` call with `executeQueryHook(...)`; add the nil-guard wrapper pattern from Task 5. + +- [ ] **Step 2: Wire executor.nim** + +Delete moved code; add `import exec/triggers` + `export triggers`. At module scope in executor.nim (after executeQueryImpl is defined): +```nim +triggers.executeQueryHook = (proc(ctx: ExecutionContext, ast: Node): ExecResult = executeQueryImpl(ctx, ast)) +``` +(adjust the lambda to the real call signature; executeQueryImpl is private, so the lambda must live in executor.nim — that is exactly why the hook exists). + +- [ ] **Step 3: Compile and test** + +Run: `nim c -d:ssl --threads:on --path:src -o:tests/test_all tests/test_all.nim && ./tests/test_all` +Expected: exit 0, 461+ `[OK]` (trigger tests exercise the hook). + +- [ ] **Step 4: Stage for commit** + +```bash +git add src/barabadb/query/exec/triggers.nim src/barabadb/query/executor.nim +# commit after user confirmation: git commit -m "refactor(exec): extract triggers/constraints into exec/triggers.nim" +``` + +--- + +### Task 12: exec/window.nim + +**Files:** +- Create: `src/barabadb/query/exec/window.nim` +- Modify: `src/barabadb/query/executor.nim` + +**Interfaces:** +- Consumes: `exec/types`, `exec/values`, `exec/eval` (`evalExpr` — partitionKey/compareRowsByOrder/computeWindowValues). +- Produces: `partitionKey`, `compareRowsByOrder`, `resolveFrameBounds`, `computeWindowValues*`, `expandStarRow` (export computeWindowValues as today; others per current status — plan_exec.nim needs them, so export all five). + +- [ ] **Step 1: Move the procs** + +Create `exec/window.nim`; move the 2558–2747 region + forward decls. + +- [ ] **Step 2: Wire executor.nim** + +Delete moved code; add `import exec/window` + `export window`. + +- [ ] **Step 3: Compile and test** + +Run: `nim c -d:ssl --threads:on --path:src -o:tests/test_all tests/test_all.nim && ./tests/test_all` +Expected: exit 0, 461+ `[OK]` (window function tests exercise this). + +- [ ] **Step 4: Stage for commit** + +```bash +git add src/barabadb/query/exec/window.nim src/barabadb/query/executor.nim +# commit after user confirmation: git commit -m "refactor(exec): extract window functions into exec/window.nim" +``` + +--- + +### Task 13: exec/plan_exec.nim + +**Files:** +- Create: `src/barabadb/query/exec/plan_exec.nim` +- Modify: `src/barabadb/query/executor.nim` + +**Interfaces:** +- Consumes: `exec/types`, `exec/values`, `exec/schema`, `exec/eval`, `exec/lower`, `exec/helpers` (`chooseJoinStrategy`, `extractJoinEquality`), `exec/scan` (`execScan`), `exec/window` (`computeWindowValues`, `expandStarRow`), `query/ir`. +- Produces: `executePlan*` (exact current signature — the symbol the eval hook points at). + +- [ ] **Step 1: Move the proc** + +Create `exec/plan_exec.nim`; move `executePlan` (~990 lines, 2748–3738) + its private helpers + forward decls. + +- [ ] **Step 2: Wire executor.nim** + +Delete moved code; add `import exec/plan_exec` + `export plan_exec`. If the `eval.executePlanHook = executePlan` wiring (Task 5 Step 3) was deferred, add it now at module scope in executor.nim. + +- [ ] **Step 3: Compile and test** + +Run: `nim c -d:ssl --threads:on --path:src -o:tests/test_all tests/test_all.nim && ./tests/test_all` +Expected: exit 0, 461+ `[OK]`. + +- [ ] **Step 4: Stage for commit** + +```bash +git add src/barabadb/query/exec/plan_exec.nim src/barabadb/query/executor.nim +# commit after user confirmation: git commit -m "refactor(exec): extract IR plan execution into exec/plan_exec.nim" +``` + +--- + +### Task 14: Slim down executor.nim + verify hook wiring + +**Files:** +- Modify: `src/barabadb/query/executor.nim` + +**Interfaces:** +- Consumes: all new exec/* modules. +- Produces: unchanged public API: `executeQuery*`, plus re-exports of everything that was exported before. + +- [ ] **Step 1: Clean up executor.nim** + +executor.nim should now contain ONLY: the import block (trimmed to what the dispatcher needs), `import exec/X` + `export X` lines for all modules, remaining forward decls for `executeQueryImpl` (self-recursion), `executeQueryImpl` (the ~1,473-line dispatcher), `executeQuery` (DDL-locked wrapper — keep the `ctx.sharedLock.lock` semantics byte-identical), `executeMigrationSql`, and the two hook-wiring assignments at module scope: +```nim +eval.executePlanHook = plan_exec.executePlan +eval.execScanHook = scan.execScan +triggers.executeQueryHook = (proc(ctx: ExecutionContext, ast: Node): ExecResult = executeQueryImpl(ctx, ast)) +``` +(adjust to real signatures). Remove leftover dead forward decls and now-unused imports — verify with the XDeclaredButNotUsed/UnusedImport hints from the compiler output; aim for zero new hints. + +- [ ] **Step 2: Compile and test** + +Run: `nim c -d:ssl --threads:on --path:src -o:tests/test_all tests/test_all.nim && ./tests/test_all` +Expected: exit 0, 461+ `[OK]`. + +- [ ] **Step 3: Verify the public API freeze** + +Run: `grep -hoE 'qexec\.[a-zA-Z]+|executor\.[a-zA-Z]+' tests/*.nim src/baradadb.nim src/barabadb/core/server.nim src/barabadb/core/httpserver.nim src/barabadb/mcp/server.nim | sort -u` and confirm every symbol resolves from executor.nim (compile of the full server proves it): +Run: `nim c -d:ssl --threads:on --path:src -o:build/baradadb src/baradadb.nim && nim c -d:ssl --threads:on --path:src -o:build/baramcp src/baramcp.nim` +Expected: both compile clean. + +- [ ] **Step 4: Stage for commit** + +```bash +git add src/barabadb/query/executor.nim +# commit after user confirmation: git commit -m "refactor(exec): slim executor.nim to dispatcher + hook wiring" +``` + +--- + +### Task 15: Full verification + docs + +**Files:** +- Modify: `src/barabadb/query/exec/README.md` +- Modify: `docs/superpowers/specs/2026-07-30-stability-hardening-design.md` (mark B2 done) + +- [ ] **Step 1: Full test suite** + +Run: `nimble test` +Expected: exit 0, 650 `[OK]`, 0 failed. This covers all 13 suites including join_tests, prop_test (uses lowerSelect/executePlan directly), test_wire_insert_stress, nimforum_smoke_test (TCP server). + +- [ ] **Step 2: Update exec/README.md** + +Rewrite the layering section to the final state: types → values → schema → context/helpers/params/migrations → eval → lower → rls → scan → dml/fk → triggers → window → plan_exec → executor, with a note documenting the two hooks (`executePlanHook`, `execScanHook`, `executeQueryHook`) and why they exist (Nim forbids circular imports; subqueries/trigger bodies are genuine recursion points). + +- [ ] **Step 3: Report sizes** + +Run: `wc -l src/barabadb/query/executor.nim src/barabadb/query/exec/*.nim | sort -n` +Expected: executor.nim ≈ 1,600 lines; no module over ~1,500 lines. + +- [ ] **Step 4: Stage for commit** + +```bash +git add src/barabadb/query/exec/README.md docs/superpowers/specs/2026-07-30-stability-hardening-design.md +# commit after user confirmation: git commit -m "docs(exec): document module layering after executor split" +``` + +--- + +## Self-Review Notes + +- Spec coverage: every region of executor.nim (per the dependency map) is assigned to exactly one task; dispatcher + wrapper stay in Task 14. +- Type consistency: hook signatures are defined by copying the real `executePlan`/`execScan`/`executeQueryImpl` call signatures at the task site — the compiler enforces the match at each task's Step 3. +- Riskiest tasks: 5 (eval + hooks) and 11 (triggers hook) — both are covered by existing correlated-subquery/hybrid/trigger tests in test_all. diff --git a/docs/superpowers/specs/2026-07-30-stability-hardening-design.md b/docs/superpowers/specs/2026-07-30-stability-hardening-design.md new file mode 100644 index 0000000..026d378 --- /dev/null +++ b/docs/superpowers/specs/2026-07-30-stability-hardening-design.md @@ -0,0 +1,132 @@ +# BaraDB Stability Hardening — Design & Findings + +Date: 2026-07-30 +Status: Implemented (phase A). Phases B/C proposed, awaiting decision. + +## Goal + +"Make the database better" — chosen direction: **stability first**. Establish a +verified baseline, fix what is actually broken, and make the whole test suite +run with one command before attempting any large refactoring or new features. + +## Baseline established + +- Debug build passes cleanly on Nim 2.2.10 (`nimble build_debug`). +- The `hunos` build failure from `HUNOS_ISSUE.md` is **no longer an issue**: + hunos 1.3.3 is installed and contains the `urandom` fix. The nimble file + already allows `>= 1.3.0`. +- Of the 13 test files in `tests/`, only `test_all` (+ `stress_test` in CI) + ran automatically. Baseline run of the other 11: 10 pass, + `nimforum_smoke_test` fails. + +## Changes implemented + +### 1. Parser: `header` usable as a column name (bug fix) + +**Root cause.** `header` is a keyword token (`tkHeader`) used by +`IMPORT/EXPORT ... HEADER`. The parser never accepted it as an identifier, so +any table with a column named `header` (e.g. the nimforum schema) failed with +`Expected identifier but got tkHeader`. + +**Fix** (`src/barabadb/query/parser.nim`): +- Added `tkHeader` to `identLikeKinds` (soft-keyword set used by + `expectIdent`, line 38). +- Added `tkHeader` to the identifier branch of `parsePrimary` (line 84) so + `SELECT header ...` and `WHERE header = ...` work. +- Dotted-path parsing (`a.b.c`) now uses `expectIdent` instead of + `expect(tkIdent)` so `post.header` works too. + +IMPORT/EXPORT parsing is unaffected: statement dispatch keys on the leading +`tkImport`/`tkExport` and the clause parser peeks for `tkHeader` explicitly. + +**Regression tests** (`tests/bugfix_test.nim`, new suite): +- CREATE TABLE / INSERT / SELECT / WHERE with a column named `header`. +- `IMPORT FROM ... HEADER no` and `EXPORT TO ... HEADER yes` still parse. + +Note: `IMPORT ... FORMAT csv` currently fails because `csv` is also a keyword +(`tkCsv`) and `parseImportFrom` expects `tkIdent` after `FORMAT`. Pre-existing +limitation, **not** addressed here (out of scope; recorded for phase B). + +### 2. All 13 test files wired into `nimble test` and CI + +- `baradadb.nimble` `test` task now builds `build/baradadb` (the smoke test + talks to it over TCP) and runs all 13 suites: quick embedded suites first, + fuzz/property/stress last. +- `.github/workflows/ci.yml` runs `nimble test` (was: only `test_all`); + the now-redundant separate stress-test steps were removed. +- Verified locally: full `nimble test` exits 0 with 648 passing checks. + +### 2b. Soft-keyword cleanup (phase B3, implemented) + +Extended the `header` approach to all clause-only keywords, so they work as +table/column names everywhere (DDL, DML, aliases, dotted paths, CTEs, JOINs, +MERGE, GRANT/REVOKE, SET): +`format, delimiter, batch, csv, ndjson, status, migration, apply, up, down, +dryrun, user, policy, enable, disable, recover, before, after, instead, of`. + +- `identLikeKinds` and the `parsePrimary` identifier branch now include them. +- All 69 `expect(tkIdent)` call sites now use `expectIdent` — a strict + superset, so previously valid SQL is unaffected (verified: full suite green). +- `IMPORT/EXPORT`: `FORMAT csv/ndjson/json` and `HEADER true/false` now parse + (previously `csv`/`true`/`false` lexed as keywords and were rejected despite + the grammar clearly intending them). Clause table names use `expectIdent`. + +Structural keywords (`where`, `group`, `order`, `join`, `on`, `for`, `using`, +`view`, `trigger`, `import`, `export`, `grant`, ...) remain reserved. + +**Regression tests** (`tests/bugfix_test.nim`): a table with 21 keyword-named +columns through CREATE/INSERT/UPDATE/SELECT/qualified refs, plus +IMPORT/EXPORT keyword-value parsing. + +### 3. ORC crash — reproduced, bisected, three root-cause attempts failed + +`nim.cfg` forces `--mm:arc` because ORC's cycle collector crashed +("markGray/trace SIGSEGV after ~20 sequential INSERTs"). The ARC cycle-breaking +in commit `ed5a719` did not fix the ORC path. + +**Reproduction** (`tests/orc_repro.py`): build the server with `--mm:orc`, +drive 1000 sequential TCP INSERTs plus 10 concurrent connections. The server +dies with the exact documented signature (`handleClient` → +`nimDecRefIsLastCyclicStatic` → `collectCyclesBacon` → `markGray` → `trace` → +SIGSEGV). + +**Bisect:** 200 pings + 200 SELECTs over TCP are fine; the crash lands between +20 and 500 sequential INSERTs — INSERT path only. + +**Failed root-cause attempts:** +1. `ed5a719` — callback cycle breaks (shard/gossip). +2. `{.cursor.}` on `ExecutionContext.registry` — breaks the real + `DatabaseRegistry ↔ ExecutionContext` cycle (kept: it is the correct + ownership annotation regardless), but the crash persists unchanged. +3. Guarding `ctx.onChange` against zero WS subscribers (reverted: fixed + nothing). + +**Conclusion:** per the 3-strikes rule this is a deep ORC+async issue — +possibly an upstream Nim 2.2.x ORC bug with async closure environments and/or +complex generic types — not a single app-level cycle. ARC remains the +supported memory manager (full suite green under it). The findings are +recorded in `nim.cfg` and `tests/orc_repro.py` for a future attempt (e.g. +re-test with a newer Nim runtime, or a minimal repro filed upstream). + +## Proposed next phases (not started) + +- **B2. Split `query/executor.nim`** (5,398 lines) into focused modules + (DML, DDL, select pipeline, transactions). Now safe to do: the full suite + guards behavior. Large diff, mechanical. +- **C. Features** — real Raft network transport, persistence for + graph/FTS/columnar engines, benchmark validation. +- **ORC (blocked):** re-test `tests/orc_repro.py` against a newer Nim runtime; + if it persists, distill a minimal repro and file upstream. Not app-actionable + today (see "ORC crash" section). + +## Verification evidence + +- `nimble test` (all 13 suites): exit 0, 650 `[OK]`, 0 failed — final run + after all changes (phase A + B3 + cursor). +- `nimforum_smoke_test` (rebuilt server): all suites `[OK]`, including + `NimForum schema creation` (previously `[FAILED]`). +- B3 TDD: new keyword tests failed first with the expected `tkFormat`/`tkCsv` + errors, then passed; `test_all` stayed green (461 `[OK]`). +- ORC investigation: embedded `test_wire_insert_stress` passes even when + compiled with `--mm:orc`; the TCP **server** compiled with `--mm:orc` + crashes as documented above. All shipped artifacts use ARC and are green. diff --git a/nim.cfg b/nim.cfg index f10d906..20ac5e7 100644 --- a/nim.cfg +++ b/nim.cfg @@ -2,7 +2,12 @@ --threads:on --path:"src" # ARC: ORC cycle collector crashes under async wire-protocol load -# (markGray/trace SIGSEGV after ~20 sequential INSERTs). ARC is stable -# for the TCP server + HTTP worker mix. Prefer breaking cycles over -# re-enabling ORC without a reproducer. +# (markGray/trace SIGSEGV, triggered from core/server.nim handleClient). +# Still reproducible as of 2026-07-30 (Nim 2.2.10) — reproducer: +# tests/orc_repro.py. Bisected: 200 pings + 200 SELECTs over TCP are +# fine; the server dies somewhere between 20 and 500 sequential INSERTs. +# Three root-cause attempts failed (callback cycle breaks in ed5a719, +# {.cursor.} on ExecutionContext.registry, guarding ctx.onChange) — this +# points at a deep ORC+async issue (possibly upstream Nim), not a single +# app-level cycle. ARC is stable for the TCP server + HTTP worker mix. --mm:arc diff --git a/src/barabadb/query/exec/types.nim b/src/barabadb/query/exec/types.nim index 401af96..3cd7e98 100644 --- a/src/barabadb/query/exec/types.nim +++ b/src/barabadb/query/exec/types.nim @@ -113,7 +113,12 @@ type outerRow*: Table[string, string] # outer query row for correlated subqueries subqueryPlan*: IRPlan # current subquery plan being evaluated currentDatabase*: string # name of the currently selected database - registry*: DatabaseRegistry # nil for single-DB mode + # The registry owns this context (registry -> DatabaseInfo -> ctx), so this + # back-reference is a non-owning cursor: it breaks the registry <-> ctx + # reference cycle. The registry always outlives its contexts (closeAll at + # shutdown). Note: breaking this cycle alone does NOT make ORC usable — + # the ORC crash under wire INSERT load persists (see tests/orc_repro.py). + registry* {.cursor.}: DatabaseRegistry # nil for single-DB mode MigrationRecord* = object name*: string diff --git a/src/barabadb/query/parser.nim b/src/barabadb/query/parser.nim index 7531738..00ddd37 100644 --- a/src/barabadb/query/parser.nim +++ b/src/barabadb/query/parser.nim @@ -35,7 +35,12 @@ proc match(p: var Parser, kind: TokenKind): bool = return false # Token kinds that can also serve as identifiers in table/column name positions -const identLikeKinds = {tkIdent, tkLabels, tkCount, tkSum, tkAvg, tkMin, tkMax, tkArrayAgg, tkStringAgg, tkJsonFmt, tkArray, tkVector, tkGraph, tkDocument} +const identLikeKinds = {tkIdent, tkLabels, tkCount, tkSum, tkAvg, tkMin, tkMax, + tkArrayAgg, tkStringAgg, tkJsonFmt, tkArray, tkVector, tkGraph, tkDocument, + tkHeader, tkFormat, tkDelimiter, tkBatch, tkCsv, tkNdjson, + tkStatus, tkMigration, tkApply, tkUp, tkDown, tkDryRun, + tkUser, tkPolicy, tkEnable, tkDisable, tkRecover, + tkBefore, tkAfter, tkInstead, tkOf} proc expectIdent(p: var Parser): Token = ## Expect a token that can serve as an identifier (table name, column name, alias, etc.). @@ -81,7 +86,11 @@ proc parsePrimary(p: var Parser): Node = of tkCurrentRole: discard p.advance() Node(kind: nkCurrentRole, line: tok.line, col: tok.col) - of tkIdent, tkLabels, tkRowNumber, tkRank, tkDenseRank, tkLead, tkLag, tkFirstValue, tkLastValue, tkNtile: + of tkIdent, tkLabels, tkRowNumber, tkRank, tkDenseRank, tkLead, tkLag, tkFirstValue, tkLastValue, tkNtile, + tkHeader, tkFormat, tkDelimiter, tkBatch, tkCsv, tkNdjson, + tkStatus, tkMigration, tkApply, tkUp, tkDown, tkDryRun, + tkUser, tkPolicy, tkEnable, tkDisable, tkRecover, + tkBefore, tkAfter, tkInstead, tkOf: discard p.advance() let funcName = tok.value # Check for function call: ident(...) @@ -105,7 +114,7 @@ proc parsePrimary(p: var Parser): Node = var parts = @[funcName] while p.peek().kind == tkDot: discard p.advance() # consume . - parts.add(p.expect(tkIdent).value) + parts.add(p.expectIdent().value) if parts.len == 1: return Node(kind: nkIdent, identName: funcName, line: tok.line, col: tok.col) return Node(kind: nkPath, pathParts: parts, line: tok.line, col: tok.col) @@ -462,7 +471,7 @@ proc parseWith(p: var Parser): Node = isRecursive = true # Parse first CTE - let cteName = p.expect(tkIdent).value + let cteName = p.expectIdent().value discard p.expect(tkAs) discard p.expect(tkLParen) let cteQuery = p.parseSelect() @@ -471,7 +480,7 @@ proc parseWith(p: var Parser): Node = # Parse additional CTEs while p.match(tkComma): - let name = p.expect(tkIdent).value + let name = p.expectIdent().value discard p.expect(tkAs) discard p.expect(tkLParen) let query = p.parseSelect() @@ -498,12 +507,12 @@ proc parseSelect(p: var Parser): Node = result.selResult = @[] var expr = p.parseExpr() if p.match(tkAs): - expr.exprAlias = p.expect(tkIdent).value + expr.exprAlias = p.expectIdent().value result.selResult.add(expr) while p.match(tkComma): expr = p.parseExpr() if p.match(tkAs): - expr.exprAlias = p.expect(tkIdent).value + expr.exprAlias = p.expectIdent().value result.selResult.add(expr) # Parse FROM @@ -516,7 +525,7 @@ proc parseSelect(p: var Parser): Node = discard p.expect(tkRParen) var alias = "" if p.match(tkAs): - alias = p.expect(tkIdent).value + alias = p.expectIdent().value elif p.peek().kind == tkIdent: alias = p.advance().value result.selFrom = Node(kind: nkFrom, fromTable: "(subquery)", @@ -525,7 +534,7 @@ proc parseSelect(p: var Parser): Node = # GRAPH_TABLE(name MATCH (pattern) COLUMNS (cols)) discard p.advance() discard p.expect(tkLParen) - let graphName = p.expect(tkIdent).value + let graphName = p.expectIdent().value var hasMatch = p.match(tkMatch) var patternNodes: seq[string] var patternEdges: seq[string] @@ -595,7 +604,7 @@ proc parseSelect(p: var Parser): Node = if p.peek().kind in {tkIdent, tkLabels, tkEdge, tkGraph, tkRank, tkBfs, tkDfs, tkMatch, tkColumns, tkEnd, tkSrc, tkDst, tkMerge}: colName &= "." & p.advance().value else: - colName &= "." & p.expect(tkIdent).value + colName &= "." & p.expectIdent().value returnCols.add(colName) while p.match(tkComma): if p.peek().kind in {tkIdent, tkLabels, tkEdge, tkGraph, tkRank, tkEnd, tkMatch, tkColumns, tkSrc, tkDst, tkBfs, tkDfs, tkMerge}: @@ -605,7 +614,7 @@ proc parseSelect(p: var Parser): Node = if p.peek().kind in {tkIdent, tkLabels, tkEdge, tkGraph, tkRank, tkBfs, tkDfs, tkMatch, tkColumns, tkEnd, tkSrc, tkDst, tkMerge}: colName &= "." & p.advance().value else: - colName &= "." & p.expect(tkIdent).value + colName &= "." & p.expectIdent().value returnCols.add(colName) if p.match(tkAs): discard p.advance() @@ -629,10 +638,10 @@ proc parseSelect(p: var Parser): Node = gtDirection: "out", gtEnd: endNode, gtMaxDepth: maxDepth, gtReturnCols: returnCols, gtAlgo: algo, line: tok.line, col: tok.col) else: - let tableTok = p.expect(tkIdent) + let tableTok = p.expectIdent() var alias = "" if p.match(tkAs): - alias = p.expect(tkIdent).value + alias = p.expectIdent().value elif p.peek().kind == tkIdent: alias = p.advance().value result.selFrom = Node(kind: nkFrom, fromTable: tableTok.value, @@ -641,10 +650,10 @@ proc parseSelect(p: var Parser): Node = # Comma join: FROM t1, t2 → implicit CROSS JOIN while p.peek().kind == tkComma: discard p.advance() - let nextTableTok = p.expect(tkIdent) + let nextTableTok = p.expectIdent() var nextAlias = "" if p.match(tkAs): - nextAlias = p.expect(tkIdent).value + nextAlias = p.expectIdent().value elif p.peek().kind == tkIdent: nextAlias = p.advance().value let joinNode = Node(kind: nkJoin, joinKind: jkCross, @@ -660,7 +669,7 @@ proc parseSelect(p: var Parser): Node = discard p.expect(tkLParen) let aggFunc = p.parseExpr() # e.g. SUM(salary) discard p.expect(tkFor) - let forCol = p.expect(tkIdent).value + let forCol = p.expectIdent().value discard p.expect(tkIn) discard p.expect(tkLParen) var inValues: seq[string] = @[] @@ -675,15 +684,15 @@ proc parseSelect(p: var Parser): Node = elif p.peek().kind == tkUnpivot: discard p.advance() discard p.expect(tkLParen) - let valCol = p.expect(tkIdent).value + let valCol = p.expectIdent().value discard p.expect(tkFor) - let forCol = p.expect(tkIdent).value + let forCol = p.expectIdent().value discard p.expect(tkIn) discard p.expect(tkLParen) var inCols: seq[string] = @[] - inCols.add(p.expect(tkIdent).value) + inCols.add(p.expectIdent().value) while p.match(tkComma): - inCols.add(p.expect(tkIdent).value) + inCols.add(p.expectIdent().value) discard p.expect(tkRParen) discard p.expect(tkRParen) result.selFrom = Node(kind: nkUnpivot, unpivotSource: result.selFrom, @@ -718,15 +727,15 @@ proc parseSelect(p: var Parser): Node = let subquery = p.parseSelect() discard p.expect(tkRParen) if p.match(tkAs): - joinAlias = p.expect(tkIdent).value + joinAlias = p.expectIdent().value elif p.peek().kind == tkIdent: joinAlias = p.advance().value joinTarget = Node(kind: nkSubquery, subQuery: subquery, line: tok.line, col: tok.col) else: - let joinTable = p.expect(tkIdent) + let joinTable = p.expectIdent() if p.match(tkAs): - joinAlias = p.expect(tkIdent).value + joinAlias = p.expectIdent().value elif p.peek().kind == tkIdent: joinAlias = p.advance().value joinTarget = Node(kind: nkFrom, fromTable: joinTable.value, @@ -846,7 +855,7 @@ proc parseSelect(p: var Parser): Node = proc parseInsert(p: var Parser): Node = let tok = p.expect(tkInsert) discard p.match(tkInto) # optional INTO - let target = p.expect(tkIdent).value + let target = p.expectIdent().value result = Node(kind: nkInsert, insTarget: target, line: tok.line, col: tok.col) result.insFields = @[] result.insValues = @[] @@ -892,18 +901,18 @@ proc parseInsert(p: var Parser): Node = proc parseUpdate(p: var Parser): Node = let tok = p.expect(tkUpdate) - let target = p.expect(tkIdent).value + let target = p.expectIdent().value result = Node(kind: nkUpdate, updTarget: target, line: tok.line, col: tok.col) if p.match(tkSet): result.updSet = @[] - let field = p.expect(tkIdent).value + let field = p.expectIdent().value discard p.match(tkEq) # = or := let val = p.parseExpr() result.updSet.add(Node(kind: nkBinOp, binOp: bkAssign, binLeft: Node(kind: nkIdent, identName: field), binRight: val)) while p.match(tkComma): - let f = p.expect(tkIdent).value + let f = p.expectIdent().value discard p.match(tkEq) let v = p.parseExpr() result.updSet.add(Node(kind: nkBinOp, binOp: bkAssign, @@ -920,7 +929,7 @@ proc parseUpdate(p: var Parser): Node = proc parseDelete(p: var Parser): Node = let tok = p.expect(tkDelete) discard p.match(tkFrom) # optional FROM keyword - let target = p.expect(tkIdent).value + let target = p.expectIdent().value result = Node(kind: nkDelete, delTarget: target, line: tok.line, col: tok.col) if p.match(tkWhere): result.delWhere = Node(kind: nkWhere, whereExpr: p.parseExpr()) @@ -934,9 +943,9 @@ proc parseMerge(p: var Parser): Node = let tok = p.expect(tkMerge) discard p.match(tkInto) # optional INTO result = Node(kind: nkMerge, line: tok.line, col: tok.col) - result.mergeTarget = p.expect(tkIdent).value + result.mergeTarget = p.expectIdent().value if p.match(tkAs): - result.mergeTargetAlias = p.expect(tkIdent).value + result.mergeTargetAlias = p.expectIdent().value elif p.peek().kind == tkIdent: result.mergeTargetAlias = p.advance().value discard p.expect(tkUsing) @@ -946,10 +955,10 @@ proc parseMerge(p: var Parser): Node = result.mergeSource = p.parseSelect() discard p.expect(tkRParen) else: - let srcTable = p.expect(tkIdent).value + let srcTable = p.expectIdent().value result.mergeSource = Node(kind: nkIdent, identName: srcTable, line: tok.line, col: tok.col) if p.match(tkAs): - result.mergeSourceAlias = p.expect(tkIdent).value + result.mergeSourceAlias = p.expectIdent().value elif p.peek().kind == tkIdent: result.mergeSourceAlias = p.advance().value discard p.expect(tkOn) @@ -977,9 +986,9 @@ proc parseMerge(p: var Parser): Node = elif p.peek().kind == tkInsert: discard p.advance() discard p.expect(tkLParen) - result.mergeNotMatchedInsert.add(Node(kind: nkIdent, identName: p.expect(tkIdent).value)) + result.mergeNotMatchedInsert.add(Node(kind: nkIdent, identName: p.expectIdent().value)) while p.match(tkComma): - result.mergeNotMatchedInsert.add(Node(kind: nkIdent, identName: p.expect(tkIdent).value)) + result.mergeNotMatchedInsert.add(Node(kind: nkIdent, identName: p.expectIdent().value)) discard p.expect(tkRParen) discard p.expect(tkValues) discard p.expect(tkLParen) @@ -990,13 +999,13 @@ proc parseMerge(p: var Parser): Node = elif p.peek().kind == tkUpdate: discard p.advance() discard p.expect(tkSet) - let col = p.expect(tkIdent).value + let col = p.expectIdent().value discard p.expect(tkEq) result.mergeMatchedUpdate.add(Node(kind: nkBinOp, binOp: bkAssign, binLeft: Node(kind: nkIdent, identName: col), binRight: p.parseExpr())) while p.match(tkComma): - let col2 = p.expect(tkIdent).value + let col2 = p.expectIdent().value discard p.expect(tkEq) result.mergeMatchedUpdate.add(Node(kind: nkBinOp, binOp: bkAssign, binLeft: Node(kind: nkIdent, identName: col2), @@ -1005,7 +1014,7 @@ proc parseMerge(p: var Parser): Node = proc parseCreateType(p: var Parser): Node = let tok = p.expect(tkCreate) discard p.expect(tkType) - let name = p.expect(tkIdent).value + let name = p.expectIdent().value result = Node(kind: nkCreateType, ctName: name, line: tok.line, col: tok.col) result.ctBases = @[] if p.match(tkIdent): @@ -1023,10 +1032,10 @@ proc parseCreateType(p: var Parser): Node = if p.peek().kind == tkMulti: discard p.advance() isMulti = true - let fieldTok = p.expect(tkIdent) + let fieldTok = p.expectIdent() if p.peek().kind == tkArrow: discard p.advance() - let target = p.expect(tkIdent).value + let target = p.expectIdent().value result.ctLinks.add(Node(kind: nkLinkDef, ldName: fieldTok.value, ldTarget: target, ldRequired: isRequired, @@ -1034,7 +1043,7 @@ proc parseCreateType(p: var Parser): Node = else: var typeName = "" if p.match(tkColon): - typeName = p.expect(tkIdent).value + typeName = p.expectIdent().value result.ctProperties.add(Node(kind: nkPropertyDef, pdName: fieldTok.value, pdType: typeName, pdRequired: isRequired)) @@ -1270,14 +1279,14 @@ proc parseAlterTable(p: var Parser): Node = if p.peek().kind == tkEnable: discard p.advance() discard p.expect(tkRow) # ROW - discard p.expect(tkIdent) # LEVEL - discard p.expect(tkIdent) # SECURITY + discard p.expectIdent() # LEVEL + discard p.expectIdent() # SECURITY return Node(kind: nkEnableRLS, erlsTable: tableName, line: tok.line, col: tok.col) elif p.peek().kind == tkDisable: discard p.advance() discard p.expect(tkRow) # ROW - discard p.expect(tkIdent) # LEVEL - discard p.expect(tkIdent) # SECURITY + discard p.expectIdent() # LEVEL + discard p.expectIdent() # SECURITY return Node(kind: nkDisableRLS, drlsTable: tableName, line: tok.line, col: tok.col) result = Node(kind: nkAlterTable, line: tok.line, col: tok.col) result.altName = tableName @@ -1349,10 +1358,10 @@ proc parseCreateView(p: var Parser): Node = var orReplace = false if p.peek().kind == tkIdent and p.peek().value.toLower() == "or": discard p.advance() - discard p.expect(tkIdent) # REPLACE + discard p.expectIdent() # REPLACE orReplace = true discard p.expect(tkView) - let name = p.expect(tkIdent).value + let name = p.expectIdent().value discard p.expect(tkAs) let query = p.parseSelect() result = Node(kind: nkCreateView, cvName: name, cvQuery: query, @@ -1366,14 +1375,14 @@ proc parseDropView(p: var Parser): Node = discard p.advance() discard p.expect(tkExists) ifExists = true - let name = p.expect(tkIdent).value + let name = p.expectIdent().value result = Node(kind: nkDropView, dvName: name, dvIfExists: ifExists, line: tok.line, col: tok.col) proc parseCreateTrigger(p: var Parser): Node = let tok = p.expect(tkCreate) discard p.expect(tkTrigger) - let name = p.expect(tkIdent).value + let name = p.expectIdent().value # Parse timing: BEFORE | AFTER | INSTEAD OF var timing = "" let timingTok = p.peek() @@ -1404,7 +1413,7 @@ proc parseCreateTrigger(p: var Parser): Node = else: raise newException(ValueError, "Expected INSERT, UPDATE, or DELETE in TRIGGER definition") discard p.expect(tkOn) - let tableName = p.expect(tkIdent).value + let tableName = p.expectIdent().value discard p.expect(tkAs) # Parse action as raw string until end of statement var actionStr = "" @@ -1425,7 +1434,7 @@ proc parseDropTrigger(p: var Parser): Node = discard p.advance() discard p.expect(tkExists) ifExists = true - let name = p.expect(tkIdent).value + let name = p.expectIdent().value result = Node(kind: nkDropTrigger, trigDropName: name, trigDropIfExists: ifExists, line: tok.line, col: tok.col) @@ -1437,13 +1446,13 @@ proc parseDropIndex(p: var Parser): Node = discard p.advance() discard p.expect(tkExists) ifExists = true - let name = p.expect(tkIdent).value + let name = p.expectIdent().value result = Node(kind: nkDropIndex, diName: name, line: tok.line, col: tok.col) proc parseCreateMigration(p: var Parser): Node = let tok = p.expect(tkCreate) discard p.expect(tkMigration) - let name = p.expect(tkIdent).value + let name = p.expectIdent().value var upBody = "" var downBody = "" if p.peek().kind == tkAs: @@ -1463,7 +1472,7 @@ proc parseCreateMigration(p: var Parser): Node = discard p.advance() section = "down" elif sectionTok.kind == tkIdent: - section = p.expect(tkIdent).value.toLower() + section = p.expectIdent().value.toLower() else: raise newException(ValueError, "Expected UP or DOWN in migration body, got: " & $sectionTok.kind) discard p.expect(tkColon) @@ -1492,7 +1501,7 @@ proc parseCreateMigration(p: var Parser): Node = proc parseApplyMigration(p: var Parser): Node = let tok = p.expect(tkApply) discard p.expect(tkMigration) - let name = p.expect(tkIdent).value + let name = p.expectIdent().value result = Node(kind: nkApplyMigration, amName: name, line: tok.line, col: tok.col) proc parseMigrationStatus(p: var Parser): Node = @@ -1519,7 +1528,7 @@ proc parseMigrationDown(p: var Parser): Node = proc parseMigrationDryRun(p: var Parser): Node = let tok = p.expect(tkMigration) discard p.expect(tkDryRun) - let name = p.expect(tkIdent).value + let name = p.expectIdent().value result = Node(kind: nkMigrationDryRun, mdrName: name, line: tok.line, col: tok.col) proc parseImportFrom(p: var Parser): Node = @@ -1527,7 +1536,7 @@ proc parseImportFrom(p: var Parser): Node = discard p.expect(tkFrom) let path = p.expect(tkStringLit).value discard p.expect(tkInto) - let table = p.expect(tkIdent).value + let table = p.expectIdent().value var format = "csv" var delimiter = ',' var hasHeader = true @@ -1536,14 +1545,20 @@ proc parseImportFrom(p: var Parser): Node = let kw = p.advance() case kw.kind of tkFormat: - let fmt = p.expect(tkIdent).value.toLower() - format = fmt + format = p.expectIdent().value.toLower() of tkDelimiter: let delim = p.expect(tkStringLit).value if delim.len > 0: delimiter = delim[0] of tkHeader: - let hdr = p.expect(tkIdent).value.toLower() - hasHeader = hdr == "true" or hdr == "yes" + if p.peek().kind == tkTrue: + discard p.advance() + hasHeader = true + elif p.peek().kind == tkFalse: + discard p.advance() + hasHeader = false + else: + let hdr = p.expectIdent().value.toLower() + hasHeader = hdr == "true" or hdr == "yes" of tkBatch: batchSize = parseInt(p.expect(tkIntLit).value) else: discard @@ -1557,7 +1572,7 @@ proc parseExportTo(p: var Parser): Node = discard p.expect(tkTo) let path = p.expect(tkStringLit).value discard p.expect(tkFrom) - let table = p.expect(tkIdent).value + let table = p.expectIdent().value var format = "csv" var delimiter = ',' var includeHeader = true @@ -1565,14 +1580,20 @@ proc parseExportTo(p: var Parser): Node = let kw = p.advance() case kw.kind of tkFormat: - let fmt = p.expect(tkIdent).value.toLower() - format = fmt + format = p.expectIdent().value.toLower() of tkDelimiter: let delim = p.expect(tkStringLit).value if delim.len > 0: delimiter = delim[0] of tkHeader: - let hdr = p.expect(tkIdent).value.toLower() - includeHeader = hdr == "true" or hdr == "yes" + if p.peek().kind == tkTrue: + discard p.advance() + includeHeader = true + elif p.peek().kind == tkFalse: + discard p.advance() + includeHeader = false + else: + let hdr = p.expectIdent().value.toLower() + includeHeader = hdr == "true" or hdr == "yes" else: discard result = Node(kind: nkExportTo, expPath: path, expTable: table, expFormat: format, expDelimiter: delimiter, @@ -1582,7 +1603,7 @@ proc parseExportTo(p: var Parser): Node = proc parseCreateUser(p: var Parser): Node = let tok = p.expect(tkCreate) discard p.expect(tkUser) - let name = p.expect(tkIdent).value + let name = p.expectIdent().value var password = "" var isSuper = false if p.peek().kind == tkWith: @@ -1611,16 +1632,16 @@ proc parseDropUser(p: var Parser): Node = discard p.advance() discard p.expect(tkExists) ifExists = true - let name = p.expect(tkIdent).value + let name = p.expectIdent().value result = Node(kind: nkDropUser, duName: name, duIfExists: ifExists, line: tok.line, col: tok.col) proc parseCreatePolicy(p: var Parser): Node = let tok = p.expect(tkCreate) discard p.expect(tkPolicy) - let name = p.expect(tkIdent).value + let name = p.expectIdent().value discard p.expect(tkOn) - let tableName = p.expect(tkIdent).value + let tableName = p.expectIdent().value var cmd = "ALL" var usingNode: Node = nil var withCheckNode: Node = nil @@ -1652,9 +1673,9 @@ proc parseDropPolicy(p: var Parser): Node = discard p.advance() discard p.expect(tkExists) ifExists = true - let name = p.expect(tkIdent).value + let name = p.expectIdent().value discard p.expect(tkOn) - let tableName = p.expect(tkIdent).value + let tableName = p.expectIdent().value result = Node(kind: nkDropPolicy, dpName: name, dpTable: tableName, dpIfExists: ifExists, line: tok.line, col: tok.col) @@ -1669,9 +1690,9 @@ proc parseGrant(p: var Parser): Node = else: raise newException(ValueError, "Expected privilege in GRANT") discard p.expect(tkOn) - let tableName = p.expect(tkIdent).value + let tableName = p.expectIdent().value discard p.expect(tkTo) - let grantee = p.expect(tkIdent).value + let grantee = p.expectIdent().value result = Node(kind: nkGrant, grPrivilege: priv, grTable: tableName, grGrantee: grantee, line: tok.line, col: tok.col) @@ -1686,19 +1707,19 @@ proc parseRevoke(p: var Parser): Node = else: raise newException(ValueError, "Expected privilege in REVOKE") discard p.expect(tkOn) - let tableName = p.expect(tkIdent).value + let tableName = p.expectIdent().value discard p.expect(tkFrom) - let grantee = p.expect(tkIdent).value + let grantee = p.expectIdent().value result = Node(kind: nkRevoke, rvPrivilege: priv, rvTable: tableName, rvGrantee: grantee, line: tok.line, col: tok.col) proc parseSetVar(p: var Parser): Node = let tok = p.expect(tkSet) - var varName = p.expect(tkIdent).value + var varName = p.expectIdent().value while p.peek().kind == tkDot: discard p.advance() varName.add(".") - varName.add(p.expect(tkIdent).value) + varName.add(p.expectIdent().value) if p.match(tkEq) or p.match(tkTo): discard let valTok = p.peek() @@ -1730,7 +1751,7 @@ proc parseCreateGraph(p: var Parser): Node = discard p.expect(tkNot) discard p.expect(tkExists) ifNotExists = true - let name = p.expect(tkIdent).value + let name = p.expectIdent().value result = Node(kind: nkCreateGraph, cgName: name, cgIfNotExists: ifNotExists, line: tok.line, col: tok.col) @@ -1742,7 +1763,7 @@ proc parseDropGraph(p: var Parser): Node = discard p.advance() discard p.expect(tkExists) ifExists = true - let name = p.expect(tkIdent).value + let name = p.expectIdent().value result = Node(kind: nkDropGraph, dgName: name, dgIfExists: ifExists, line: tok.line, col: tok.col) @@ -1755,7 +1776,7 @@ proc parseCreateDatabase(p: var Parser): Node = discard p.expect(tkNot) discard p.expect(tkExists) ifNotExists = true - let name = p.expect(tkIdent).value + let name = p.expectIdent().value result = Node(kind: nkCreateDatabase, cdDbName: name, cdIfNotExists: ifNotExists, line: tok.line, col: tok.col) @@ -1767,13 +1788,13 @@ proc parseDropDatabase(p: var Parser): Node = discard p.advance() discard p.expect(tkExists) ifExists = true - let name = p.expect(tkIdent).value + let name = p.expectIdent().value result = Node(kind: nkDropDatabase, ddDbName: name, ddIfExists: ifExists, line: tok.line, col: tok.col) proc parseUseDatabase(p: var Parser): Node = let tok = p.expect(tkUse) - let name = p.expect(tkIdent).value + let name = p.expectIdent().value result = Node(kind: nkUseDatabase, udDbName: name, line: tok.line, col: tok.col) diff --git a/tests/bugfix_test.nim b/tests/bugfix_test.nim index c27a8a8..8887474 100644 --- a/tests/bugfix_test.nim +++ b/tests/bugfix_test.nim @@ -250,3 +250,74 @@ suite "Bug fixes — IN list, nkPath exprToSql, multi-table joins": check rmax.success check rmax.rows.len == 1 check valueToString(rmax.rows[0]["m"]) == "30" + +suite "Bug fixes — keyword 'header' usable as column name": + + test "CREATE TABLE / INSERT / SELECT with column named 'header'": + var ctx = setupCtx() + defer: teardown(ctx) + # nimforum schema uses a column named 'header' (tkHeader is the CSV IMPORT keyword) + let c = executeQuery(ctx, parse("CREATE TABLE post (id INTEGER PRIMARY KEY, header TEXT, content TEXT)")) + check c.success + let i = executeQuery(ctx, parse("INSERT INTO post (id, header, content) VALUES (1, 'Hello', 'World')")) + check i.success + let r = executeQuery(ctx, parse("SELECT header FROM post WHERE id = 1")) + check r.success + check r.rows.len == 1 + check valueToString(r.rows[0]["header"]) == "Hello" + let rw = executeQuery(ctx, parse("SELECT id FROM post WHERE header = 'Hello'")) + check rw.success + check rw.rows.len == 1 + + test "IMPORT ... HEADER clause still parses after soft-keyword change": + let ast = parse("IMPORT FROM 'data.csv' INTO post HEADER no") + check ast.stmts.len == 1 + check ast.stmts[0].kind == nkImportFrom + check ast.stmts[0].impHasHeader == false + let ast2 = parse("EXPORT TO 'out.csv' FROM post HEADER yes") + check ast2.stmts.len == 1 + check ast2.stmts[0].kind == nkExportTo + check ast2.stmts[0].expIncludeHeader == true + +suite "Bug fixes — clause keywords usable as identifiers": + + test "columns named after clause keywords (format, status, user, ...)": + var ctx = setupCtx() + defer: teardown(ctx) + let c = executeQuery(ctx, parse(""" + CREATE TABLE kw (id INTEGER PRIMARY KEY, format TEXT, status TEXT, user TEXT, + batch INTEGER, csv TEXT, ndjson TEXT, delimiter TEXT, + migration TEXT, apply TEXT, up TEXT, down TEXT, dryrun TEXT, + policy TEXT, enable TEXT, disable TEXT, recover TEXT, + before TEXT, after TEXT, instead TEXT, of TEXT) + """)) + check c.success + let i = executeQuery(ctx, parse( + "INSERT INTO kw (id, format, status, user, batch, of) VALUES (1, 'csv', 'active', 'admin', 7, 'x')")) + check i.success + let u = executeQuery(ctx, parse("UPDATE kw SET status = 'done' WHERE id = 1")) + check u.success + let r = executeQuery(ctx, parse("SELECT format, status, user, batch, of FROM kw WHERE id = 1")) + check r.success + check r.rows.len == 1 + check valueToString(r.rows[0]["format"]) == "csv" + check valueToString(r.rows[0]["status"]) == "done" + check valueToString(r.rows[0]["user"]) == "admin" + check valueToString(r.rows[0]["batch"]) == "7" + let rq = executeQuery(ctx, parse("SELECT kw.status FROM kw WHERE kw.status = 'done'")) + check rq.success + check rq.rows.len == 1 + + test "IMPORT/EXPORT accept keyword values: FORMAT csv/ndjson/json, HEADER true/false": + let a1 = parse("IMPORT FROM 'd.csv' INTO t FORMAT csv HEADER true") + check a1.stmts[0].impFormat == "csv" + check a1.stmts[0].impHasHeader == true + let a2 = parse("IMPORT FROM 'd.csv' INTO t FORMAT ndjson HEADER false") + check a2.stmts[0].impFormat == "ndjson" + check a2.stmts[0].impHasHeader == false + let a3 = parse("EXPORT TO 'o.csv' FROM t FORMAT json HEADER true") + check a3.stmts[0].expFormat == "json" + check a3.stmts[0].expIncludeHeader == true + let a4 = parse("EXPORT TO 'o.csv' FROM t FORMAT csv DELIMITER ';' HEADER false") + check a4.stmts[0].expFormat == "csv" + check a4.stmts[0].expIncludeHeader == false diff --git a/tests/orc_repro.py b/tests/orc_repro.py new file mode 100644 index 0000000..45d24f7 --- /dev/null +++ b/tests/orc_repro.py @@ -0,0 +1,57 @@ +#!/usr/bin/env python3 +"""Reproducer for the ORC cycle-collector crash (markGray/trace SIGSEGV). + +Usage: + 1. Build the server with ORC: + nim c -d:ssl --threads:on --path:src --mm:orc -o:/tmp/baradadb_orc src/baradadb.nim + 2. Start it: + BARADB_PORT=39472 BARADB_DATA_DIR=/tmp/baradb_orc_data BARADB_LOG_LEVEL=error /tmp/baradadb_orc + 3. Run this script (from the repo root): + python3 tests/orc_repro.py + +Expected (as of 2026-07-30, Nim 2.2.10): the server dies with + orc.nim markGray -> trace -> SIGSEGV +triggered from core/server.nim handleClient, and this script fails with +ConnectionResetError. Under ARC (the default in nim.cfg) the same load passes. + +Bisect results: 200 pings + 200 SELECTs over TCP are fine; the crash lands +somewhere between 20 and 500 sequential INSERTs (INSERT path only). +Failed root-cause attempts: callback cycle breaks (ed5a719), {.cursor.} on +ExecutionContext.registry (the registry<->ctx cycle), guarding ctx.onChange +against zero WS subscribers. Conclusion: deep ORC+async issue (possibly +upstream Nim), not a single app-level cycle. ARC remains the supported MM. +""" + +import asyncio +import sys + +sys.path.insert(0, "clients/python") +from baradb import Client + +PORT = 39472 + + +async def worker(w: int) -> None: + base = 1000 + w * 100 + async with Client("127.0.0.1", PORT) as c: + for i in range(100): + await c.query(f"INSERT INTO orc_stress (id, val) VALUES ({base + i}, 'v{base + i}')") + + +async def main() -> None: + async with Client("127.0.0.1", PORT) as c: + await c.query("CREATE TABLE orc_stress (id INT PRIMARY KEY, val STRING)") + # Original report: SIGSEGV after ~20 sequential INSERTs under async load + for i in range(1000): + await c.query(f"INSERT INTO orc_stress (id, val) VALUES ({i}, 'x{i}')") + r = await c.query("SELECT COUNT(*) AS n FROM orc_stress") + print("after 1000 sequential INSERTs:", r.rows) + await asyncio.gather(*[worker(w) for w in range(10)]) + async with Client("127.0.0.1", PORT) as c: + r = await c.query("SELECT COUNT(*) AS n FROM orc_stress") + print("final count:", r.rows) + print("ping:", await c.ping()) + print("ORC STRESS OK") + + +asyncio.run(main())