Use native Icebird batches for faster queries

[platypii]

Summary

• upgrade Icebird from 0.8.22 to 0.8.25 and Squirreling from 0.15.3 to 0.16.1
• preserve native prepared batches through storage, schema, visibility, union, and heap-budget wrappers
• add a reproducible benchmark over an immutable real HypAware log snapshot
• record the design in LLP 0294

Real-data benchmark

Measured on the same retired ai_gateway_messages snapshot for both revisions: 264,048 rows, 1,088 Parquet files, and 473.7 MiB compressed. Values are medians from five measured iterations after one warmup.

Workload	Baseline time	Updated time	Time change	Baseline peak RSS	Updated peak RSS	RSS change
COUNT(*)	21.51 ms	24.38 ms	13.3% slower	188.2 MiB	189.8 MiB	+0.9%
Two-column aggregate	1,277.85 ms	477.24 ms	62.7% faster	612.0 MiB	661.4 MiB	+8.1%
Filtered aggregate	708.02 ms	449.89 ms	36.5% faster	429.6 MiB	553.1 MiB	+28.7%
Projected scan	867.48 ms	524.89 ms	39.5% faster	530.3 MiB	517.8 MiB	2.4% lower

Every result checksum matched the baseline. Peak RSS includes the complete Node process lifetime and V8 retained memory, so it is noisier than elapsed time and is not a per-query allocation measurement. Post-GC retained heap deltas were near zero for both versions.

Run the benchmark with:

npm run bench:icebird-real

Validation

• npm run build:types
• npm run typecheck
• 70 focused query and storage tests pass
• benchmark command smoke check passes
• git diff --check passes

Full-suite note: npm test reported 5,037 passing, 2 skipped, and 2 unrelated failures. The unchanged CLI help assertion omits the already-rendered graph command. The Claude overlapping-body case was timing-sensitive in the concurrent run; its focused file passed 3/3 when rerun with loopback-listener permission.

[philcunliffe]

Review record: PR #982, head dc3e0bbe

Verdict: approve with nits. The design is sound and the risky seams (internal-field hiding, local-only visibility, LIMIT/OFFSET over a union, absent-column padding) are all gated correctly. One low-severity finding, fixed and pushed. Nothing blocking.

Scope reviewed

All 30 files in the merge-base diff (+1321/-102). Deep read of src/core/query/union-source.js, src/core/query/sql.js, src/core/cache/storage.js, hypaware-core/plugins-workspace/ai-gateway/src/dataset.js, plus the new LLP 0294 and the four doc forward-refs. Cross-checked every assumption against the vendored contracts in node_modules/squirreling/src/types.d.ts, node_modules/squirreling/src/backend/batch.js, and node_modules/icebird/src/sql/icebergDataSource.js.

Findings

LOW (1), fixed

src/core/query/visibility.js:9, test/core/parquet-source.test.js:16, test/core/star-expansion-drifted-union.test.js:35 - the refactor replaced every AsyncDataSource annotation in these three files with ScannableDataSource but left the now-unreferenced AsyncDataSource specifier in each @import list. star-expansion-drifted-union.test.js also ended up with two separate @import lines from hypaware-plugin-kernel-types.js. Dead type imports are exactly the kind of thing a later reader treats as a live constraint.

Fixed in 4c8df94: specifier removed from all three, and the star-expansion test's two kernel-types imports folded into one. Verified positively - the committed tree has zero AsyncDataSource occurrences in each of the three files, versus one each at dc3e0bbe.

Open findings: none.

What was verified positively (not just "tests are green")

