Bounded query execution: signal threading, streaming scanColumn, execution budget with typed refusal (LLP 0054/0058/0059) - #1075
Draft
philcunliffe wants to merge 20 commits into
Draft
Conversation
Mint the missing `design` LLP for the /v1/query OOM kernel-fix package. The package shipped spec (0054) + decisions (0055/0056) + prose plan (0057) with no design-type doc, so `neutral backlog` flagged 0054 as needing a design. LLP 0058 ties the spec to a concrete technical design grounded in the real query kernel: the buffering OOM site at `src/core/query/sql.js` (`squirrelExecuteSql` called with no signal, then `collect()`), the dead `context.signal` abort path, the dormant `scanColumn` aggregate fast path no kernel source implements, and the display-only `ContextControls` budget. It lays out signal threading, streaming aggregates via `scanColumn` down the source stack (per 0055), and the refuse-over-budget execution budget (per 0056), citing the decisions rather than restating them. @ref LLP 0054 (implements, coverage) / 0055, 0056 (constrained-by). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Mints the neutral executable plan for the /v1/query OOM kernel fix change set: a ## Tasks breakdown that extends the prose plan LLP 0057 and implements the technical design LLP 0058. Nine independently mergeable tasks across the three design mechanisms (signal threading, scanColumn down the source stack, execution budget with refusal), their two upstream engine PRs (icebird, squirreling), caller wiring, and an end-to-end memory-invariant smoke. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…T1, LLP 0059)
executeQuerySql now composes the caller-supplied `signal` with an optional
relative `timeoutMs` deadline (via AbortSignal.timeout/AbortSignal.any) and
forwards it as squirrelExecuteSql({ tables, query, signal }) at the call site
that previously omitted it. squirreling@0.12.24 already threads `signal` through
`context.signal` to its blocking operators (sort.js, aggregates.js) and the leaf
parquet scan honors it at the row-group boundary, so this wiring is the whole
fix: a long or runaway query can now be torn down mid-scan instead of running to
completion. This is the abort enabler only; it bounds nothing on its own (the
execution budget and refusal are T6/T7).
Add `signal?` and `timeoutMs?` to ExecuteSqlOptions. Annotate the call site with
@ref LLP 0054#signal-threading [implements].
Tests (test/core/query-sql-signal.test.js): a caller-aborted signal tears down a
running query mid-scan; an already-aborted signal stops it before completion; a
timeoutMs deadline aborts a slow query; and a normal no-signal query still
returns all rows (backward compatible).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
unionSources now exposes scanColumn when every partition supports it, concatenating per-partition column streams in source order. limit/offset are stripped from the per-partition scanColumn calls (not distributive across a concatenation, same discipline the row scan already applies at union-source.js:47) and re-applied by the union itself over the merged stream, since scanColumn has no appliedLimitOffset escape hatch. A union with any partition lacking scanColumn omits it entirely so the engine falls back to the buffering path rather than silently undercounting. emptySource gains a scanColumn that yields an empty stream. @ref LLP 0055 [implements] Task-Id: T3
Forward scanColumn to the wrapped source in the ai-gateway withSchemaColumns wrapper so the engine's streaming column-scan aggregate fast path can light up over the ai_gateway_messages dataset (LLP 0055). A requested column present on the wrapped source streams through unmodified; a column absent from the wrapped source's physical schema (the same additive schema-drift case the row scan already tolerates, LLP 0032#capture) null-fills in bounded chunks instead of throwing, honoring limit/offset/signal. scanColumn is only exposed when the wrapped source itself exposes one, so a source with no streaming capability still falls back to the buffering scan path untouched. Unit-tested: a present column streams through, an absent column null-fills (including under limit/offset), and scanColumn never throws on drift. Task-Id: T4
Bumps the kernel's pinned `icebird` dependency to 0.8.12, the upstream
release that adds `scanColumn({ column, limit, offset, signal })` to the
Iceberg `AsyncDataSource` icebird's `icebergDataSource` factory returns,
streaming a single column's values row-group chunk by row-group chunk
and honoring `signal` for mid-scan abort (LLP 0054#signal-threading).
This exact icebird release matches the kernel's pinned squirreling@0.12.24
`AsyncDataSource.scanColumn?` contract (a plain
`AsyncIterable<ArrayLike<SqlPrimitive>>`) with no unwrap needed, so the
version bump itself is the whole kernel-side change: no wrapping or
adaptation code is required at the call site.
Annotate `dataSourceForTable` in src/core/cache/iceberg/store.js (the
kernel source factory that calls `icebergDataSource`) with
`@ref LLP 0055 [implements]`, per LLP 0055's streaming-column-scan
decision and LLP 0058's design. This is an enabler only: nothing yet
forwards `scanColumn` through the core union (union-source.js, task T3)
or the ai-gateway schema wrapper (dataset.js, task T4), so squirreling's
`tryColumnScanAggregate` fast path stays dark until those land (task T5).
Tests (test/core/cache-iceberg-scan-column.test.js) build a real local
Iceberg table through the cache spool/flush path and drive the pinned
icebird's `scanColumn` directly: row-order correctness, limit/offset
slicing, and an already-aborted signal rejecting the scan. Upstream
icebird carries its own scanColumn correctness tests; these prove the
kernel's consumption point works end to end against the real pinned
dependency.
Task-Id: T2
Co-Authored-By: Claude <noreply@anthropic.com>
Bump the pinned squirreling dependency 0.12.24 -> 0.16.0 (deliberate exact-version bump, never a caret) to consume the upstream execution budget change. Inside the three blocking operators (sort.js's push loop; aggregates.js's scalar slow-path group and the high-cardinality hash/distinct paths) squirreling now tracks running buffered-row and buffered-byte counts and, when either would exceed the per-run ceiling, aborts and raises a distinct QueryBudgetExceededError value carrying the limit hit and the operator that hit it, returning no rows. This is a refusal, not a truncation: a partial COUNT(DISTINCT)/GROUP BY undercounts, and an ORDER BY prefix is only correct after full buffering. The upstream change ships its own budget.js accountant (BufferBudget/QueryBudgetExceededError), an ExecutionBudget option on ExecuteSqlOptions/ExecuteContext, and @ref LLP 0056 [implements] annotations at each budget-check site, plus upstream tests for the threshold and the error shape (18 new tests, full upstream suite still green: 1848 passed). No hypaware kernel code consumes the new budget option yet; wiring ExecuteSqlOptions.budget through executeQuerySql and re-exporting QueryBudgetExceededError is T7. Task-Id: T6
… into HEAD # Conflicts: # package.json
Add ExecutionBudget (buffered-row ceiling, estimated buffered-byte ceiling, whichever trips first) to ExecuteSqlOptions, distinct from the display-only ContextControls (maxCell/maxBytes). sql.js forwards the budget into squirrelExecuteSql alongside the T1 signal and defaults it to a conservative safe-low ceiling pending the LLP 0057 Phase 0 measurement. Re-export QueryBudgetExceededError (from the T6 squirreling pinned bump) through hypaware/core/query so callers can catch a refusal without importing the engine directly. @ref LLP 0054#execution-budget [implements] Tests: refuse at the row ceiling and the byte ceiling with the typed error shape (operator/limitKind/limit/observed) against a small fake data source with a deliberately low budget; confirm a query safely under budget still returns every row; confirm the kernel default budget doesn't interfere with a small, un-configured query. Note: `npm run typecheck` still reports 10 pre-existing errors in src/core/query/union-source.js and its tests (T3/T4 scope), from squirreling@0.16.0's scanColumn returning `AsyncIterable | ScanColumnResults` where T3's union forward assumed a plain AsyncIterable. Confirmed present on origin/integration/bounded-query-execution before this commit and untouched by it; none of the new errors are in files this task touches. Task-Id: T7
… stack With scanColumn wired down the real source stack (icebird leaf via T2, core union via T3, ai-gateway schema wrapper via T4), squirreling's dormant tryColumnScanAggregate fast path now fires for COUNT/MIN/MAX/SUM/AVG and COUNT(DISTINCT low-card) instead of bailing at the `!table?.scanColumn` guard. Add kernel integration tests (test/core/query-column-scan-aggregates.test.js) that run real SQL through executeQuerySql over the real unionSources + withSchemaColumns composition (matching ai-gateway's createDataSource wiring exactly), proving: the row-buffering scan() path is never invoked, each column is scanned exactly once per partition regardless of how many aggregates read it, COUNT(DISTINCT session_id) returns the exact cardinality, and the largest chunk ever materialized stays bounded to a fixed constant regardless of total row count — the structural proof of O(1)/O(cardinality) memory, not O(rows). Fix a real type-narrowing gap surfaced by consuming the actually-pinned squirreling@0.16.0: its AsyncDataSource.scanColumn return type widened to a union (bare AsyncIterable, still what icebird@0.8.12 yields, or the newer ScanColumnResults `.chunks()` wrapper) after T3/T4 were written against squirreling@0.12.24's single-shape contract. Add a small columnChunks normalizer in union-source.js so the union concatenates either shape uniformly, plus a test proving it against a source that returns the newer shape. Apply the same normalization at the three existing test call sites (ai-gateway-dataset, cache-iceberg-scan-column, union-source) that iterate a scanColumn result directly, so `npm run typecheck` passes against the pinned engine. Task-Id: T5
hyp query sql passes no explicit budget, so it (and the query_sql MCP tool, which shares the same querySqlVerb.operation -> executeQuerySql call) already inherits the host-default execution budget wired in T7 (LLP 0054 #uniform-surface). Both surfaces were already correctly plumbed generically: the CLI's runVerbCommand turns any thrown operation error into a stderr line plus a non-zero exit, and the MCP host's tools/call handler already turns a thrown operation into an isError tool result. Documented that inheritance in verb.js with an @ref, and added CLI- and MCP-level tests (using the real querySqlVerb) that exercise a refusal at the real host-default row ceiling end to end, proving the refusal renders as a stderr message + exit 1 on the CLI and as an MCP tool error (never a silent empty/partial result) on the MCP tool - not just against a synthetic verb. Exported DEFAULT_EXECUTION_BUDGET from sql.js (kernel-internal, not re-exported through index.js) so those tests can size their fixture to the same ceiling an un-configured caller actually runs under. Around the query.execute_sql span, catch QueryBudgetExceededError specifically and emit a query.budget_exceeded log line plus span attributes (component, operation, error_kind: budget_exceeded, the refusing operator, limit_kind, limit, and observed buffered-row/byte high-water mark) so the refusal is greppable off the logs/trace on every caller that shares this one executeQuerySql implementation. Server-side wiring (mapping the refusal to a 4xx, the operator- configured budget) stays out of scope here; tracked in server LLP 0020. Task-Id: T8
…T9, LLP 0059) Adds the end-to-end gate that proves LLP 0054's #memory-invariant holds across every mechanism the bounded-query-execution change set landed: signal threading (T1), scanColumn streaming (T2-T5), and the execution budget with typed refusal (T6-T8). Unlike the tiny synthetic ceilings the unit/kernel-integration tests use (query-sql-budget.test.js, query-column-scan-aggregates.test.js), this smoke drives the REAL, unconfigured host-default execution budget (DEFAULT_EXECUTION_BUDGET, T7) over a fixture shaped and sized like the actual ~495k-row ai_gateway_messages crasher from hyparam/hypaware-server#9, composed through the real production stack (withSchemaColumns(unionSources(...))), the same way T5's own test proves the streaming path but now asserting the acceptance-level contract: - ORDER BY over the full dataset refuses with a typed QueryBudgetExceededError (operator ORDER BY) instead of buffering every row and OOM-crashing the process. - COUNT(DISTINCT content_text) (high-cardinality) also refuses (operator COUNT(DISTINCT)) even on the streaming column-scan fast path, since its dedup set is charged per distinct value. - COUNT(DISTINCT session_id) (low-cardinality) completes over the full scan with the exact cardinality and bounded heap growth, proving O(cardinality) memory, not O(rows). Each refusal's `observed` high-water mark is asserted to trip right at the ceiling, not anywhere near the full row count - the deterministic, GC-independent proof of boundedness - backed by a coarse process.memoryUsage() heap-growth check as a secondary signal. The assert_telemetry step reads the T8 budget/refusal log/trace records directly (error_kind: budget_exceeded, operator, limit, observed) so the internal path is proved, not just the process's return/throw behavior. Registered in the README V1 smoke battery (the flow index). Task-Id: T9
philcunliffe
added
the
neutral:stuck
Contributor
Author
Neutral is stuck on this PR: it needs a human decisionWhat neutral was doing. The Why it cannot proceed. The merge was aborted and nothing was pushed. This is not a mechanical conflict: master independently shipped this change set's entire spec under a different, later design, so resolving the ten conflicted files would mean choosing between two designs, not reconciling two edits. Verified against
Conflicted files: What neutral needs from you. Which of these is the intended outcome:
Under (a) or (b) the branch's LLP docs need renumbering off 0058/0059 regardless, since master has taken those numbers. How to unstick. Reply with a comment on this PR (or push to the branch). Neutral monitors this thread and will re-engage with your guidance on its next tick. |
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
1 participant
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.
Implements the
bounded-query-executionchange set: the kernel/v1/queryOOM fix across the kernel query plane and its two first-party upstream engines.Design of record:
llp/0058-bounded-query-execution.design.md(implements spec LLP 0054, constrained by LLP 0055/0056). Executable plan:llp/0059-bounded-query-execution.plan.md.What landed
src/core/query/sql.jsconstructs anAbortSignal(linked to any caller-supplied signal and an optional deadline) and forwards it intosquirrelExecuteSql.@ref LLP 0054#signal-threadingscanColumn: pinned bumpicebird@0.8.11to0.8.12, single-column row-group streaming that honorssignal.scanColumnforward insrc/core/query/union-source.js: per-partition column streams concatenated,limit/offsetre-applied by the engine over the merged stream.withSchemaColumnsscanColumnforward: an absent column null-fills rather than throwing, matching the additive schema-drift rule its rowscanalready applies.tryColumnScanAggregatestops bailing, so scalar aggregates hold O(1) rows andCOUNT(DISTINCT low-card)holds O(cardinality).QueryBudgetExceededErrorat the ceiling (a refusal, not a truncation).ExecuteSqlOptions, distinct from the display-onlyContextControls.query.execute_sqlspan.bounded_query_refusalacceptance smoke: drives the known issue-hy-2xeb: Install pipeline for GitHub-URL plugins (hy-gh-1) #9 crashers and asserts a clean typed refusal with bounded heap, plus a now-streamingCOUNT(DISTINCT session_id).Change-Set: bounded-query-execution