Use native prepared batches through query sources

[platypii]

Summary

• update Icebird to 0.8.23 and Squirreling to 0.16.1
• preserve schema-addressed prepared scans through storage, compatible partition unions, AI gateway schema wrapping, and heap-budget enforcement
• retain correctness-preserving row fallbacks for schema drift and row-level visibility, documented in LLP 0294

Benchmarks

Real local cache using hyp query overview --json --days 7 --include-local-only, with one warmup followed by five measured runs:

	Samples (ms)	Median
Before	3541.3, 3716.6, 3804.2, 3628.2, 3606.8	3628.2 ms
After	1943.2, 1957.2, 1940.5, 1965.0, 1980.7	1957.2 ms

Median runtime dropped by 46.1%, a 1.85x speedup. Every run returned the same 7,740-byte JSON result, and the before/after SHA-256 digest was identical: 163d3c7e083640d464fe1d2c40be1d13b66d05581d66cc4a8bbafcacb7d5d8e0.

Validation

• npm run typecheck
• npm run build:types
• focused query, connector, and LLP suite on Node 24: 95 tests passed
• npm test: 4,914 passed and 2 skipped; one unrelated concurrent telemetry test was flaky under aggregate load and passed in isolation
• CI tests and typechecks pass on Node 22 and Node 24; LLP duplicate and cross-branch number checks pass
• no lint script is defined in this repository

[philcunliffe]

neutral review: update/icebird-squirreling-native-batches @ d6f350d2

Verdict: changes requested (all findings are Low/Medium quality gaps, none are correctness regressions in the new code).

This is a careful, well-documented change. The dependency pins are real, the call sites match the installed APIs, and the union's conservative gating is the right shape. What holds it back is a capability silently dropped on one of the new branches, two new branches with no test, one missing @ref, and one user-visible consequence of the wrapper split that deserves an explicit decision.

Verification I ran (in a detached worktree at d6f350d2)

• Pins exist on npm. icebird@0.8.23 and squirreling@0.16.1 both resolve (0.16.1 is the current squirreling head). No lockfile in the repo, so nothing else to bump.
• No API drift at the call sites. Every type the diff now imports from 'squirreling' exists in the installed 0.16.1 src/types.d.ts: PreparedScan (147), ScanRequest (129), ScanProperties (136), ScanResidual (141), ColumnDemand (122), RelationSchema (36), AsyncCells (207), and the widened AsyncDataSource union (222) that makes columns/scan() optional for a prepared-only source. rowsToBatches is a real named export of squirreling's index.
• npm run typecheck clean, npm test green (4488 pass, 1 skipped).
• Differential probe. Built two real Iceberg partitions through storage.dataSourceForTable + unionSources and ran count(*), WHERE, LIMIT/OFFSET, and SELECT * on the prepared path against the same union with schema/prepareScan stripped. Identical results on the clean fixture.
• Two pre-existing bugs are fixed by this PR. After deleteMatchingRows (the hyp purge path) on one partition of a 6-row two-partition fixture: select count(*) returns 5 on this branch and returned 3 on master (numRows ?? 0 swallowed the unknown count); select id where k='x' returns [1,3,4,6] here and returned [1,3,4] on master.
• Smokes. cache_roundtrip passes. local_only_query_withhold and context_graph_projects_rows fail identically on origin/master in this sandbox, so they are environmental, not caused by this PR.

Findings

