Repository navigation
New SQL engine on the Resource API (phases 0-4; phase-5 cutover gated) - #1285
Merged
Merged
Conversation
Contributor
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Contributor
|
Reviewed; no blockers found. |
kriszyp
changed the title
kriszyp
changed the title
Member
Author
Phase 5 (cutover) — progress + a gateBuilt the cutover-readiness differential ( I then trialed the actual flip (default
Both are documented in 🤖 Generated by Claude (Opus 4.8). |
kriszyp
marked this pull request as ready for review
June 30, 2026 11:20
cb1kenobi
reviewed
Jul 9, 2026
kriszyp
force-pushed
the
sql-engine-v2
branch
2 times, most recently
from
July 15, 2026 18:22
ddb6328 to
4878b8c
Compare
Introduce core/sqlEngine/ as a new SQL execution path that maps SELECT
queries directly to Table.search rather than going through AlaSQL's
in-memory execution. AlaSQL stays only as a parser frontend.
Phase 0: scaffolding and router.
The router (sqlEngine/router.ts) selects between the legacy AlaSQL
executor and the new engine via the HARPER_SQL_ENGINE env var
('legacy' | 'new' | 'auto'). 'auto' tries the new engine and falls
back to legacy on EngineUnsupportedError. Default is 'legacy', so
behavior is unchanged unless explicitly opted in.
Phase 1: single-table SELECT pipeline.
parse -> normalize (AlaSQL AST -> internal IR) -> bind (resolve
table/columns/indexes via getDatabases()) -> build logical plan ->
optimize (predicate normalize/pushdown, sort-from-index, limit
pushdown, projection pushdown, scannable validation) -> compile to
physical operators (IndexScan, Filter, Project, Sort, Limit) ->
streaming async-iterable executor. WHERE clauses map to Resource API
condition trees with the canonical comparators (equals/ne/lt/le/gt/
ge/between/starts_with/ends_with/contains); residual predicates that
can't be pushed apply as in-memory Filter operators. JOIN, GROUP BY,
aggregates, subqueries, and mutations are explicitly rejected and
fall back to legacy in 'auto' mode.
Tests: 21 new unit tests cover router dispatch and end-to-end
pipeline behavior against a mock Table; 110 existing legacy SQL tests
continue to pass through the router.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Implements GROUP BY / aggregate execution via PhysicalHashAggregate, in-memory DISTINCT via PhysicalDistinct, and registers COUNT/SUM/AVG/ MIN/MAX/PROD/MEDIAN/MODE/MAD/MEAN. AggCollector in logical/build.ts rewrites aggCall nodes to synthetic column refs so no aggCall escapes above the Aggregate node. Normalizer handles AlaSQL's REDUCE quirk for MEDIAN and promotes registry-known aggregate funcCalls (PROD, MEAN) to aggCall nodes. Optimizer rules recurse through Aggregate correctly. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Implements JOIN execution on the Resource API: INNER/LEFT/CROSS plus comma-separated FROM. RIGHT/FULL rejected with EngineUnsupportedError per the v1 plan. - binder: multi-table alias scope; resolves every column reference to its owning table's canonical alias (rejects ambiguous / unknown qualifiers). - logical/build: left-deep join tree from FROM + joins; WHERE conjuncts split — single-table conjuncts attach to that scan (lowered into the index scan by predicate pushdown), cross-table conjuncts become a residual filter above the join. USING expands to an equi-ON. - row model: join rows are keyed `<alias>.<attr>` (collision-free); a `qualified` flag is threaded through compileExpr and the expression- compiling operators. Single-table queries keep the bare-key path. - physical: PhysicalIndexNestedLoopJoin (default when the inner join key is indexed — the only strategy that satisfies the no-full-scan policy without allowFullScan; probes the inner table per outer row), PhysicalHashJoin (equi, full inner side, maxHashRows cap), PhysicalNestedLoopJoin (CROSS / non-equi). PhysicalQualify re-keys base scans. LEFT OUTER null-fills the right side, matching legacy undefined->null semantics. - optimizer: planJoins rule selects strategy and marks the indexed inner scan so validateScannable treats it as index-served; predicate normalization + pushdown recurse through Join. - PhysicalProject: join output uses clean unqualified names (AS honored), `_2`/`_3` suffix on collision (legacy emitted [id]/[id1]; differential harness will normalize). Deferred to a phase-3 follow-up: PhysicalRelationshipJoin (declared- relationship fast path) and projection pushdown through joins. 14 new join pipeline tests; full sqlEngine suite green (49 passing). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
differential.ts and limitPushdown.ts (from the phase-2 commit) failed prettier --check; reformat so the branch's format:check is clean. No behavior change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…cope) Cross-model review (Codex + domain pass) found three real defects: - LEFT JOIN with a WHERE on the nullable (right) side: single-table WHERE conjuncts were pushed into the inner scan unconditionally, so unmatched left rows got null-filled and wrongly survived (and `WHERE o.col IS NULL` anti-joins lost their rows). Now conjuncts referencing a LEFT-join nullable alias stay as a post-join residual filter, which is correct for both `o.x > k` and `o.x IS NULL`. - Duplicate effective aliases (e.g. unaliased/reused self-join) collided in the `alias.attr` row model, overwriting columns on merge. Now rejected at bind time with EngineUnsupportedError. - Each join's ON was resolved against the full table scope; an unqualified ON column could mis-bind to / be falsely flagged ambiguous against a not-yet-joined table. Now resolved against the accumulated scope (scope[0..i+1]) for join i. Removed the now-stale "rejects JOIN in phase 1" pipeline test (joins are supported; it was passing only via the no-full-scan policy). Added regression tests for the LEFT-join pushdown (filter + IS NULL anti-join) and duplicate-alias rejection. Full sqlEngine suite green (51 passing). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…PLAN.md Records the phase-4 mutation plan and the one blocking unknown: the legacy SQL path writes via harperBridge (not the Resource API), so the blessed transactional-write invocation through a databases-resolved Table needs empirical verification before implementation. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The Gemini review leg (run via agy on stdin) surfaced findings the Codex leg and domain pass missed. Adjudicated and applied the valid ones: - Binder: a base-table-name qualifier that matches more than one table in a self-join now rejects as ambiguous instead of silently binding the first (resolveQualifier). - Binder: an undeclared, unqualified column in a multi-table (join) query now rejects (-> legacy fallback in 'auto' mode) instead of silently defaulting to the FROM table and yielding nulls. Single-table schemaless behavior is kept. - joinAnalysis: rightBaseScan unwraps a stack of Filters (while, not a single if) so indexNL/hash strategies are still detected. - PhysicalHashJoin: exclude NaN from join keys (JSON.stringify would coerce NaN to "null", letting two NaN keys collide and match). Verified false positive (left in place): non-probe equi-pairs in a multi-key indexNL join are already reconstructed into residualOn (physical/plan.ts). Deferred + documented in PLAN.md: single-table predicates in an INNER-JOIN ON clause are not yet pushed into the inner scan; the binder mutates AST in place. 54 sqlEngine unit tests pass (3 new regression tests). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Implements the write path on the Resource API: - normalizer: normalizeInsert/Update/Delete over the AlaSQL ASTs. INSERT is VALUES-form only for now; INSERT … SELECT is rejected (legacy fallback). - binder: bindInsert/Update/Delete resolve the single target table. - executor/runMutation.ts: runInsert/runUpdate/runDelete. The databases-resolved Table class's static get/put/patch/delete/getNewId run inside one transaction(context, …) (the per-row writes join it -> atomic batch). UPDATE/DELETE locate targets via the ordinary SELECT pipeline inside the same transaction. INSERT skips existing-PK rows (get-then-put) to match legacy createRecords skip semantics, and auto-generates a PK when absent. The transaction runner is injectable (_setTransactionRunner) for unit testing. - index.ts: dispatch by statement kind; return the legacy response shapes (inserted_hashes/skipped_hashes, update_hashes, deleted_hashes + messages). The transactional-write pattern was confirmed against resources/Resource.ts, resources/transaction.ts and resources/Table.ts (documented in PLAN.md), not guessed. 8 mutation unit tests added (62 sqlEngine tests pass). Deferred: INSERT … SELECT; unconditional UPDATE/DELETE needs allowFullScan; real-instance behavioral/differential test (separate follow-up). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A behavioral test against a real Harper instance surfaced a bug the mocked unit
tests missed: sqlTranslator/index.ts (processAST) dispatches SELECT with the
bare AlaSQL AST but wraps INSERT/UPDATE/DELETE as { statement, hdb_user } (the
legacy handler arg shape). runStatement normalized the envelope directly and
threw "DELETE requires a table". runStatement now unwraps that envelope.
- index.ts: unwrap the { statement, hdb_user } envelope before normalizing.
- unitTests/sqlEngine/mutation.test.js: make runSql faithful to processAST
(wrap mutations) so this class of bug is caught at the unit level too.
- integrationTests/apiTests/sql-engine-mutations.test.mjs: new behavioral test
that boots a real instance with HARPER_SQL_ENGINE=new and verifies INSERT/
UPDATE/DELETE against real storage (data read back via search_by_hash) plus
the legacy response shapes. 5/5 pass.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Differential integration test boots two real Harper instances (one
HARPER_SQL_ENGINE=new, one legacy) with identical seed data, runs the same
battery of SQL (single-table SELECT, aggregates, INNER/LEFT joins, INSERT/
UPDATE/DELETE) against both, and compares operation responses + persisted state
(audit timestamps stripped). A final test asserts zero divergence; the summary
of any differences is printed.
The differential surfaced one real divergence: the new engine's DELETE message
always said "records", but legacy pluralizes by deleted count ("1 of 1 record",
"0 of 0 records"). Fixed runMutation.ts to match; updated unit + behavioral test
expectations. Result: 17/17 comparisons identical across both engines.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Adds the broad new-vs-legacy differential (auto-mode new side, 30-case battery
covering predicate variety, NULL semantics, ORDER BY/LIMIT/DISTINCT, the full
aggregate set, joins, and mutations) — 30/30 identical.
Attempted the phase-5 default flip (legacy -> auto) and validated against the
existing SQL suite, which exposed two parity blockers the differential battery
didn't cover, so the flip is REVERTED (default stays legacy):
1. Literal type-coercion on hash lookups: `id IN ('123')` (string) vs a numeric
PK matches in legacy but not the new engine — a silent wrong result that auto
does not fall back on. (PLAN decision #6 not yet implemented.)
2. A non-PK `LIKE` DELETE returns 403 via the new selector path (root cause TBD).
Both documented in PLAN.md with an ordered cutover checklist. delete.test.mjs is
76/76 green again under the reverted default.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…lumns)
`… WHERE id IN ('5','6')` against a numeric column matched in legacy AlaSQL but
not the new engine — a silent wrong result that `auto` does not fall back on.
A real-instance probe pinned the behavior: legacy coerces only for `IN` (on both
PK and indexed non-PK attributes), not for single `=` (legacy `id = '5'` also
returns nothing), so `=` is left strict for parity. whereToConditions now expands
each IN literal into its loosely-equal variants (numeric-string <-> number, guarded
by an exact round-trip so '05'/'5px' aren't coerced), keeping each branch an
indexed equality lookup rather than forcing a full scan.
Validated: differential in-str-pk/in-str-nonpk/in-mixed cases (33/33 identical) +
a unit test; the existing delete.test.mjs under auto dropped from 5 failures to 3
(only the unrelated LIKE-DELETE 403 blocker remains).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Clears the two deterministic CI failures on the phase-5 branch:
- runLinter (oxlint): removed an unused `config` import in pipeline.test.js
(eslint(no-unused-vars) error).
- Format Check (prettier): formatted PLAN.md and aggregate.test.js.
Also untracks test_export.json.json — a northwind `export_local` output artifact
that was accidentally committed and rewrites on every test run (it was flagged by
the format check); added to .gitignore.
The remaining red CI shards are pre-existing flakes / runtime limitations, not
regressions from the engine flip: cert-reload #586 (3/6 v22; the SQL differential
passed in that shard), v26-only async-job timeouts (6/6), and a Bun-only
`finishUtf8` gap on a prefix-LIKE blob select (5/6 Bun) — see PR discussion.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
pt2 is a prefix LIKE, which the new SQL engine serves via an index range scan; on Bun that routes blob string-key decoding through ordered-binary's new Function reader and hits its finishUtf8 scope bug (kriszyp/ordered-binary#8). Not a SQL-correctness issue — passes on all Node versions. Guarded with the file's existing bunSkip, consistent with the other finishUtf8-affected cases. Remove once ordered-binary#8 ships. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…etc.) An unaliased aggregate (`SELECT COUNT(*) ...`) surfaced under the engine's internal `__agg_N__` column name instead of legacy AlaSQL's `COUNT(*)`. When the aggregate's predicate is index-served the new engine handles it directly (no legacy fallback), so any consumer keying off `COUNT(*)` — including the new scale.test.ts — got `undefined`. This failed CI shard 4/6 deterministically once main's scale suite merged in. Fix: compute the legacy column label (`COUNT(*)`, `SUM(price)`, `COUNT(DISTINCT x)`, …) from the original expression at normalize time and carry it on ProjectionNode.label (survives the aggregate→column rewrite via the `...p` spread). PhysicalProject now prefers `alias ?? label ?? <fallback>`. The existing differential's aggregate cases all used `AS` aliases, masking the gap; added unaliased `count-bare` / `count-bare-group` / `min-max-bare` cases plus a unit assertion so it can't reopen. scale.test.ts now passes; northwind (562) and the differential (45/45) are green under the cutover default. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…gacy PhysicalHashAggregate ignored the per-aggregate `distinct` flag — `step()` ran for every row — so COUNT(DISTINCT x)/SUM(DISTINCT x) silently counted/summed all rows instead of the deduped set. Confirmed on real engines: COUNT(DISTINCT tag) returned 5 vs legacy's 3. Because an index-served predicate keeps the query on the new engine (no fallback), this was a silent wrong result under the 'auto' cutover default. Until real dedup is implemented, reject distinct aggregates with EngineUnsupportedError so 'auto' falls back to legacy (which dedups correctly). Differential `count-distinct` case + a unit rejection lock it. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…cap fallback, NOT IN 3VL Addresses the phase-5 review findings (cb1kenobi/Barber AI + carried-over claude[bot] hash-join coverage): - limitPushdown (High): don't push LIMIT/OFFSET into a Scan that still carries a residual filter — Table.search would cap rows before the residual runs and silently under-fetch. Keep the Limit above the residual PhysicalFilter. - cap overflow (Medium): the in-memory sort/hash/aggregate/nested-loop row caps now throw EngineUnsupportedError instead of EngineRuntimeError, so 'auto' falls back to legacy (runSelect fully materializes before returning, so nothing is emitted when the cap trips). EngineRuntimeError is now reserved for genuinely non-recoverable runtime failures. - NOT IN (Medium): `col NOT IN (...)` now AND-s an IS NOT NULL guard, mirroring the `!=` path, so NULL rows are excluded to match legacy AlaSQL 3VL. - config.ts (Nit): refresh stale parity numbers (46/46, northwind 575). Tests: hash-join probe/merge + LEFT null-fill coverage (was no-match only); limit-with-residual under-fetch regression; NOT IN NULL-exclusion regression. sqlEngine unit suite 77 passing. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Branch predates #1555, which added oxlint no-restricted-imports rejecting node:assert/strict. Swap the import specifier (node:assert/strict -> node:assert). Call sites are untouched except in sql-engine-differential.test.mjs, whose purpose is exact new-vs-legacy equivalence: its 3 sites use strictEqual/deepStrictEqual to preserve strict comparison after dropping the /strict import. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ing them A mixed predicate like `WHERE status = 'active' AND createdAt < X` (status indexed, createdAt not) failed at runtime: whereToConditions pushed every representable conjunct into the Resource API condition tree, and Table.search then ran searchByIndex on the unindexed createdAt and threw "createdAt is not indexed" — so the UPDATE/DELETE matched 0 rows. Legacy handled it (index-driven scan + in-memory filter), so this was a regression surfaced by bulk-conditional-mutation.test.ts. Make whereToConditions index-aware: a conjunct is pushed only when every attribute it references is indexed (conditionIsPushable); anything else becomes a residual, applied as the existing post-scan PhysicalFilter. Pushability is comparator-agnostic — a full-scan comparator (LIKE->contains/ends_with) on an *indexed* attribute stays pushable as a secondary filter. Scannability (validateScannable R8) keeps the stricter conditionUsesIndex driver check, now shared from whereToConditions so the two decisions can't drift. plan.ts and limitPushdown pass the table attributes through. Verified: bulk-conditional-mutation integration test passes (8/8); sqlEngine unit suite 83/83 incl. new whereToConditions guards. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
#1822) The new SQL engine mapped `WHERE id >= a AND id < b` to two independent single-sided conditions. Table.search leads with the first (`ge`), which range-reads from `a` to the end of the table and filters `< b` in memory — O(table) latency regardless of how narrow the window is. The ops/REST path avoids this by fusing chained bounds; the SQL path never did. whereToConditions now fuses the unambiguous case (exactly one lower ge/gt and one upper le/lt) on the PRIMARY KEY into a single gele/gelt/gtlt/gtle comparator, which resources/search.ts issues as one bounded getRange (start+end) — O(window). BETWEEN already mapped to a single condition and is untouched. Scoped to the primary key on purpose. The PK is always typed, so prepareConditions coerces the fused bounds to the key type and the single seek's start/end always match stored-key ordering. A non-PK indexed attribute may be UNTYPED (e.g. system `hdb_job.start_datetime`), where coercion is skipped: a fused string-literal-vs-numeric-storage seek returns nothing, while the two-condition path tolerates the mismatch via its in-memory filter. Fusing non-PK ranges regressed `search_jobs_by_start_date` (northwind CI) to empty results; gating to the PK fixes it. (Typed secondary-index fusion is a safe future extension but needs attribute-type threading.) Tests: - unitTests/sqlEngine/pipeline.test.js: PK `>=`/`<=` fuses to gele, `>=`/`<` to gelt; a non-PK two-sided range and cross-attribute conjuncts stay unfused. - integrationTests/database/sql-pk-range-fusion.test.ts: on a 60k-row table (HARPER_SQL_ENGINE=new), the explicit two-sided PK range returns exactly the window with ops-`between` parity and runs within a small constant factor of the equivalent BETWEEN. Counterfactually verified: with fusion off the ratio is ~72× (full scan) and the guard fails; with fusion on it is ~1×. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Table.patch cannot re-key a record (identity is the call argument, not the
payload), but runUpdate unconditionally reported the row in update_hashes —
a false success ("updated 1 of 1") while the stored pk stayed unchanged.
Bind-time rejection surfaces EngineUnsupportedError before any planning, so
'auto' falls back to legacy, which declines the re-key ("updated 0 of 1",
skipped_hashes) — restoring exact parity. Found by SQL QA differential wave.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…es; convert runtime search rejections to fallback
The `!=`/NOT IN expansion AND-s a not-null guard ({ne, null}), and IS [NOT]
NULL maps to a null-valued condition — but Table.search throws a raw 400
('not indexed for nulls') for value===null on any index without indexNulls
(the primary key never has it), even when another condition drives the scan.
That error isn't EngineUnsupportedError, so it sailed past the router's catch
and 'auto' leaked a 400/403 where legacy returns rows (F-145, found by the
QA differential fuzzer at seed 566).
Three layers:
- binder: AttributeInfo carries indexNulls (attribute descriptor or index dbi).
- whereToConditions: null-valued conditions are pushable/index-driving only
when the index serves nulls. A `!=`/NOT IN conjunct keeps its value legs
pushed (they carry the quoted-boolean/numeric coercion branches the
expression evaluator doesn't replicate) and re-expresses just the guard as
a residual IS NOT NULL filter (3VL-correct post-scan).
- PhysicalIndexScan: safety net — any remaining capability-shaped ClientError/
IndexRebuildingError from search converts to EngineUnsupportedError, so the
fallback contract holds even if plan-time prediction drifts from search
again.
Anchors: 3 pk-shape F-145 cases in the differential battery (50/50 identical),
pushed-vs-residual guard unit tests for both indexNulls states (88 passing).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…(F-146) runUpdate read each matched row once, computed absolute new values in JS, and patched them — a read-compute-write that silently loses increments when two UPDATEs race on the same row (QA fuzzer/concurrency wave saw ~49% loss on `SET qty = qty + 1` under contention; single-node legacy happened not to, but that's statement serialization, not real CAS — multi-node legacy loses too). A self-referential `col = col + N` / `N + col` / `col - N` by a numeric literal is now emitted as `new Addition(delta)`, the same CRDT primitive REST/ops addTo uses, so the storage layer applies it atomically against the committed record. Non-self-referential and row-dependent assignments keep the absolute path. Verified: concurrent 60× `qty = qty + 1` loses 0 (tracked integration anchor); QA-568 probe 2 now TOTAL lost=0 (was ~49%); differential 50/50 identical (single-statement parity preserved); 89 unit passing. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…-219) An ORDER BY on an indexed attribute with no index-driving WHERE was rejected by validateScannable (R8) and fell back to legacy's full-scan+in-memory-sort, contradicting the docs' 'ORDER BY on an indexed attribute streams from index order' promise. Table.search already aligns such a sort to the index (adds a comparator:'sort' condition), so the rows stream ordered and a pushed LIMIT early-terminates. - New shared sortDrivesIndex() (whereToConditions) used by both validateScannable and the physical plan builder so scannability and the allowFullScan flag can't drift. - validateScannable accepts a scan whose pushed sort is on an indexed attribute. - PhysicalIndexScan gains an allowFullScan option; plan.ts sets it true for a sort-driven scan (Table.search flags the sort scan needFullScan and would otherwise reject it under allowFullScan:false — the 403 the QA fuzzer saw). Verified O(window): ORDER BY <indexed> LIMIT 20 medians 2.4/2.4/2.0ms across 3k->24k rows (0.82x growth over 8x table) under forced HARPER_SQL_ENGINE=new — the LIMIT early-terminates, not a relabeled full scan. Differential 52/52 identical (correctness); 90 unit passing. GROUP BY streaming (Aggregate sits directly over the Scan, no pushed sort) is a separate follow-up. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…swept expired rows QA-269's write-surface TTL matrix showed SQL UPDATE failing to reset TTL while REST PUT/PATCH, ops update, and addTo all correctly reset it. Root cause: UPDATE and DELETE locate their target rows via Table.search() (a plain SELECT), which lazily evicts and drops any row whose TTL has already passed but hasn't been swept by the background eviction scan yet — correct behavior for a read, but wrong for a mutation that's about to overwrite that row's TTL anyway. The other write surfaces never hit this because they load the record by id directly, bypassing the freshness check a read applies. If the round trip to locate an UPDATE's target row landed even slightly after the row's natural TTL, the row silently vanished from the result set, so the UPDATE touched nothing and the TTL never reset — a real, if narrow, race that widens under load or extra per-statement overhead. Fix: thread an opt-in `includeExpiredRows` flag through SqlEngineContext down to Table.search() (`target.includeExpired`), set only by runUpdate/runDelete's row-finder selects, so a not-yet-swept expired row is treated as a live match the same way a direct by-id write already is. Existing SELECT callers are unaffected (flag defaults off). Also narrowed runUpdate's selector to project only the primary key plus whatever columns a row-dependent SET assignment actually reads, instead of unconditionally fetching the whole row — matching legacy's SQL UPDATE and this module's own runDelete, and cutting unnecessary round-trip latency that was widening the race window. Verified: QA-269 passes on both rocksdb and lmdb, repeatedly, including under an artificially tightened TTL margin that reliably reproduced the original failure pre-fix. sql-engine-differential and sql-engine-mutations integration suites, plus unitTests/sqlEngine (91 tests, +1 new covering a cross-column relative assignment) and unitTests/resources all pass with no regressions. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
….md cheat sheet Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…imary key (D-219) D-219 let any indexed sort attribute drive a no-WHERE ORDER BY via the index's natural order. That is only correct for the PRIMARY KEY: a non-PK secondary index diverges from legacy's full-scan-then-stable-sort in two ways — a DESC sort reverses the composite [value, primaryKey] entry (ties come back primary-key-DESC vs legacy's stable primary-key-ASC), and the secondary index omits null-valued rows. This surfaced after the rebase as two northwind failures (single-key `ORDER BY <secondary> DESC`); their two-key siblings pass because R7 only pushes single-key sorts. sortDrivesIndex now requires the sort key to be the primary key (unique → no ties, always present → no null omission), so its ordered scan matches legacy in both directions; secondary-index sorts fall back to legacy as before D-219. Also adds the two test-coverage items flagged in review: - pipeline.test.js: force search() to throw ClientError / IndexRebuildingError and assert PhysicalIndexScan converts both to EngineUnsupportedError (and 'auto' falls back to legacy); plus a generic error still propagates unconverted. - ttl.test.ts: a short-TTL row past expiry (held physically present by a 1h eviction window) — a plain SELECT hides it while SQL UPDATE/DELETE find it via includeExpired (QA-269). Distinct ids per leg since a plain SELECT lazily evicts the row it reads. Refs #1285 Co-Authored-By: Claude Opus <noreply@anthropic.com>
cb1kenobi
approved these changes
Jul 17, 2026
cb1kenobi
left a comment
cb1kenobi
left a comment
Member
There was a problem hiding this comment.
Reviewed and tested. Pretty clean implementation. Nice fallback support. I'm good with this PR.
Co-Authored-By: Codex <noreply@openai.com>
The deterministic indexed query still has ten matches; the missing row came from starting 1,100 asynchronous fixture writes while waiting only for the final returned promise. Await every write so the optimizer assertions run against the complete fixture instead of changing their expected behavior. Co-Authored-By: Codex <noreply@openai.com>
Member
Author
SQL engine performance: new vs legacyPLAN.md phase-5 item 6 ("a subset of the legacy corpus run with timings; budget ≤ legacy time") didn't have a harness yet, so I built one and ran it against this branch at Result: every measured query shape is faster on the new engine — 16/16, nothing slower, zero result mismatches. Geomean ratio 0.148 at 40k rows (~6.8× faster overall). The budget is met comfortably. MethodEngine selection is process-global ( Two details worth calling out, because they're the difference between a real measurement and a flattering one:
Results — 40k rows, p50, 25 timed iterations per query per engine
Two caveats on reading these numbers1. The ratio is a function of table size, not a fixed speedup. Legacy full-scans anything it can't serve from an index, so its cost grows with the row count while the new engine's index-served plans stay flat. Same 10-row query, three table sizes:
That's the actual story of this PR — not "6.8× faster" but "O(index) instead of O(table)". The geomean will keep climbing with scale, so it should always be quoted with a row count. 2. Anything near the 0.441ms floor is transport-dominated. The tiny-query risk didn't materializePLAN.md flagged "a streaming pipeline has more per-row overhead than AlaSQL on inputs that already fit in memory" as a cutover risk. It doesn't show up: the smallest shapes ( One finding:
|
ORDER BY id LIMIT 20 |
1k rows | 10k rows | 40k rows |
|---|---|---|---|
| legacy | 5.18ms | 34.72ms | 141.14ms |
| new | 3.89ms | 29.67ms | 118.53ms |
D-219 intends this to be O(window). The cause is an interaction between the two D-219 commits:
- 7a919c2 added the index-order scan and verified O(window) using
ORDER BY <indexed>— a secondary index. That genuinely works; I measured it flat at 1.27ms → 1.13ms across an 8× table growth. - 50f917b then correctly restricted
sortDrivesIndexto the primary key, because a secondary index diverges from legacy on DESC ties and null omission.
The catch is that Table.search only adds its order-aligned pseudo-condition when the sort attribute has a secondary index — and the primary key doesn't have one. So the rule now fires on exactly the one attribute kind search can't serve from index order, and always degrades to full scan + in-memory sort. Driving Table.search directly (no SQL) shows it plainly:
GET /sqlbench/?sort(id)&limit(20) -> 404 "id is not indexed and not combined with any other conditions"
GET /sqlbench/?sort(qty)&limit(20) -> 1.2ms flat across 5k -> 40k
Worth flagging: plan.ts sets allowFullScan for a sort-driven scan, and that's what suppresses this 404 — so the new engine was quietly converting a query search would have rejected (→ legacy fallback) into a permitted full scan.
The fix belongs in Table.search, not sortDrivesIndex — the PK-only restriction in 50f917b is right; what was missing is search's ability to honor it. Opened separately as #1844 since it's core search semantics and independent of this PR. With it, on this branch:
ORDER BY id LIMIT 20 @ 40k |
legacy | new | ratio |
|---|---|---|---|
| before #1844 | 142.15ms | 117.99ms | 0.830 |
| after #1844 | 141.32ms | 1.58ms | 0.011 |
Flat across table growth, and the battery geomean goes 0.148 → 0.111. None of the numbers in the main table above include that fix — they're what this PR delivers on its own.
Benchmark code
benchmarks/sql-engine/ (runner + fixture + README), following the existing indexed-write / concurrent-rw pattern and emitting the same machine-parseable SQL_ENGINE_RESULT / SQL_ENGINE_SUMMARY lines for a future regression gate. Coming as a follow-up PR against main so it doesn't hold this one up:
node benchmarks/sql-engine/run.mts # quick (~2 min)
node benchmarks/sql-engine/run.mts --scale=nightly
—
🤖 Benchmarked and written by Claude Opus 4.8 via Claude Code
kriszyp
added a commit
that referenced
this pull request
Jul 22, 2026
Table.search only added the order-aligned pseudo-condition when the sort
attribute carried a *secondary* index. The primary key has none, so sorting by
it with no other condition was rejected:
GET /MyTable/?sort(id)&limit(20)
404 "id is not indexed and not combined with any other conditions"
The primary store is itself keyed in primary-key order, so a scan of it is
already aligned with the sort — there is nothing to reject. Sorting by primary
key is about the most natural sort a table has, and it was the one sort that
did not work.
With the pseudo-condition in place the scan streams in primary-key order and a
pushed limit early-terminates, so `sort(<pk>)&limit(n)` becomes O(window)
instead of a full scan plus an in-memory sort. The primary key is unique and
always present, so neither hazard that applies to secondary-index sorts (DESC
reversing composite [value, primaryKey] ties; the index omitting null-valued
rows) applies here.
Found while benchmarking the new SQL engine (#1285), where `ORDER BY <pk>
LIMIT 20` was the one shape that scaled linearly with table size on both the
new and legacy engines. On that branch this takes the new engine's
ORDER BY <pk> LIMIT 20 from 118ms to 1.6ms at 40k rows and makes it flat across
table growth; the benchmark is a follow-up PR.
Verified: rest.test.mjs regression tests for ASC / DESC / offset (all three fail
with the exact 404 without this change); ASC, DESC, offset and DESC+offset each
match a locally computed expectation over a 3k-row table.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A new SQL engine built directly on the Resource API (
Table.search/get/put/patch/delete/transaction), replacing the AlaSQL two-pass hybrid (dataLayer/SQLSearch.js) that degrades to full attribute scans onOR/comparators and materializes all fetched rows for aggregates/sort. AlaSQL is kept only as a parser. Full design + phasing insqlEngine/PLAN.md.Behind a feature flag —
sql.engine = legacy | new | auto(alsoHARPER_SQL_ENGINE). Default isauto(phase-5 cutover): the new engine handles every request it can plan and falls back to legacy onEngineUnsupportedErrorfor anything it can't — so no query changes behavior unless the new engine produces an identical result.allowFullScanstaysfalse(genuine full scans fall back to legacy).Status by phase
transaction(context, …). Legacy response shapes preserved.INSERT … SELECTand no-WHERE UPDATE/DELETE fall back to legacy.legacy → auto; legacy path untouched and reachable via the flag. Gated on full parity (see below). Burn-in (watchsql-engine v2 fallback:logs) → flip tonew→ delete legacy path are follow-ups in later releases.allowFullScanparity gaps. Tracked in New SQL engine: phase 6 (broaden coverage beyond 5.2 cutover) + parity-gate hardening #1525; 5.3+, not 5.2.Cutover parity (the gate for the
autodefault)integrationTests/apiTests/sql-engine-differential.test.mjs): boots anew-side (HARPER_SQL_ENGINE=auto, the production setting) and alegacy-side with identical data, compares response + persisted state across a broad battery (SELECT, OR/NOT/BETWEEN/LIKE/IN, NULL semantics, ORDER BY/LIMIT/OFFSET/DISTINCT, aggregates incl. bare/unaliased + COUNT(DISTINCT), INNER/LEFT join, INSERT/UPDATE/DELETE). Because the new side runs inauto, an unsupported query falls back and matches by construction — the only way to fail is a silent divergence, which is the real cutover risk. 46/46 identical.autodefault: northwind (575) and delete.test — 0 failures.Bugs fixed in this PR's cutover pass
Two silent-divergence bugs that the prior parity gates missed because the differential's aggregate cases all used
ASaliases:SELECT COUNT(*) …surfaced under the engine's internal__agg_N__name instead of legacy'sCOUNT(*). Index-served aggregates run on the new engine (no fallback), so a consumer keying offCOUNT(*)— including main's newscale.test.ts— gotundefined(failed CI shard 4/6). Fix: compute the legacy label at normalize time ontoProjectionNode.label;PhysicalProjectprefersalias ?? label ?? fallback.PhysicalHashAggregatenever readspec.distinct, soCOUNT(DISTINCT x)counted every row (real-engine: 5 vs legacy 3). Rejected withEngineUnsupportedError→ legacy fallback until real dedup lands (New SQL engine: phase 6 (broaden coverage beyond 5.2 cutover) + parity-gate hardening #1525).Parity gate hardened: added bare/unaliased + COUNT(DISTINCT) differential cases and unit regressions so the blind spot can't reopen.
Tests
unitTests/sqlEngine/— per-operator + pipeline + join + mutation + aggregate suites (mock-table harness).integrationTests/apiTests/sql-engine-differential.test.mjs— new-vs-legacy parity (46/46).autodefault: northwind (575) + delete +integrationTests/server/scale.test.ts(100K) all green.Deferred (fall back to legacy; tracked in #1525)
PhysicalRelationshipJoin; projection pushdown through joins; single-table predicates in an INNER-JOINON;INSERT … SELECT; no-WHERE UPDATE/DELETE; real DISTINCT-aggregate dedup;allowFullScan:trueparity gaps. None affect 5.2 (all fall back).🤖 Generated by Claude (Opus 4.8).