• Prepared/row differential over real Iceberg partitions. Built two-partition unions through appendRowsToTable -> dataSourceForTable -> unionSources and ran every result through both the prepared source and a row-only twin ({columns, numRows, scan}), comparing serialized output byte for byte across SELECT *, projections, pushed and unpushed WHERE, LIMIT/OFFSET with and without a filter, LIKE, IS NULL, COUNT/SUM/AVG/MIN/MAX, and ORDER BY ... DESC LIMIT 1. Covered INT64, BOOLEAN, DOUBLE, TIMESTAMP, STRING and null cells. All identical, including value types (BigInt stayed BigInt, Date stayed Date), so the native path introduces no user-visible representation drift.
• Internal cache fields do not leak through the prepared path. Staged a table carrying _hyp_ingest_seq, took it through createQueryStorageService().dataSourceForTable, unioned it, and ran SELECT *. The wrapper advertises only public columns in both columns and schema, and the prepared result set contains no internal field. The claim in the storage.js:405 comment (Squirreling plans demands from the advertised schema, so no prepared demand can name an internal field) holds against the actual planner.
• Visibility is not bypassable. withLocalOnlyVisibility returns a source with only columns and scan, so schema/prepareScan/scanColumn are all dropped and a restricted caller can never reach native batches. sql.js:410-421 orders this correctly: visibility wraps first, withHeapBudget second (sql.js:521), so the budget decoration cannot re-expose a prepared surface the visibility wrapper deliberately withheld.
• residual identity check is sound for the real child. icebird's prepareScan always echoes residual.filter = request.filter by identity (icebergDataSource.js:134-138), so the union's === comparison at union-source.js:397-399 takes the native path when schemas align and degrades to the row fallback otherwise. Both directions are safe: the fallback re-applies the whole filter as residual, and re-applying an already-applied predicate is idempotent.
• Batch wrapping respects the engine's contract. budgetedBatch (sql.js:257-278) discriminates deferred columns with 'read' in column, which is correct because no ColumnVector variant has a read key (squirreling/src/types.d.ts:61-83), and the spread preserves input/rowOffset/rowOrdinals, which readBatchColumn reads off the column object (backend/batch.js:178-186). One wrapped batch object per source batch, so readBatchColumn's per-batch memoization WeakMap still works.
• Budget enforcement does not go dark on the native path. guard.check samples on every call (the 4096 stride lives in the row loop, not the guard), so per-batch and per-deferred-read checks are real enforcement, and test/core/query-sql-budget.test.js asserts a trip with site === 'native_batch'.
• @ref hygiene. All four annotations added or moved by this PR resolve: LLP 0294#transparent-wrappers, #partition-union, #schema-drift are real {#...} markers in the new doc, and LLP 0098#wrapper-duties still exists.
• Style rules. No em dashes and no statement-terminating semicolons anywhere in the changed files.

Gates run locally

npm test (5040 pass, 0 fail, 1 skipped, including hyparquet-floor-pin), npm run typecheck clean, and 11 hermetic smokes green: gateway_claude_capture, gateway_codex_capture, claude_telemetry_capture, local_parquet_export, local_only_export_withhold, source_optout_export_withhold, hypignore_capture_drop, otel_loopback_capture, cli_bundled_plugins_activated, status_diagnostics, walkthrough_picker_to_first_query. Re-ran the suite and typecheck after the fix.

Non-blocking observations (no change requested)

1. union-source.js:334-341 - schemasAreCompatible compares field.dataType with isDeepStrictEqual, but icebird stamps every field { type: 'unknown' } (icebergDataSource.js:82-90), so today that clause is vacuous and two partitions whose same-named column has genuinely different physical types would still concatenate natively. Not a regression: the row path never checked types either, and the engine is value-dynamic. Worth knowing if icebird starts reporting real types.
2. union-source.js:369-385 - every child is prepared eagerly, then discarded when nativeCompatible is false and rowFallbackPreparedScan takes over. icebird's prepareScan is pure but does per-request manifest and data-file pruning, so the fallback pays for a prune it never uses. Unreachable with icebird as the child (see the residual note above), so this is defensive code paying a defensive cost.
3. sql.js:265-275 - the deferred-read decoration calls process.memoryUsage() once per column per batch. On Linux that reads /proc/self/statm for RSS, so a wide projection over many batches adds a fixed per-read cost to the very path this PR exists to speed up. benchmarks/icebird-real-data.mjs is the right place to confirm it stays in the noise.
4. sql.js:417-419 - the new throw for a governable prepared-only source is the documented LLP 0294 trade-off (fail closed rather than bypass the privacy rule) and reads correctly, but it is a runtime error a third-party plugin author would only meet in production. The message names the dataset and both missing members, which is the right level of detail.

[philcunliffe]

Review record: PR #982, head 7433fe14

Verdict: approve with one fix, applied and pushed. This is round 2 of the neutral review (round 1 reviewed dc3e0bbe; two follow-up commits landed since). One LOW finding on a privacy gate, fixed in bf5ece2d with a regression test that fails without the fix. Nothing blocking remains.

Scope reviewed

The full 30-file merge-base diff (+1449/-108), with a deep read of src/core/query/sql.js, src/core/query/union-source.js, src/core/cache/storage.js, src/core/query/visibility.js, hypaware-core/plugins-workspace/ai-gateway/src/dataset.js, benchmarks/icebird-real-data.mjs, LLP 0294 and the four doc forward-refs. Every claim about the upstream contract was checked against the vendored code actually installed at these pins, not from memory: node_modules/squirreling/src/types.d.ts, .../backend/batch.js, .../backend/dataSource.js, and node_modules/icebird/src/sql/icebergDataSource.js + src/read.js.

Findings

LOW (1), fixed in bf5ece2d

src/core/query/sql.js:150 (at 7433fe14) - sourceColumnNames() inverted the precedence its own JSDoc promised.

The doc says the helper "derives the names the way the engine's own dataSourceColumns does". Squirreling's actual helper (node_modules/squirreling/src/backend/dataSource.js:27-33) is:

if (source.prepareScan && source.schema) return source.schema.fields.map(f => f.name)
return source.columns ?? []

The kernel's was the other way round:

return source.columns ?? source.schema?.fields.map((field) => field.name) ?? []

For a source carrying both prepareScan+schema and a columns list, the engine plans from the schema and never reads columns, while the kernel read columns and never reached the schema. One of the helper's three call sites is the LLP 0105 local-only visibility gate at sql.js:446-450:

const sourceColumns = sourceColumnNames(source)
const governable = sourceColumns.includes('cwd') ||
  contentColumns.some((c) => sourceColumns.includes(c))

So a third-party prepared source advertising a narrow columns list next to a wider schema would have had governable decided against a list the engine ignores. A declared localOnlyContentColumns entry (or cwd) present only in the schema would miss the gate, withLocalOnlyVisibility would never be applied, and a restricted caller would get the prepared scan unfiltered.

Not reachable through any source in this repo: storage (storage.js:401-414), the ai-gateway declared-schema wrapper (dataset.js:240-252), unionSources (union-source.js:186-190, gated on schemaMatchesColumns), and icebird itself (icebergDataSource.js:82-90, where columns and relationSchema.fields are built from the same schema.fields in the same order) all keep the two lists identical. But LLP 0294 explicitly opens the door to third-party prepared sources, the failure mode is a silent privacy leak rather than an error, and the fix is three lines.

Fixed by making the code match the comment. sql.js:158-160 now reads:

function sourceColumnNames(source) {
  if (source.prepareScan && source.schema) return source.schema.fields.map((field) => field.name)
  return source.columns ?? []
}

Behavior is byte-identical for every in-repo source (the two lists agree everywhere), and where they could disagree the new precedence can only widen the set of sources that get wrapped, which is the fail-closed direction LLP 0294 already chose for the prepared-only refusal immediately below it.

Pinned by a new test at test/core/query-sql-budget.test.js:339, the local-only gate reads a prepared source schema, not its narrower columns list. Verified it is a real pin, not a tautology: reverted sql.js to the 7433fe14 version with the test in place and it failed (not ok 11, 12 pass / 1 fail); restored the fix and it passes (13/13).

What I checked and found correct

• Prepared batches cannot leak internal cache fields. storage.js filters INTERNAL_FIELDS out of both columns and schema.fields, and squirreling plans demands from the schema, so no prepared request can name one. Confirmed readBatchColumn never consults source.columns.
• The visibility wrapper genuinely drops the native path. withLocalOnlyVisibility builds a fresh object literal with only columns and scan (visibility.js:129-176), so a filtered query cannot fall through to prepareScan. Storage's scan() does no row-level policy filtering either, so forwarding prepareScan past it bypasses nothing.
• The union's residual negotiation is sound at these pins. icebird's prepareScan always returns residual.filter === request.filter by reference (icebergDataSource.js:132-141, filter used for pruning only), so the reference-equality checks at union-source.js:427-431 resolve to the native path for icebird children and fail closed to rowFallbackPreparedScan for anything that disagrees.
• budgetedBatch copies the whole AsyncBatch. Confirmed against squirreling/src/types.d.ts:105-108 that AsyncBatch is exactly {selection, columns}, and that the deferred BatchColumn extras (input, rowOffset, rowOrdinals) survive the spread at sql.js:288-292. read.call(column, request) keeps this on the original. The substituted batch object is harmless: readBatchColumn passes column.input ?? batch, and icebird's deferred read destructures only {selection, signal} (icebird/src/read.js:662).
• The sync-read preservation is real, not cargo-culted. isThenable + the non-async read keep resolveColumnResults off the promise path for sources whose reads resolve inline. icebird's own read is async, so this only pays off for other sources, but it costs nothing and the comment is accurate.
• numRows correctness. The change from s.numRows ?? 0 to "one unknown makes the total unknown" is a genuine fix: icebird leaves numRows undefined on a snapshot carrying position deletes, which is exactly the hyp purge case, and the old code would have reported a short total. The new purge test pins it.
• The purge test is the right test. purged rows stay purged through the prepared union over real Iceberg partitions uses real appendRowsToTable/deleteMatchingRows over two partitions, deletes the leading row of the first file (so a delete applied against filtered rather than physical ordinals would drop the wrong row), and cross-checks ORDER BY, a residual filter, COUNT, and LIMIT/OFFSET. That is the one silent-wrong-answer risk in the whole PR and it is covered against the real engine.
• Style rules. No em dashes and no statement-terminating semicolons anywhere in the diff; git diff --check clean; no unused type-import specifiers left in any changed file (I re-ran the check that produced round 1's finding, across all 16 changed .js/.mjs files, and it is clean).
• LLP hygiene. All three anchors targeted by new @refs exist (0294#transparent-wrappers, #partition-union, #schema-drift), and the forward-refs added to the Accepted 0015/0098/0261 are Extended-by annotations, which the repo's conventions permit on settled docs.

Gates run locally

• npm test: 5042 pass, 0 fail, 1 skipped after the fix (5041/0/1 before). hyparquet-floor-pin passed.
• npm run typecheck: clean, before and after.
• node --test test/core/query-sql-budget.test.js: 13/13.

Non-blocking observations (no change requested)

1. union-source.js:340-347 - schemasAreCompatible compares field.dataType with isDeepStrictEqual, but icebird stamps every field { type: 'unknown' } (icebergDataSource.js:84-88), so that clause is vacuous today. Carried over from round 1; still true, still not a regression, still worth knowing if icebird starts reporting real types.
2. union-source.js:412-425 - every child is prepared eagerly and then discarded when nativeCompatible is false. icebird's prepareScan does per-request manifest and data-file pruning, so the fallback pays for a prune it never uses. Unreachable with icebird as the child; defensive code paying a defensive cost.
3. benchmarks/icebird-real-data.mjs defaults to --max-heap-mb 0, which skips withHeapBudget entirely, so the headline table in the PR body measures the bare source stack rather than the production path. The script says so plainly in runScenario, and the --max-heap-mb flag exists to measure the other one. Worth a line in the release notes if these numbers get quoted.

[philcunliffe]

Triage of residual review findings at head bf5ece2d6c111a5bccc384e86790740a796a607f: both review rounds ended approve, and every fix the reviews pushed is verified present in the tree (the sourceColumnNames schema-precedence fix plus its regression test at test/core/query-sql-budget.test.js:339).

Four residual observations remain, all classified non-blocking (no wrong behavior, data loss, security exposure, crash, or perf regression past a stated budget). They are deferred to #983:

1. Vacuous dataType clause in schemasAreCompatible while icebird reports { type: 'unknown' }.
2. Eager child prepareScan work discarded when the union takes the row fallback.
3. Benchmark defaults to --max-heap-mb 0, so the headline numbers measure the bare stack, not the budgeted production path.
4. Per-deferred-read process.memoryUsage() sampling in budgetedBatch is unmeasured on that budgeted path.

No blocker found; the PR can merge safely with #983 tracking the deferrals.