F1 (Medium, design consequence). After this PR the same query can return different row sets to different caller classes on a purged table.
src/core/query/visibility.js:124 builds guarded with only columns and scan, deliberately not forwarding prepareScan (correct per LLP 0266#transparent-wrappers). src/core/query/union-source.js:186 lights the prepared path for a top-of-lattice caller. The two now disagree, because icebird's row scan() applies position deletes against the filtered row sequence rather than physical row positions. Isolated repro against icebird@0.8.23 on a single partition holding ids 4/5/6 with id 5 position-deleted:

icebird scan() with WHERE k='x': [4]         appliedWhere=true   <- wrong, id 6 is lost
icebird scan() with no WHERE:    [4, 6]                          <- correct

The prepared path dodges this because icebergDataSource.prepareScan().batches() delegates to scan() with no where when hasDeletes (node_modules/icebird/src/sql/icebergDataSource.js:143-147). So:

• top-of-lattice caller, aligned schemas: prepared path, correct rows;
• restricted caller (visibility wrapper) or drifted-schema union: scan() path, rows silently dropped.

The bug is upstream and pre-existing (reproduced identically on 0.8.22), so this PR is not the cause and is a net improvement. But the PR is what makes the two paths disagree, and LLP 0266#schema-drift explicitly leans on "the row path remains authoritative" while that row path is currently lossy in exactly the purge case LLP 0104 cares about. Ask: file the upstream icebird issue and record the exposure in LLP 0266 (or in 0261/0104), so the fallback is not documented as a clean safety net when it is not one. Bonus: this would make a good addition to test/core/iceberg-source-parity.test.js, which is the differential harness built for precisely this class of divergence.

F2 (Low, defect in new code). The prepared-only branch of withHeapBudget drops scanColumn.
src/core/query/sql.js:153-162 returns a fresh object carrying only numRows, columns, schema, and prepareScan. A third-party source that implements prepareScan + scanColumn but no scan() loses its column stream, and squirreling's scalar-aggregate fast path bails on exactly that (node_modules/squirreling/src/execute/aggregates.js:299, if (!table?.scanColumn) return) before it ever consults prepareScan. The scan()-bearing branch forwards scanColumn correctly at sql.js:191, so this is an inconsistency inside the same function. One extra forward fixes it.

F3 (Low, untested branch). rowFallbackPreparedScan has no test.
src/core/query/union-source.js:435. It is unreachable with icebird children, because icebergDataSource.prepareScan always returns residual.filter === request.filter, so nativeCompatible at union-source.js:375 is always satisfied for the real sources in this repo. That makes it the one net LLP 0266#partition-union relies on and the one net nothing exercises. Two fake children with divergent residual contracts would cover it, and would also cover the columns: [] shape executeCount sends (node_modules/squirreling/src/execute/execute.js:573) which routes through union.scan({ columns: [] }).

F4 (Low, untested branch). The new privacy-critical throw has no test.
src/core/query/sql.js:371: Dataset "X" must provide scan() to enforce local-only visibility. This is the refusal LLP 0266 promises ("a governable prepared-only third-party source is refused rather than bypassing the privacy rule"). A test that a governable prepared-only source throws instead of returning unfiltered rows is worth having, since a future refactor that silently drops the guard is a privacy leak, not a perf regression.

F5 (Low, convention). The #partition-union decision is annotated on the test but not on the code that implements it.
test/core/union-source.test.js:141 carries @ref LLP 0266#partition-union [tests], but commonPreparedSchema (src/core/query/union-source.js:302) and prepareUnionScan (:354) carry none, even though the id-remap-by-name rule and the "strip LIMIT/OFFSET before preparing children" rule are precisely the non-obvious decisions CLAUDE.md asks to annotate. storage.js:409 and ai-gateway/src/dataset.js:243 both do this correctly for their sections.

Things I checked and found correct

• storage.js:405-414 cannot leak an internal cache field through the prepared path: squirreling derives every ColumnDemand from the advertised (filtered) schema at execute.js:531-541, ids are preserved by the filter, and icebird echoes back only the requested fields. Confirmed empirically that SELECT * returns only id/value.
• The union's residual gate at union-source.js:375-379 is correctly conservative: identity comparison on residual.filter means a partial residual can never be mistaken for a shared one.
• Stripping limit/offset before preparing children (union-source.js:371-372) and re-declaring them as residual matches executePreparedScan's invariant check at execute.js:433-437, so the "applied limit/offset without applying where" throw cannot fire.
• numRows now going undefined when any child is unknown (union-source.js:143-150) is safe: the only consumers are union-source.js:241, which already guards for undefined, and the numRows !== 0 partition filters in the plugins, which are unaffected.
• The ai-gateway declared-schema gate (dataset.js:243-250) can only pass when the declared set is a subset of the physical set, in which case the wrapper's column order equals the physical schema order, so wrapped.columns and wrapped.schema.fields cannot disagree.
• LLP hygiene is right: 0266 is a new doc, 0015/0098/0261 got forward-ref notes only, nothing Accepted was rewritten.
• Style: no semicolons, no em dashes anywhere in the diff, JSDoc types only, root-anchored .js type-import specifiers.

Addendum: second-pass findings

A second reviewer pass over the same SHA surfaced three items the review above missed. I verified each one against the installed icebird@0.8.23 / squirreling@0.16.1 before recording them. The first is the most substantive finding on this PR.

F6 (Medium, defect in new code). The LLP 0097 inline heap guard loses most of its sampling resolution on the prepared path, and for this repo's own cache files it can degrade to one sample per data file.
src/core/query/sql.js:228 calls guard.check('native_batch') once per prepared batch. The row path at sql.js:176-178 checks every BUDGET_CHECK_ROW_STRIDE rows, which is 4096 (sql.js:103). Icebird yields exactly one batch per parquet row group (node_modules/icebird/src/read.js:560, for (const rowGroup of parquetMetadata.row_groups)), and this repo's own writer puts one flushed batch into one row group (src/core/cache/iceberg/stream_append.js:386, rowGroupSize: sorted.length, with the comment "One batch is one row group"). So on a partition flushed as a single large batch, the inline guard fires roughly once for the whole file.

It is worse than a coarser sample, because the batch columns are lazy. Each one is { async read({ selection, signal }) { ... } } (read.js:582-603), so the engine decodes the row group only after guard.check has already returned. The check therefore samples heap that has not yet been allocated. sql.js:481-486 states plainly that the terminal check and the interval watchdog are not sufficient on their own, which is exactly why the inline stride exists. A blocking ORDER BY over a large partition can now retain the whole row group with no inline sample in between. Consider checking inside the batch consumption rather than only at the batch boundary, or splitting the check across the batch's selection.

F7 (Low). schemasAreCompatible compares dataType with JSON.stringify, which is key-order sensitive.
src/core/query/union-source.js:336. Two partitions with structurally identical types serialized in different key order ({type:'array',items:...} vs {items:...,type:'array'}) are judged incompatible and silently drop to the row path. Inert today because icebird stamps every field { type: 'unknown' } (node_modules/icebird/src/sql/icebergDataSource.js:86), so this is latent rather than live, but it silently disables the native path for any future source that emits richer dataType objects. A structural compare would be more honest than a stringify.

F8 (Low, message accuracy). The local-only refusal names the wrong missing member half the time.
src/core/query/sql.js:370-372 guards on !source.scan || !source.columns but the message only says "must provide scan() to enforce local-only visibility". A source that has scan() but no columns gets pointed at the wrong member. One-line fix, and it matters because this is the branch a plugin author will hit blind.

With F6 added, the ask list below is no longer purely cosmetic: F6 is a real weakening of a documented safety property (LLP 0097) introduced by this PR.

What would make this approvable

F2 (forward scanColumn), F5 (add the @ref), and F8 (fix the message) are each a line or two. F3 and F4 are one test each. F1 needs a decision recorded, not necessarily code. F6 is the one that needs real thought, since it trades away the inline heap sampling the row path guarantees.

[philcunliffe]

Requesting changes on quality gaps only. Nothing here is a correctness regression in the new code, and the version pins, API surface, typecheck, tests, and a differential prepared-vs-row probe over real Iceberg partitions all check out. Full evidence is in the review comment.

Asks, smallest first:

1. src/core/query/sql.js:153-162 - the prepared-only branch of withHeapBudget drops scanColumn, so a prepareScan + scanColumn source without scan() loses squirreling's scalar-aggregate fast path (aggregates.js:299 bails on a missing scanColumn before it ever looks at prepareScan). The scan()-bearing branch forwards it correctly at sql.js:191; make the two consistent.

2. src/core/query/union-source.js:302 and :354 - commonPreparedSchema and prepareUnionScan implement LLP 0266#partition-union (id remap by name, strip LIMIT/OFFSET before preparing children) but carry no @ref. Only the test at test/core/union-source.test.js:141 has one, while storage.js:409 and ai-gateway/src/dataset.js:243 annotate their sections correctly.

3. src/core/query/union-source.js:435 - rowFallbackPreparedScan has no test and is unreachable with icebird children, since icebergDataSource.prepareScan always returns residual.filter === request.filter. It is the safety net LLP 0266 leans on and nothing exercises it. Two fake children with divergent residual contracts would cover it.

4. src/core/query/sql.js:371 - the new "must provide scan() to enforce local-only visibility" refusal is privacy-critical and untested. A future refactor that drops it is a leak, not a perf regression.

5. Decision to record, no code required: this PR makes the prepared and row paths disagree on a purged table. Icebird's row scan() applies position deletes against the filtered row sequence rather than physical row positions, so on icebird@0.8.23 a partition holding ids 4/5/6 with id 5 position-deleted returns [4] for WHERE k='x' instead of [4, 6]. prepareScan().batches() dodges it by delegating to scan() with no where when hasDeletes. Net effect: a top-of-lattice caller gets correct rows through the prepared path, while a restricted caller (the visibility wrapper at visibility.js:124 deliberately does not forward prepareScan) or a drifted-schema union still silently loses rows. The bug is upstream and reproduces identically on 0.8.22, so this PR is a net improvement, but LLP 0266#schema-drift documents the row path as authoritative while it is currently lossy in exactly the hyp purge case LLP 0104 cares about. Worth an upstream icebird issue plus a note in the LLP, and it would fit test/core/iceberg-source-parity.test.js.

Two pre-existing bugs this PR fixes are worth calling out in the release notes: after a purge on a two-partition fixture, count(*) goes from 3 to 5 and WHERE k='x' goes from [1,3,4] to [1,3,4,6] versus master.

[platypii]

Addressed the review in 85a38f36:

• F1: filed icebird#41 and recorded the row-fallback exposure in LLP 0266
• F2: shared the budgeted scanColumn forwarding across scannable and prepared-only sources, with a prepared-only aggregate regression test
• F3: covered divergent filter residuals and the empty-projection count fallback
• F4/F8: added the privacy-refusal test and made the error name both required members
• F5: attached LLP 0266 references to the union implementation
• F6: budget checks now run after each deferred native column materializes, with a retained-growth regression test
• F7: replaced serialization comparison with structural equality and covered key-order variance

Validation: typecheck and declaration build pass; 106 focused Node 24 tests pass; LLP reference hygiene passes; CI passes on Node 22 and Node 24. The final real-cache benchmark is 3,628.2 ms to 1,957.2 ms median, with identical output digest.

[philcunliffe]

neutral review (round 2): update/icebird-squirreling-native-batches @ 85a38f36

Verdict: changes requested, on one finding only.

All eight asks from the d6f350d2 round are addressed and I verified each one against the code rather than the summary. What holds this back is a defect the round-2 diff did not introduce but that both rounds missed: withHeapBudget detaches source.scan from its receiver, which master did not. It is one word to fix. Everything else below is Low and optional.

Round-1 asks: all addressed

Ask	Where	Verified
F1 record the icebird row-scan/deletes exposure	llp/0266:84-89, icebird#41	yes (see F10 for one gap in the list)
F2 forward scanColumn on the prepared-only branch	sql.js:161 -> new forwardBudgetedScanColumn at sql.js:206	yes, shared by both branches, with a test at test/core/query-sql-budget.test.js
F3 test rowFallbackPreparedScan	test/core/union-source.test.js "different filter residuals" + "empty prepared projection"	yes
F4 test the privacy refusal	test/core/query-sql-budget.test.js "governable prepared-only source refuses restricted visibility"	yes
F5 @ref LLP 0266#partition-union on the implementation	union-source.js:303, :356	yes
F6 inline heap sampling on the prepared path	budgetedBatch at sql.js:253-273, sampling after each deferred read resolves	yes, with a retained-growth regression test
F7 structural dataType compare	union-source.js:339 isDeepStrictEqual	yes, with a key-order test (see F11 for a caveat)
F8 error names both members	sql.js:410 "must provide columns and scan()"	yes

CI is green on this SHA (test 22/24, typecheck 22/24, duplicate-numbers), the PR is MERGEABLE, the diff has no semicolons and zero em dashes, and LLP hygiene is right (0266 is new; 0015/0098/0261 got forward-ref notes only).

Findings

F9 (Medium, blocking, defect introduced by this PR). withHeapBudget calls scan() without its receiver.

src/core/query/sql.js:166 hoists const scan = source.scan, then src/core/query/sql.js:172 calls scan(options). Master called source.scan(options) directly. This is an oversight rather than intent, because the very same function preserves the receiver everywhere else:

• sql.js:157 and sql.js:194: prepareScan.call(source, request)
• sql.js:212: scanColumn.call(source, options)

and squirreling itself invokes both members with a receiver (execute.js:291 table.prepareScan(...), execute.js:305 table.scanColumn(...)).

Every registered dataset passes through this decorator (sql.js:513 tables[name] = withHeapBudget(tables[name], guard)), and AsyncDataSource is the public plugin contract in hypaware-plugin-kernel-types.d.ts, so a plugin that implements scan() as a class or prototype method now fails for every hyp query sql against its dataset. Reproduced standalone:

master form  source.scan(options): OK
PR form      scan(options): TypeError: Cannot read properties of undefined (reading 'rows')
(src.scan)(options): OK  <- parenthesized member call keeps the receiver

Reachability today: latent, not live. No in-repo source uses this in scan (storage.js:377, visibility.js:133, union-source.js:157, ai-gateway/src/dataset.js:182 are all object literals over closures), and icebird's scan closes over locals while its prepared path routes through a captured thisSource (icebergDataSource.js:146). So this is a contract regression, not a broken build. It is still worth fixing before merge: it is a one-word change (scan.call(source, options)), it silently narrows what a plugin author may hand the kernel, and the failure mode for whoever hits it is a TypeError from deep inside the kernel with no hint that the decorator ate the receiver.

For the record, I checked and these are not instances of the same bug: union-source.js:377 (source.prepareScan)({...}) and union-source.js:453 (union.scan)({...}) are parenthesized member expressions, which preserve the Reference and therefore the receiver. Only the assign-to-local form breaks it.

F10 (Low). The LLP 0266 exposure list for icebird#41 omits the scanColumn path, which the engine picks ahead of prepareScan.

llp/0266-native-prepared-batches-through-query-sources.decision.md:88 says the row-scan defect leaves "visibility and schema-drift fallbacks" exposed. There is a third exposed path. execute.js:290 takes the prepared branch only if (!existingColumnResult && table.prepareScan && table.schema), and the scalar-aggregate fast path supplies existingColumnResult (aggregates.js:299-334, handed back through executeScan(plan, context, columnScan.fallback.result) at aggregates.js:213). So on a purged partition, SELECT id ... WHERE g = 'b' now takes the fixed prepared path while SELECT COUNT(*) ... WHERE g = 'b' still takes the lossy scanColumn path, and the two shapes of the same predicate disagree. Not a regression (master gets both wrong), but the doc reads as if the prepared path closed everything except two named fallbacks, and it did not.

F11 (Low). The dataType half of the compatibility gate is inert for cache-backed partitions.

src/core/query/union-source.js:339 is now a structural compare, which is the right fix for F7. But icebird/src/sql/icebergDataSource.js:86 stamps dataType: { type: 'unknown' } on every field, so for icebird children only ordered names and nullable actually discriminate. llp/0266:60 states the gate requires "identical ordered names, data types, and nullability"; for this repo's own sources the data-type clause is unenforceable today. One sentence in the decision noting that the type check is a forward guarantee, not a currently-active one, would keep a future change from leaning on it.

F12 (Low). The prepared-only branch drops the columns fallback its two siblings use.

src/core/query/sql.js:159 sets columns: source.columns flat, while the scan branch seven lines later (sql.js:170) uses source.columns ?? source.schema?.fields.map((field) => field.name) ?? [], and executeQuerySql uses the same fallback at sql.js:402. A prepared-only source that omits columns (precisely the shape squirreling's widened contract allows and this branch exists to support) comes out of the decorator with columns: undefined where both sibling sites would have derived the names from the schema.

F13 (Low). prepareUnionScan prepares every child before deciding whether it can use them.

src/core/query/union-source.js:369-381: all child prepareScan calls run in the childScans map, and only then is nativeCompatible evaluated at :381. On the fallback path that work is discarded, and more to the point a child that throws inside prepareScan aborts the whole union rather than degrading to the row path rowFallbackPreparedScan exists to provide. Unreachable with icebird children (they always echo residual.filter === request.filter and always return { fields: requestedFields }), so this only matters for the third-party sources the fallback was written for.

Things I checked and found correct

• budgetedBatch (sql.js:253) preserves selection, leaves direct-vector columns untouched, and calls read.call(column, request) against the original column, so the F6 fix does not itself lose a receiver.
• The union's residual gate (union-source.js:381-383) is still identity comparison on residual.filter, so a partial residual can never be mistaken for a shared one.
• Stripping limit/offset before preparing children (union-source.js:377-378) still matches executePreparedScan's invariant check, so the "applied limit/offset without applying where" throw cannot fire.
• The new privacy refusal at sql.js:409-411 still guards on the raw source, before withHeapBudget wraps it at sql.js:513, so the widened prepared-only shape cannot slip past the visibility rule.
• Benchmark claim is plausible and unchanged from round 1; I did not re-measure.

What would make this approvable

F9 alone: scan.call(source, options) at src/core/query/sql.js:172. F10 through F13 are notes, not asks, and I would not hold the PR for any of them.

[philcunliffe]

Round 2. All eight round-1 asks are addressed and verified against the code, CI is green, and the PR is mergeable. One blocking ask remains, and it predates the round-2 fixup (both rounds missed it).

src/core/query/sql.js:172 - withHeapBudget calls scan() without its receiver.

sql.js:166 hoists const scan = source.scan and sql.js:172 calls scan(options). Master called source.scan(options). The same function preserves the receiver everywhere else (prepareScan.call(source, ...) at :157 and :194, scanColumn.call(source, ...) at :212), and squirreling itself calls table.prepareScan(...) / table.scanColumn(...) with receivers (execute.js:291, :305), so this reads as an oversight.

Every registered dataset passes through this decorator (sql.js:513), and AsyncDataSource is the public plugin contract in hypaware-plugin-kernel-types.d.ts. A plugin implementing scan() as a class or prototype method returns rows on master and throws TypeError: Cannot read properties of undefined on this branch, for any hyp query sql against its dataset.

Latent today, not live: no in-repo source uses this in scan, and icebird's scan closes over locals while its prepared path routes through a captured thisSource (icebergDataSource.js:146). But it is a one-word fix - scan.call(source, options) - and it silently narrows the plugin contract with a failure mode that surfaces as a TypeError from deep inside the kernel.

Not the same bug, checked: union-source.js:377 (source.prepareScan)({...}) and :453 (union.scan)({...}) are parenthesized member expressions, which keep the receiver.

Four Low notes (LLP 0266's icebird#41 exposure list omits the scanColumn aggregate path that the engine picks ahead of prepareScan; the dataType half of schemasAreCompatible is inert while icebird stamps every field unknown; the prepared-only branch of withHeapBudget drops the columns schema fallback its two siblings use; prepareUnionScan prepares every child before checking compatibility) are in the review comment with evidence. None of them should hold the PR.

[philcunliffe]

Additional requested change: llp/0266 is a duplicate number.

This branch adds llp/0266-native-prepared-batches-through-query-sources.decision.md, but 0266 is also claimed on fix/issue-836 (#850, llp/0266-core-command-argument-validation.decision.md), which is the older PR. Whichever of the two merges second lands a second LLP 0266 on master.

Four other in-flight branches hit the same collision this tick and have already been renumbered to yield the number to the oldest claimant, so 0266 is now contested only between this PR and #850. The next numbers free across master and every open branch are 0280 and up.

Because this is a review-only delegation (neutral:review), neutral did not push the rename itself. Suggested fix, entirely mechanical (LLP 0156 permits renumbering that does not change meaning):

git mv llp/0266-native-prepared-batches-through-query-sources.decision.md \
       llp/0280-native-prepared-batches-through-query-sources.decision.md
# then update the in-file title and every "LLP 0266" reference in code and docs

Please fold this in alongside the F9 receiver fix. Replying on this thread reaches neutral: it monitors this PR and will re-engage on its next tick.

[platypii]

Addressed the round-2 blocker with a minimal fix in bccd1c29:

• call the captured row scanner as scan.call(source, options) and cover a receiver-dependent source with a regression test
• use schema field names when a prepared-only source omits columns
• name the scanColumn aggregate path in the Icebird Feature: cache-lifecycle #41 exposure note
• renumber this PR's LLP from 0266 to 0294, the number returned by the repository allocator after fetching all refs

I left the dataType compatibility and prepare-all-children notes unchanged. Icebird's current unknown types make the former inert rather than incorrect, and the latter has no demonstrated semantic defect.

Validation is green: 95 focused Node 24 tests, typecheck, declaration build, LLP reference and number checks, plus CI tests/typechecks on Node 22 and Node 24.

[philcunliffe]

neutral review (round 3): update/icebird-squirreling-native-batches @ bccd1c29

Verdict: clean. Nothing actionable remains.

Both round-2 asks landed, and I verified each against the code rather than the summary. The round-3 delta over 85a38f36 is genuinely small once the intervening rebase onto master is subtracted: a one-word receiver fix, a columns fallback, one new test, the LLP renumber, and one sentence added to the decision doc. Everything else in 85a38f36..bccd1c29 is master moving underneath the branch (spool.js, the command/verb registry declarations, hypgrep), not this PR's work.

Round-2 asks: both addressed

Ask	Where	Verified
F9 (blocking) withHeapBudget detached scan from its receiver	src/core/query/sql.js:172 is now scan.call(source, options)	yes, with a regression test at test/core/query-sql-budget.test.js:174 ("the budget decoration preserves the scan receiver") that asserts this === source inside a receiver-dependent scan
LLP 0266 number collision with #850	llp/0294-native-prepared-batches-through-query-sources.decision.md; 0266 on this branch is now master's unrelated 0266-cli-compatibility-rollover.plan.md	yes. Both CI number gates are green (duplicate-numbers, cross-branch-numbers), and grep -rn "LLP 0266" finds no stale reference to the native-batches doc: the three remaining hits are session_command.js's 0266#milestones, an llp/0281 citation, and the llp-number-minting fixture, none of them this document

Also picked up unasked, and correct:

• F12 (the prepared-only branch dropped the columns schema fallback its siblings use) is fixed at src/core/query/sql.js:159: source.columns ?? schema.fields.map((field) => field.name). The three sites (sql.js:159, sql.js:171, sql.js:402) now derive names the same way.
• F10 (the icebird#41 exposure list omitted the scanColumn aggregate path) is fixed at llp/0294:... — the exposure sentence now reads "visibility wrappers, scanColumn aggregates, and schema-drift fallbacks remain exposed".

Independent verification I ran in a detached worktree at bccd1c29

• npm install clean, and the two pins are mutually consistent rather than merely both-resolvable: icebird@0.8.23 itself depends on squirreling@0.16.1 exactly, and npm dedupes to a single copy (npm ls shows icebird@0.8.23 -> squirreling@0.16.1 deduped). So the prepared scan icebird produces and the one squirreling's planner consumes are the same contract, not two builds of it. hyparquet also dedupes to the pinned 1.28.2. No lockfile is tracked (.gitignore:9), so package.json is the whole of the pin.
• npm run typecheck: exit 0.
• npm test: 4917 tests, 4916 pass, 1 skipped, 0 fail, exit 0. No flake this time.
• All 10 CI checks green on this SHA (tests + typechecks on Node 22 and 24, both number gates).

Receiver audit, repo-wide over the diff. F9 was the reason to check the rest. Every member the new code hoists to a local is re-attached: sql.js:161/sql.js:194 prepareScan.call(source, request), sql.js:176 scan.call(source, options), sql.js:212 scanColumn.call(source, options), sql.js:283 read.call(column, request), storage.js:412 and dataset.js:250 prepareScan.call(source, request). The remaining call sites (storage.js:403 (source.scanColumn)(options), union-source.js:377 (source.prepareScan)({...}), union-source.js:453 (union.scan)({...})) are parenthesized member expressions, which keep the Reference and therefore the receiver. No detached-member call survives in the diff.

Pushdown correctness, re-derived rather than taken on trust. The union forwards request.filter to each child unchanged while remapping only ColumnDemand.field. That is sound, and the reason is in the installed types: ColumnDemand (squirreling/src/types.d.ts:122) carries exactly one identifier, a flat field: number, so a name-keyed remap is complete with no nested ids to miss; and ExprNode's column reference is IdentifierNode { name: string } (squirreling/src/ast.d.ts:82), i.e. by name, not by field id. Since commonPreparedSchema already required identical ordered names across children, a filter that is valid against the union's logical schema is valid against every child verbatim. Had ExprNode been id-addressed, forwarding it unchanged would have mis-pushed the predicate on every child but the first; it is not, so it does not.

Null / absent-column handling. The drift gate is the load-bearing part and it holds on both sides. commonPreparedSchema (union-source.js:303) returns undefined unless every child has schema + prepareScan, the first schema's ordered names equal the union's columns, and every other child matches on name, nullable, and a structural dataType compare, so a drifted union never exposes prepareScan and the row path keeps sole ownership of the undefined-vs-null split LLP 0261 settled. The ai-gateway wrapper applies the mirror-image gate at dataset.js:245, forwarding a prepared scan only when the physical schema already contains every declared column. A partition missing git_remote stays on the row/scanColumn paths. Native batches never get the chance to invent a third pad value.

The prepared-only shape is type-guaranteed, not assumed. withHeapBudget's new !source.scan branch casts source.schema and source.prepareScan to non-null. That cast is discharged by the contract rather than by hope: AsyncDataSource in squirreling@0.16.1 (types.d.ts:222) is a two-arm union where the arm without a required scan has schema and prepareScan required. So !source.scan implies both are present.

Privacy ordering still holds under the widened contract. withLocalOnlyVisibility deliberately forwards neither schema nor prepareScan, and it wraps source at sql.js:411 before withHeapBudget wraps the result at sql.js:513. A governable dataset therefore reaches the budget decorator already stripped of its prepared members, so the native path cannot light behind the visibility filter. The prepared-only refusal at sql.js:409-411 is a new error, not a regression: that source shape was unrepresentable under squirreling@0.15.3.

budgetedBatch is structurally complete. AsyncBatch (types.d.ts:105) has exactly selection and columns, both of which it carries. The deferred BatchColumn arm (types.d.ts:96) has read, input?, rowOffset?, rowOrdinals?; the ...column spread preserves the three it does not override, and the ColumnVector arm is passed through by identity via the !('read' in column) guard. Nothing is dropped on either arm.

The numRows change is a fix, not drift. unionSources now yields numRows: undefined when any child's count is unknown, instead of silently contributing 0. Given createDataSource's existing rule that an unknown count means "purged partition with position deletes, not empty" (dataset.js:148, LLP 0104), the old behaviour understated the union total on exactly the partitions that rule exists to protect. Making the total unknown is the correct propagation.

LLP hygiene. All three @ref anchors resolve (#transparent-wrappers, #partition-union, #schema-drift are all defined in llp/0294). The touched Accepted docs (llp/0015, llp/0098, llp/0261) receive only forward-refs, which is what CLAUDE.md's "accepted docs are settled" rule permits. Kernel-contract additions are additive: ScannableDataSource is a new exported alias and DatasetRegistration.createDataSource's declared return type is untouched, so no third-party plugin is narrowed by this PR.

Carried forward, not blocking

Two round-2 notes the author declined, with reasons I accept. Neither is an ask.

• dataType half of the union compatibility gate is inert for cache-backed partitions (union-source.js:339). icebird stamps dataType: { type: 'unknown' } on every field, so today only ordered names and nullable discriminate. The check is a forward guarantee. It is correct code that currently cannot fail; it is not wrong.
• prepareUnionScan prepares every child before evaluating nativeCompatible (union-source.js:369-381). On the fallback path that work is discarded, and a child that throws inside prepareScan aborts the union rather than degrading to rowFallbackPreparedScan. Unreachable with icebird children, which always echo residual.filter === request.filter and always return { fields: requestedFields }. Worth remembering if a third-party prepared source ever joins a union.

Benchmark

Not re-measured. The claim is unchanged from round 1 and the mechanism now checks out end to end, which is the part a reviewer can actually verify: storage, union, ai-gateway, and heap-budget wrappers each forward schema + prepareScan under an explicit transparency gate, so a schema-aligned Iceberg dataset reaches native batches without falling back to one AsyncRow and one promise per cell.

Verdict

Approving. Both blocking items from round 2 are closed, the pins are mutually consistent and match what the code assumes, pushdown is sound for a name-addressed filter language, drift and visibility both keep the row path, and local npm test plus npm run typecheck plus all 10 CI checks are green.

Review-only delegation (neutral:review, LLP 0025/0032): neutral pushed nothing to this branch and will not merge. The maintainer merges.

[philcunliffe]

Approved: both round-2 blockers are closed (the withHeapBudget scan receiver is restored at src/core/query/sql.js:172 with a regression test, and the LLP is renumbered to 0294 with both CI number gates green). Supersedes my round-2 changes-requested review. Review-only delegation: neutral pushed nothing and will not merge.

[philcunliffe]

Approved: both round-2 blockers are closed (scan receiver restored at src/core/query/sql.js:172 with a regression test; LLP renumbered 0266 -> 0294 with both CI number gates green), pins are mutually consistent, and typecheck plus 4916 tests plus all 10 CI checks are green.

[platypii]

Superceded by #982