Uh oh!
There was an error while loading. Please reload this page.
createDataSource declares the scannable shape it already returns - #985
Merged
Conversation
#982 widened `AsyncDataSource` (squirreling 0.15.3 to 0.16.1) into a row-or-batch union and added `ScannableDataSource` for the stronger shape Hypaware's adapters actually guarantee. It then updated the internals: every dataset plugin annotates `ScannableDataSource`, and `dataSourceForTable` declares `Promise<ScannableDataSource | null>`. The two plugin-facing interfaces were left on the wide union, so a consumer that receives a source through `DatasetRegistration` or `SinkQueryReader` is handed a value whose `scan()` and `columns` are possibly undefined, even though every source Hypaware produces has them. Nothing inside this repo noticed: the one production caller passes the source straight to squirreling, which accepts the union by design, and the one test that iterates rows itself narrowed at the call site instead (`assert.ok(scan)` in claude-telemetry-events-dataset.test.js, added by the same PR). Downstream it is not avoidable. hypaware-server consumes these interfaces and iterates rows in five places; its typecheck went red the first time CI resolved a client with this surface, against server code that had not changed. Declaring what the implementations already promise. Type-only: no runtime behaviour changes, and no source Hypaware builds is affected, since `prepareScan` is always added alongside `scan`, never instead of it. Co-Authored-By: Claude Opus 5 <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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
What
Two return types in
hypaware-plugin-kernel-types.d.tsmove fromAsyncDataSourceto theScannableDataSourcethat #982 introduced:DatasetRegistration.createDataSourceandSinkQueryReader.createDataSource.Type-only. No
.jstouched, no runtime behaviour changes.Why
#982 bumped squirreling 0.15.3 to 0.16.1, where
AsyncDataSourcebecame aunion: a source implements either
scan()(row) orprepareScan()+schema(native batch). It added
ScannableDataSourcefor the stronger shape, notingthat Hypaware's "storage, union, visibility, and legacy parquet adapters all
guarantee this stronger shape even when they also expose prepared native
batches", and migrated the internals accordingly: every dataset plugin
annotates
ScannableDataSource, anddataSourceForTabledeclaresPromise<ScannableDataSource | null>.The two plugin-facing interfaces stayed on the wide union. So the contract a
consumer reads promises less than every implementation delivers: receive a
source through
DatasetRegistration, andscan()andcolumnsare possiblyundefined.
Why CI here stayed green
Nothing in this repo consumes that contract the way a downstream repo does:
src/core/query/sql.js:440) hands the sourcestraight to squirreling as a
tablesentry, and squirreling accepts theunion by design
than fixing the declaration, in Use native Icebird batches for faster queries #982 itself:
Everything else implementing
createDataSourceis a producer, alreadyannotated with the strong type.
Downstream
hypaware-serverconsumes these interfaces and iterates rows in five places(
plugins/github/src/dataset.js, plus three test files). Its typecheck wentred the first time CI resolved a client containing #982, against server code
that had not changed, because it consumes the client via
file:../hypawareand checks out this repo's default branch on every run. This change fixes it
with no server-side change at all.
Tradeoff worth a look
Narrowing a return type is free for callers but tightens the contract on
implementers: after this, anything implementing
DatasetRegistrationorSinkQueryReadermust return a source carryingscan(), so a pluginreturning batch-only (
schema+prepareScan, noscan) becomes a typeerror.
Every in-repo implementation already satisfies it, and
prepareScanis addedalongside
scanand never instead of it (ai-gateway/src/dataset.js:244wraps and keeps both), so this looks like the intent of #982 rather than a
restriction. But it is an API decision rather than a typo fix, so please push
back if batch-only registrations are meant to be expressible.
If you would rather keep the wide interface, the alternative is that every
consumer narrows at its call sites the way the telemetry test does above.
Verification
Static: all in-repo implementations already return the strong shape, either
annotated
ScannableDataSourceor returningdataSourceForTable, whichdeclares it. This PR's CI run is the actual gate for typecheck and test.
🤖 Generated with Claude Code