Skip to content

Commit 81760e5

Browse files
committed
fix(table-in-out): a 0-row batch is not end-of-stream in FINALIZE
A multi-batch streaming FINALIZE returned only its FIRST batch over HTTP, and did so silently: the rows that arrived were correct, so nothing but a count revealed the missing ones. Measured against the Python worker, a flush of 200 batches returned 1 row with the right SUM. VgiTableInOutFinalize ended the stream on `!batch || batch->num_rows() == 0`. Null is end-of-stream; ZERO ROWS is not — it is a producer turn that carried no data. The FINALIZE init response buffers its batches and the last one is empty, so the first read returned the data batch and the second returned that empty one, ending the stream while `stream_state_token_` was still live. The continuation request was never issued at all: 2 reads, 1 emitted batch and 0 producer exchanges, against 6 reads and 5 batches for the same query over subprocess, where the empty batch never arrives because the IPC reader returns null at EOS. That is why the bug was invisible on the transport everything is developed against. Now: read until a non-empty batch or a null. Skipping empties inside the call rather than returning HAVE_MORE_OUTPUT keeps the decision here, so it does not depend on how DuckDB treats an empty output chunk from FinalExecute. The loop terminates on null — over HTTP once a response carries no continuation token, over subprocess at IPC EOS. Adds multi_batch_finalize.test, backed by the new `multi_batch_finish` fixture in vgi-python and vgi-rust. It emits one batch per input row so a wrong SUM means a batch's CONTENTS were lost and a wrong COUNT means a whole BATCH was — the second is what catches a truncated flush. Every existing finalize fixture in every SDK emits exactly one batch, which is the easy case (one batch fits in one lock-step HTTP turn and never needs a continuation), and that is precisely why nothing caught this. Green on both transports; the whole HTTP table_in_out suite still passes. Pins the CI fixtures ref to v0.29.1, the vgi-python tag that has the fixture.
1 parent 5b89945 commit 81760e5

6 files changed

Lines changed: 149 additions & 8 deletions

File tree

‎.github/workflows/integration.yml‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,7 @@ jobs:
4747
env:
4848
VGI_INSTALL_FIXTURES: "1"
4949
VGI_FIXTURES_DIR: /tmp/vgi-python
50-
VGI_FIXTURES_REF: ${{ inputs.fixtures_ref || 'v0.29.0' }}
50+
VGI_FIXTURES_REF: ${{ inputs.fixtures_ref || 'v0.29.1' }}
5151
GEN: ninja
5252
USE_MERGED_VCPKG_MANIFEST: "1"
5353
VCPKG_BINARY_SOURCES: "clear;x-gha,readwrite"

‎CLAUDE.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -250,7 +250,7 @@ accumulated unnoticed, so it is wired shut:
250250
works for the Linux in-docker leg, where the env vars are forwarded into the
251251
container and the install happens inside it.
252252
- `VGI_FIXTURES_REF` pins the worker revision to a **vgi-python release tag**
253-
(currently `v0.29.0`), not `main`. **Bump it in lockstep with the protocol
253+
(currently `v0.29.1`), not `main`. **Bump it in lockstep with the protocol
254254
version** — it must name a release speaking the same `VGI_PROTOCOL_VERSION` as
255255
`src/`, or ATTACH fails with a message naming both versions (the right failure,
256256
but it fails the build). Override per-run with the `fixtures_ref`

‎Makefile‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -423,7 +423,7 @@ VGI_RUST_DIR ?= $(HOME)/Development/vgi-rust
423423
# conformance lane, so it gets the same two gates the rust/go/java/typescript
424424
# lanes have.
425425
#
426-
# Python runs 303 today.
426+
# Python runs 304 today.
427427
#
428428
# VGI_TEST_DEDICATED_WORKER is DECLARED as an expected skip rather than wired.
429429
# Two tests want a non-launcher worker, and pointing the var at the raw `uv run`

‎src/vgi_table_in_out_impl.cpp‎

