Skip to content

Add scanColumn streaming primitive to the iceberg SQL data source - #28

Merged
philcunliffe merged 2 commits into
masterfrom
feat/scan-column
Jul 1, 2026
Merged

Add scanColumn streaming primitive to the iceberg SQL data source#28
philcunliffe merged 2 commits into
masterfrom
feat/scan-column

Conversation

@philcunliffe

Copy link
Copy Markdown
Contributor

What

Implements scanColumn(options): AsyncIterable<ArrayLike<SqlPrimitive>> on the iceberg SQL data source (src/sql/icebergDataSource.js), the optional AsyncDataSource.scanColumn hook squirreling already defines (ScanColumnOptions = { column, limit?, offset?, signal? }).

It streams a single column's values in row order, yielding one chunk per parquet row group by reusing the existing readDataFile reader with a single wanted column.

Why (streaming / memory)

squirreling's scalar-aggregate fast path tryColumnScanAggregate can compute COUNT / MIN / MAX / SUM / AVG and low-cardinality COUNT(DISTINCT …) in O(1)/O(cardinality) state by pulling a single column through scanColumn. But it bails at the !table.scanColumn guard when no source implements the hook, falling back to buffering every scanned row — which OOMs on large tables.

With this hook present, peak memory is bounded by a single row group's worth of values instead of the whole table: each yielded chunk is one row group's column values, and the engine's aggregate holds only an accumulator.

Semantics

scanColumn is never given a WHERE (the engine only takes this path when the scan has no filter) and returns no appliedLimitOffset flag, so it fully honors LIMIT/OFFSET itself:

  • No deletes: record_count is exact, so whole files are skipped for OFFSET and the per-file read is bounded for LIMIT.
  • With deletes: record_count is pre-delete, so LIMIT/OFFSET are applied over the post-delete value stream.

signal aborts between chunks (throws AbortError), mirroring scan().

Tests

test/sql/icebergDataSource.test.js adds a scanColumn suite:

  • streams a column in row order, matching scan({ columns: [c] }) for the same column;
  • respects LIMIT/OFFSET with no deletes (incl. zero-limit and offset-past-end);
  • applies LIMIT/OFFSET over post-delete values on a table with deletes (v4 fixture);
  • aborts promptly on a pre-aborted signal;
  • proves a scalar aggregate run through squirreling actually invokes the hook (streaming fast path, not the buffering fallback).

Gates: npm test (582 passing), npm run lint clean, npm run build:types (tsc) clean.


Part of the hypaware bounded-query-execution effort (design: hypaware LLP 0058; task T2). Implements the source hook squirreling's tryColumnScanAggregate already consumes.

icebergDataSource exposed only scan(), which materializes whole rows.
squirreling's scalar-aggregate fast path (tryColumnScanAggregate) can
compute COUNT/MIN/MAX/SUM/AVG and low-cardinality COUNT(DISTINCT ...) in
O(1)/O(cardinality) state by pulling a single column through the optional
AsyncDataSource.scanColumn hook -- but with no source implementing that
hook the engine fell back to buffering every scanned row, which OOMs on
large tables.

Implement scanColumn({ column, limit, offset, signal }) ->
AsyncIterable<ArrayLike<SqlPrimitive>> on the same object scan() returns.
It reuses the existing readDataFile row-group reader with a single
wanted column and yields one chunk of values per parquet row group, so
peak memory is bounded by a single row group's values rather than the
whole table.

scanColumn is never given a WHERE (the engine only takes this path when
the scan has no filter) and returns no appliedLimitOffset flag, so it
fully honors LIMIT/OFFSET itself: without deletes record_count is exact,
so whole files are skipped for OFFSET and the per-file read is bounded
for LIMIT; with deletes record_count is pre-delete, so LIMIT/OFFSET are
applied over the post-delete value stream. signal aborts between chunks,
mirroring scan().

Tests cover row-order equality with scan() for the same column,
LIMIT/OFFSET (including over post-delete values), prompt abort on a
pre-aborted signal, and that a scalar aggregate run through squirreling
actually invokes the hook (streaming fast path, not the buffering
fallback).

Part of the hypaware bounded-query-execution effort
(design: hypaware LLP 0058; task T2).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@philcunliffe

Copy link
Copy Markdown
Contributor Author

Dual-agent review — request_changes

  • Verdict: request_changes
  • Risk class: medium
  • Auto-merge advisory: 👎 thumbs down — verdict is request_changes; needs human-gated follow-up

Advisory only: no merge was attempted. Reviewed head 5eb0483.

Risk capstone

Cross-reference: reviewer findings vs high-risk surfaces

