fix: a scalar stream is an exchange over HTTP too, and a CI lane that proves it - #8
Merged
Merged
Conversation
`InitScalar` returned `RpcStream(..., InputSchema: null)`. A scalar call is an
EXCHANGE — DuckDB pushes one batch of argument columns per turn and reads one
batch of results back — and the only marker on the wire that says so is a
non-empty `InputSchema`. The canonical Python worker sets exactly this
(`input_schema = request.bind_call.input_schema`, in `Worker._init_stream`'s
`ScalarFunctionGenerator` branch); this port had the decoded schema in hand two
lines earlier and dropped it.
vgi-rpc's HTTP dispatch classifies a stream with
`InputSchema is not { FieldsList.Count: > 0 }` as a PRODUCER, and folds a
producer's first tick into the `/init` request itself, driving it with a
zero-COLUMN batch. So every scalar function served over HTTP was called before
DuckDB had sent a single argument row, and threw out of `RecordBatch.Column(0)`:
System.ArgumentOutOfRangeException: Index was out of range.
at Apache.Arrow.RecordBatch.Column(Int32 i)
at ...ExampleWorker.Scalar.DoubleFunction.Process(...)
at ...Internal.ScalarStreamState.ExchangeAsync(...)
at ...Http.RpcHttpEndpoints.HandleStreamInitAsync(...)
26 integration files failed there first, across scalar/, settings/,
global_functions/, overload/, aggregate/, cache/, connection_string.test and
unary_error_propagation.test. The pipe/launcher transport never synthesizes a
turn — it only delivers batches the client actually sent — so the launcher lane
was green throughout, which is why a published release carries this.
The regression test asserts the invariant twice: once as the schema it should
declare, and once spelled exactly as the transport spells the predicate, because
that predicate is what actually decides.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…s green without running
The launcher lane reported 333/334 while **81 of 334 files were red over HTTP**
on the same worker build. A worker's two transports are separate dispatch
implementations — the pipe server loop and the HTTP endpoint handlers share the
function code and nothing else — so one lane is not evidence about the other.
`TRANSPORT=launch|http` picks; CI runs both as a matrix, neither fail-fast.
Two things differ by lane, both for reasons intrinsic to the test:
`database_worker/package.test` packages `$VGI_TEST_WORKER` as an EXECUTABLE and
runs it, which a URL is not, so it is excluded on http (and still required on
launch); `VGI_REQUIRE_LAUNCHER_TRANSPORT` is set only on launch.
Then the guards, which matter more than the lane. A failed `require`/`require-env`
is a SKIP, not a failure, so "all tests passed" alone proves nothing ran — and on
an HTTP lane there is a second, sharper version of that hazard: DuckDB's
sqllogictest runner defaults `ignore_error_messages` to
{"HTTP", "Unable to connect"}, so a worker answering 500 produces an error
message containing "HTTP" and the statement is SKIPPED rather than failed. That
is not theoretical — an intermediate state of this work turned 118 assertions
into silent skips while the summary still read `0 failed`, and two files
(`table_in_out/echo/all_types`, `echo/union_tags`) had been red on that lane for
as long as it existed while reading as "2 skipped". The lane now fails on: no
test cases matched; an unreadable or below-floor assertion count; the http worker
dying mid-run; and ANY assertion lost to that default — zero, not a tolerance,
established by re-running the whole suite once with the directive disabled and
finding nothing that legitimately errors with an HTTP-containing message.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…mpile floor The scalar-stream fix in this repo closes 26 of the 81 integration files that were red over HTTP. The other 55 were four defects in the transport, fixed in QueryFarm.VgiRpc 0.10.2: HTTP dispatch never read `OutputCollector.EmittedMetadata` (so `vgi.cache.*`, `vgi_partition_values#b64`, `vgi_batch_index` and `vgi_rpc.parent_row#b64` were dropped on that transport alone, and the extension rejected the batches that had declared those features); the `/init` tick carried null input metadata (so conditional revalidation never saw `vgi.cache.if_none_match`); the transport's framing keys crossed into user metadata in one direction and let a worker overwrite its own continuation cursor in the other; and `ValueCodec.EmptyRow` — written as the continuation-token sentinel — threw for FixedSizeBinary, Union and Interval columns and for any ENUM whose dictionary index was not Int16. Worth stating plainly in the pin comment, because the usual signal is absent: this repo BUILDS against 0.10.1 and its unit tests PASS. Only the HTTP integration lane fails, which is exactly why that lane now exists. Lowering the pin to make a restore resolve would put the silent-drop behaviour back. Verified against the published package, not a sibling source build: QueryFarm.VgiRpc/0.10.2 resolves as `package` in project.assets.json, and the full integration suite is green on both lanes from that restore. Co-Authored-By: Claude Opus 5 (1M context) <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 free
to 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.
Published
vgi-csharpfails 81 of 334 integration files over the HTTP transport while thelauncher lane reports 0 failed on the same worker build. Measured on Linux aarch64 against the
pinned
Query-farm/vgisuite (5d45bb4) and the published community extension.This PR carries the 26 files that are this repo's own, the dependency bump that carries the other
55, and the CI lane that would have caught all of it.
The worker bug (26 files)
InitScalarreturnedRpcStream(..., InputSchema: null). A scalar call is an EXCHANGE — DuckDBpushes one batch of argument columns per turn and reads one batch back — and the only marker on the
wire that says so is a non-empty
InputSchema. The canonical Python worker sets exactly this(
input_schema = request.bind_call.input_schema); this port had the decoded schema in hand twolines earlier and dropped it.
vgi-rpc's HTTP dispatch classifies
InputSchema is not { FieldsList.Count: > 0 }as a PRODUCER andfolds a producer's first tick into
/init, driving it with a zero-COLUMN batch. So every scalarfunction over HTTP was called before DuckDB had sent an argument row and threw out of
RecordBatch.Column(0). The pipe transport never synthesizes a turn, so the launcher lane was greenthroughout — which is why a published release carries this.
The transport bugs (55 files)
Four defects in
RpcHttpEndpoints, fixed in QueryFarm.VgiRpc 0.10.2(Query-farm/vgi-rpc-csharp#5): per-batch
EmittedMetadatawas never read on the HTTP path, sovgi.cache.*,vgi_partition_values#b64,vgi_batch_indexandvgi_rpc.parent_row#b64weredropped and the extension rejected the batches that had declared those features; the
/inittickcarried null input metadata, so conditional revalidation never saw its validators; framing keys
crossed into user metadata one way and let a worker overwrite its own continuation cursor the other;
and
ValueCodec.EmptyRowthrew for FixedSizeBinary/Union/Interval columns and for any ENUM whosedictionary index was not Int16.
The pin comment says plainly that this is a correctness floor, not a compile floor: this repo
builds against 0.10.1 and its unit tests pass. Only the HTTP lane fails.
The CI gap
ci/run-integration.shwas a single launcher lane, so none of this was ever exercised. It now takesTRANSPORT=launch|httpand CI runs both as a matrix. Two things differ by lane, both intrinsic tothe test rather than to the worker:
database_worker/package.testpackages$VGI_TEST_WORKERas anexecutable and runs it (a URL is not one), so it is excluded on http and still required on launch;
VGI_REQUIRE_LAUNCHER_TRANSPORTis set only on launch.The guards matter more than the lane. A failed
require/require-envis a SKIP, and on an HTTP lanethere is a sharper version of that: DuckDB's sqllogictest runner defaults
ignore_error_messagesto{"HTTP", "Unable to connect"}, so a worker answering 500 produces an error message containing"HTTP" and the statement is skipped, not failed. Not theoretical — an intermediate state of this
work turned 118 assertions into silent skips while the summary still read
0 failed, and twofiles (
table_in_out/echo/all_types,echo/union_tags) had been red on that lane for as long as itexisted while reading as "2 skipped". The lane now fails on: no test cases matched; an unreadable or
below-floor assertion count; the http worker dying mid-run; and any assertion lost to that
default — zero, not a tolerance, established by re-running the whole suite once with the directive
disabled and finding nothing that legitimately errors with an HTTP-containing message.
Verified against the published package
Not a sibling source build —
QueryFarm.VgiRpc/0.10.2resolves aspackageinproject.assets.json, from nuget.org, on a clean restore.dotnet test: 187 passing. The newScalarStreamShapeTestsasserts the invariant twice — once asthe schema the stream should declare, and once spelled exactly as the transport spells the predicate,
because that predicate is what actually decides.
Known follow-up
VGI_HTTP_TRANSPORTis deliberately still unset, leaving five HTTP-only files skipped. Four wereverified to pass as-is; the fifth (
cache/identity_isolation.test) needs the worker to answer as anamed principal, and
Worker.RunHttpAsyncexposes noauthenticatehook at all — a worker writtenagainst this port cannot authenticate an HTTP caller. That is a product API change and belongs in its
own commit;
ci/README.mdrecords it.🤖 Generated with Claude Code