Lines changed: 31 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1165,10 +1165,38 @@ OperatorFinalizeResultType VgiTableInOutFinalize(ExecutionContext &context, Tabl
11651165
{{"conn", conn->GetConnIdHex()},
11661166
{"worker_path", bind_data.worker_path()},
11671167
{"function_name", bind_data.function_name}});
1168-
auto output_batch = conn->ReadDataBatch();
1168+
// EOS is a NULL batch. A 0-ROW batch is not — it is a producer turn that
1169+
// carried no data, and treating it as end-of-stream silently truncated every
1170+
// multi-batch FINALIZE over HTTP.
1171+
//
1172+
// How it presented: the FINALIZE init response buffers its batches, and the
1173+
// last one is empty. Over subprocess that empty batch never arrives (the IPC
1174+
// reader returns null at EOS), so the bug was invisible there; over HTTP the
1175+
// first read returned the data batch and the second returned the empty one,
1176+
// which ended the stream while `stream_state_token_` was still live. The
1177+
// continuation request was never issued at all — measured as 2 reads, 1
1178+
// emitted batch and 0 producer exchanges for a flush of 200. A worker whose
1179+
// finalize emits one batch was unaffected, which is every other fixture, in
1180+
// every SDK.
1181+
//
1182+
// It failed SILENTLY rather than loudly: the rows that did arrive were
1183+
// correct, so only a count betrayed the missing ones.
1184+
//
1185+
// Skipping empties here rather than returning HAVE_MORE_OUTPUT keeps the
1186+
// decision inside this call, so it does not depend on how DuckDB treats an
1187+
// empty output chunk from FinalExecute. The loop terminates because an
1188+
// exhausted producer returns null: over HTTP once the response carries no
1189+
// continuation token, over subprocess at IPC EOS.
1190+
std::shared_ptr<arrow::RecordBatch> output_batch;
1191+
for (;;) {
1192+
output_batch = conn->ReadDataBatch();
1193+
if (!output_batch || output_batch->num_rows() > 0) {
1194+
break;
1195+
}
1196+
}
11691197

1170-
if (!output_batch || output_batch->num_rows() == 0) {
1171-
// No more output - clean up
1198+
if (!output_batch) {
1199+
// End of stream - clean up
11721200
release_and_finish();
11731201
return OperatorFinalizeResultType::FINISHED;
11741202
}

‎test/sql/integration/table/function_registration.test‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -72,6 +72,12 @@ ATTACH 'example' AS example (TYPE vgi, LOCATION '${VGI_TEST_WORKER}');
7272
# 1 parallel-streaming-finalize fixture (substream_partial_sum, a streaming
7373
# TableInOutFunction whose per-substream finalize emits this substream's partial
7474
# sum — see table_in_out/parallel_finalize.test)
75+
# 1 multi-batch-finalize fixture (multi_batch_finish, a streaming
76+
# TableInOutFunction whose finalize emits ONE BATCH PER INPUT ROW. Every other
77+
# finalize fixture emits a single batch, which is the easy case: over HTTP a
78+
# producer is lock-step, so one batch completes in one turn and never needs a
79+
# continuation. Two or more do — and that path silently truncated to the first
80+
# batch until this fixture existed. See table_in_out/multi_batch_finalize.test)
7581
# 7 blended ("UNNEST-style") fixtures (geo_encode + geo_encode3, arity-overloaded
7682
# RowTransformFunctions sharing the name "geo_encode"; row_sum, a VARARGS blended
7783
# fn; blended_drop, a 1->0 scan-mode edge-case map — positional args ARE the
@@ -132,13 +138,13 @@ ATTACH 'example' AS example (TYPE vgi, LOCATION '${VGI_TEST_WORKER}');
132138
# re-emits its splits across pages); and four pair splits with another
133139
# feature (split_batch_index, split_partitioned, split_dynamic_filter,
134140
# split_cacheable). Back splits/*.test.
135-
# = 161
141+
# = 162
136142
query I
137143
SELECT COUNT(*)
138144
FROM duckdb_functions()
139145
WHERE database_name = 'example' AND function_type = 'table';
140146
----
141-
161
147+
162
142148

143149
# The 34 pure table functions (no TABLE parameter type)
144150
# Note: make_series appears 5 times (5 overloads), make_pairs appears 3 times,
Lines changed: 107 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,107 @@
1+
# name: test/sql/integration/table_in_out/multi_batch_finalize.test
2+
# description: A streaming table-in-out FINALIZE that emits MANY batches.
3+
#
4+
# Every other finalize fixture in every SDK returns exactly ONE batch, and one
5+
# batch is the easy case: over HTTP a producer is strictly lock-step (one batch
6+
# per response), so a single-batch flush completes inside its one turn and never
7+
# needs a continuation token. A flush of TWO OR MORE batches does — and in
8+
# vgi-rust that path was a hard error instead of a pagination, which nothing
9+
# noticed for as long as no fixture emitted a second batch.
10+
#
11+
# `multi_batch_finish` emits one batch per input row the substream saw: `rows`
12+
# batches, the first carrying that substream's `total` and the rest carrying 0.
13+
# Splitting it that way makes the two failure modes tell themselves apart:
14+
#
15+
# * SUM(n) wrong -> a batch's CONTENTS were lost, duplicated or reordered.
16+
# * COUNT(*) wrong -> a BATCH was lost or repeated. This is the one that
17+
# catches a broken continuation, because a flush truncated after its first
18+
# batch still SUMS correctly and only the count betrays it.
19+
#
20+
# Both invariants hold however DuckDB fans the input across substreams (each
21+
# substream's partials sum to the whole, and its batch count equals its row
22+
# count), so nothing here assumes a thread or substream count.
23+
# group: [vgi_integration_table_in_out]
24+
25+
require-env VGI_TEST_WORKER
26+
27+
require vgi
28+
29+
# HTTP transport ATTACH needs httpfs loaded (else a "VGI HTTP transport requires
30+
# the httpfs extension" error — containing "HTTP" — makes the harness silently
31+
# skip the whole file). Harmless on subprocess.
32+
require httpfs
33+
34+
statement ok
35+
ATTACH 'example' AS ex (TYPE vgi, LOCATION '${VGI_TEST_WORKER}');
36+
37+
# ---------------------------------------------------------------------------
38+
# Single substream, many batches — the case the HTTP continuation must serve.
39+
# threads=1 keeps it to ONE substream, so one finalize really does flush 200
40+
# batches rather than 200 substreams flushing one batch each.
41+
# ---------------------------------------------------------------------------
42+
43+
statement ok
44+
SET threads=1;
45+
46+
# 200 input rows -> 200 finalize batches from a single flush.
47+
query II
48+
SELECT COUNT(*), SUM(n) FROM ex.multi_batch_finish((SELECT i AS n FROM range(1, 201) t(i)));
49+
----
50+
200 20100
51+
52+
# Two rows is the smallest input that needs a continuation at all: one batch
53+
# fits in a single HTTP turn, two do not.
54+
query II
55+
SELECT COUNT(*), SUM(n) FROM ex.multi_batch_finish((SELECT i AS n FROM range(1, 3) t(i)));
56+
----
57+
2 3
58+
59+
# One row still works — the single-batch flush keeps its one-turn drain.
60+
query II
61+
SELECT COUNT(*), SUM(n) FROM ex.multi_batch_finish((SELECT i AS n FROM range(1, 2) t(i)));
62+
----
63+
1 1
64+
65+
# Empty input: no rows accumulated, so no batches to flush.
66+
query I
67+
SELECT COUNT(*) FROM ex.multi_batch_finish((SELECT i AS n FROM range(1, 1) t(i)));
68+
----
69+
0
70+
71+
# ---------------------------------------------------------------------------
72+
# Parallel — several substreams, each flushing several batches. The invariants
73+
# are per-substream sums and counts, so they hold at any fan-out.
74+
# ---------------------------------------------------------------------------
75+
76+
statement ok
77+
SET threads=8;
78+
79+
statement ok
80+
CREATE TABLE big AS SELECT i AS n FROM range(1, 5001) t(i);
81+
82+
# sum(1..5000) = 12502500, and one output row per input row.
83+
query II
84+
SELECT COUNT(*), SUM(n) FROM ex.multi_batch_finish((SELECT n FROM big));
85+
----
86+
5000 12502500
87+
88+
# UNION ALL: each branch is its own substream, so this is several independent
89+
# multi-batch flushes unioned together.
90+
query II
91+
SELECT COUNT(*), SUM(n) FROM (
92+
SELECT n FROM ex.multi_batch_finish((SELECT n FROM big))
93+
UNION ALL
94+
SELECT n FROM ex.multi_batch_finish((SELECT n FROM big))
95+
);
96+
----
97+
10000 25005000
98+
99+
# pool false → a fresh worker per substream acquire, so a flush that depended on
100+
# state left behind on a shared worker would show up here.
101+
statement ok
102+
ATTACH 'example' AS ex2 (TYPE vgi, LOCATION '${VGI_TEST_WORKER}', pool false);
103+
104+
query II
105+
SELECT COUNT(*), SUM(n) FROM ex2.multi_batch_finish((SELECT n FROM big));
106+
----
107+
5000 12502500

0 commit comments

Comments
 (0)