Source Finding (severity, evidence) Intersects
Codex major — N column re-reads for multi-aggregate (aggregates.js:287,329; icebergDataSource.js:234,256) Risks bullet 2; Direct callers
Codex minor — single-column projection path untested (execute.js:280; test:512,572) Direct callers
Claude minor — multi-data-file cross-file LIMIT/OFFSET branches untested (icebergDataSource.js:243-253,296) Risks bullet 1
Claude minor — mid-stream abort untested (icebergDataSource.js:236,269) Risks bullet 3
Codex review

Fix Validations

Iceberg source now satisfies Squirreling’s scanColumn hook

  • Status: correct
  • Evidence: src/sql/icebergDataSource.js:221, node_modules/squirreling/src/types.d.ts:77, node_modules/squirreling/src/types.d.ts:114
  • Assessment: The new method matches the optional AsyncDataSource.scanColumn(options): AsyncIterable<ArrayLike<SqlPrimitive>> contract and applies limit, offset, and signal internally.

Scalar aggregate fast path is no longer blocked by missing source hook

  • Status: correct
  • Evidence: node_modules/squirreling/src/execute/aggregates.js:256, node_modules/squirreling/src/execute/aggregates.js:267, node_modules/squirreling/src/execute/aggregates.js:329, test/sql/icebergDataSource.test.js:572
  • Assessment: Existing handling explicitly bailed out when table.scanColumn was absent, so the previous fallback would not resolve the bounded-memory goal. The new test verifies aggregate execution invokes the hook.

Findings

3) Change Impact / Blast Radius

  • Severity: major
  • Confidence: high
  • Evidence: node_modules/squirreling/src/execute/aggregates.js:285, node_modules/squirreling/src/execute/aggregates.js:287, node_modules/squirreling/src/execute/aggregates.js:329, src/sql/icebergDataSource.js:234, src/sql/icebergDataSource.js:256, test/sql/icebergDataSource.test.js:590
  • Why it matters: Enabling scanColumn makes multi-aggregate queries use the fast path, but Squirreling currently calls table.scanColumn once per aggregate expression, so the added five-aggregate test can read the same Parquet column five times instead of one buffered scan.
  • Suggested fix: Coalesce aggregate specs by { column, limit, offset } in the fast path and compute all requested aggregate functions from one column stream, or add an explicit characterization test and release note if the repeated scans are accepted.

9) Test Evidence Quality

  • Severity: minor
  • Confidence: high
  • Evidence: node_modules/squirreling/src/execute/execute.js:280, node_modules/squirreling/src/execute/execute.js:283, test/sql/icebergDataSource.test.js:512, test/sql/icebergDataSource.test.js:572
  • Why it matters: The hook is also used for plain single-column unfiltered scans, not only aggregates, but the new tests cover direct source draining and aggregate SQL only.
  • Suggested fix: Add an executeSql test for SELECT "Popularity Rank" FROM bunnies LIMIT ... OFFSET ... with a spy proving scanColumn is used and rows match the normal scan path.

No Finding

  1. Behavioral Correctness
  2. Contract & Interface Fidelity
  3. Concurrency, Ordering & State Safety
  4. Error Handling & Resilience
  5. Security Surface
  6. Resource Lifecycle & Cleanup
  7. Release Safety
  8. Architectural Consistency
  9. Debuggability & Operability

Evidence Bundle

  • Changed hot paths: src/sql/icebergDataSource.js:221, src/sql/icebergDataSource.js:230, src/sql/icebergDataSource.js:234, src/sql/icebergDataSource.js:256, src/sql/icebergDataSource.js:289
  • Impacted callers: node_modules/squirreling/src/execute/aggregates.js:256, node_modules/squirreling/src/execute/aggregates.js:287, node_modules/squirreling/src/execute/aggregates.js:329, node_modules/squirreling/src/execute/execute.js:280
  • Impacted tests: test/sql/icebergDataSource.test.js:512, test/sql/icebergDataSource.test.js:527, test/sql/icebergDataSource.test.js:542, test/sql/icebergDataSource.test.js:561, test/sql/icebergDataSource.test.js:572
  • Unresolved uncertainty: I did not run the test suite; I also did not inspect readDataFile internals due the 5-file review limit. Mid-stream abort behavior and exact row-group chunking are not asserted beyond the direct source-level checks.
Claude review

Claude review

Multi-data-file cross-file LIMIT/OFFSET branches are never exercised

  • Severity: minor
  • Confidence: 82
  • Evidence: test/sql/icebergDataSource.test.js (scanColumn block); src/sql/icebergDataSource.js:243-253,296
  • Why it matters: Both tested snapshots (bunnies v2 = 1 data file/21 rows; v4 = 1 data file/21 rows, 6 position-deletes) resolve to a single data file, so the cross-file paths never run: whole-file OFFSET skip-then-resume into the next file (remainingSkip -= recordCount; continue then partial fileRowStart on the next entry), the cross-file LIMIT early-break (if (stop || remaining <= 0) break at :296), and a fileRowEnd bound spanning into a later file. Multi-file tables are the common production case and exactly where off-by-one offset bugs hide; the logic was independently traced and verified correct by inspection, so this is a coverage gap, not a known defect.
  • Suggested fix: Add a scanColumn case over an existing no-delete multi-file fixture (e.g. test/files/hyperparam-iceberg/spark/rename_column — 3 data files, 0 deletes) with an OFFSET that crosses a whole-file boundary and a LIMIT satisfied before the last file, asserting equality against the scan() oracle.

Only the pre-aborted signal is tested; mid-stream abort is not

  • Severity: minor
  • Confidence: 80
  • Evidence: test/sql/icebergDataSource.test.js (abort test drains a pre-aborted controller); src/sql/icebergDataSource.js:236,269
  • Why it matters: The JSDoc (icebergDataSource.js:212) promises "signal aborts between chunks, mirroring scan," but only the at-start guard (:227) is exercised; the between-file (:236) and between-chunk (:269) abort checks have no coverage.
  • Suggested fix: Add a test that aborts after consuming the first chunk and asserts the iterator throws AbortError.

Reviewers: Codex (gpt-5.5) + Claude (5 parallel subagents). Correctness (row-order, LIMIT/OFFSET-over-deletes, abort, one-row-group memory bound) verified clean; the request_changes is driven by a blast-radius/efficiency note and test-coverage gaps, not a correctness defect.

… hook

Close the three review-flagged coverage gaps on scanColumn (test-only;
production scanColumn was reviewed correct and is unchanged):

- Cross-file LIMIT/OFFSET over the multi-data-file spark/rename_column
  table (3 data files) — the only fixture exercising the whole-file
  OFFSET skip-and-resume and the cross-file LIMIT early-break; bunnies
  v2/v4 both collapse to a single data file. Asserted against the
  scan({columns:[c]}) oracle slice, including chunk-count proof that the
  last file is never opened under a satisfied LIMIT.
- Mid-stream abort: consume the first chunk, then abort, and assert the
  next pull rejects at the between-file guard (same error scan raises).
- Plain single-column SELECT ... LIMIT/OFFSET routed through the
  scanColumn hook (execute.js fast path), asserting the hook fires and
  rows match the hook-disabled scan() path.
- Characterization (pinned, not endorsed): N aggregates on one column
  currently re-scan it N times — an upstream squirreling coalescing
  opportunity, not an icebird bug.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@philcunliffe

Copy link
Copy Markdown
Contributor Author

🤖 neutral: reviewed, green, mergeable — held for your merge

Part of the hypaware bounded-query-execution effort (design hypaware LLP 0058, task T2). Driven to held:

  • Dual-review (Codex + Claude): correctness verified YES on all axes — row order matches scan({columns:[c]}), LIMIT/OFFSET pushdown is correct including the post-delete (position-delete) case, signal aborts between files/chunks, and peak memory is bounded to one parquet row group (nothing accumulated across groups).
  • Round-2 fix (test-only, no production change): closed the three review test-coverage gaps — added cross-file LIMIT/OFFSET over a real multi-data-file table (spark/rename_column, 3 data files), a mid-stream abort test, and a plain single-column SELECT … LIMIT/OFFSET through the hook. Plus a characterization test pinning the multi-aggregate behavior below.
  • State: head 2a17d7c, MERGEABLE / CLEAN, CI green (lint + test + typecheck; vitest 586 pass), tsc clean.

One non-blocking follow-up (lives upstream in squirreling, not this diff): when a query runs N aggregates on the same column (COUNT/MIN/MAX/SUM/AVG), squirreling's tryColumnScanAggregate invokes scanColumn once per aggregate spec → N column re-reads. A memory-for-IO tradeoff; the coalescing fix (group specs by {column,limit,offset} into one stream) belongs in squirreling. Filed separately.

Merge is yours — neutral never merges. When you merge + publish a new icebird version, neutral bumps the hypaware pin. Held for your call.

const skip = offset ?? 0
const take = limit ?? Infinity
return {
async *[Symbol.asyncIterator]() {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this a weird way to do it vs async *scanColumn() declaration, but I don't see any reason this wouldn't work.

@philcunliffe
philcunliffe merged commit 6c3a775 into master Jul 1, 2026
6 checks passed
@platypii
platypii deleted the feat/scan-column branch July 11, 2026 21:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants