Skip to content

Commit 2b8e4fc

Browse files
committed
fix(client): table_in_out_function(has_finalize=False) skips FINALIZE init()
Real cross-SDK interop bug found live against the vgi-open-meteo worker (TypeScript SDK, https://vgi-open-meteo.rusty-bb6.workers.dev): every table_in_out_function() call unconditionally sent a FINALIZE-phase init(), even for a blended row-transform function, which never has a finalize stage (enforced at resolve_metadata() -- overriding finish()/finalize() on a RowTransformFunction is a TypeError). The Python fixture worker happens to no-op that gracefully, so vgi-python's own test suite never caught it -- but the TypeScript worker rejects it outright: "a blended row-transform function has no FINALIZE phase (it is a per-row map; has_finalize is advertised false)." Every literal or correlated-column call against that worker failed. New has_finalize: bool = True parameter, defaulting to True so every existing caller is unaffected. When False, the FINALIZE init() is never sent at all -- not just tolerated when a worker happens to no-op it -- mirroring the DuckDB C++ extension's own approach of conditionally registering in_out_function_final in the first place. New tests/table_in_out/test_blended_exchange.py::TestHasFinalizeSkip proves output equivalence with/without the flag on both transports (the cross-SDK rejection itself is TypeScript-only, not reproducible against the Python fixture worker locally). Full suite: 2509 passed, 101 skipped.
1 parent 55ea6a6 commit 2b8e4fc

2 files changed

Lines changed: 65 additions & 1 deletion

File tree

‎tests/table_in_out/test_blended_exchange.py‎

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -238,3 +238,45 @@ def test_row_sum_varargs(self, fixture_worker: str) -> None:
238238

239239
assert len(results) == 1
240240
assert results[0].column("row_sum").to_pylist() == [3.0, 5.0]
241+
242+
243+
class TestHasFinalizeSkip:
244+
"""`has_finalize=False` skips the FINALIZE `init()` entirely for a no-finalize function.
245+
246+
Real interop bug found live against a non-Python worker SDK: the client
247+
unconditionally sent a FINALIZE-phase init() for every table-in-out call,
248+
which the Python fixture worker happens to no-op gracefully but a
249+
TypeScript-SDK worker rejects outright ("a blended row-transform function
250+
has no FINALIZE phase"). `has_finalize=False` avoids sending that init()
251+
at all rather than depending on every SDK tolerating an unexpected one.
252+
Output must be identical either way for a function that has no finalize
253+
stage (there is nothing to skip yielding) -- this is the equivalence this
254+
suite can prove locally; the cross-SDK rejection itself is TypeScript-only.
255+
"""
256+
257+
def test_output_identical_with_and_without_has_finalize(self, client_transport: Any) -> None:
258+
"""Same output whether the FINALIZE init() is sent or skipped, both transports."""
259+
input_schema = make_schema([pa.field("n", pa.int64())])
260+
batch = pa.RecordBatch.from_pydict({"n": [0, 1, 3, 2]}, schema=input_schema)
261+
262+
with client_transport() as client:
263+
with_finalize = list(
264+
client.table_in_out_function(
265+
function_name="blended_explode",
266+
schema_name="main",
267+
input=iter([batch]),
268+
)
269+
)
270+
with client_transport() as client:
271+
without_finalize = list(
272+
client.table_in_out_function(
273+
function_name="blended_explode",
274+
schema_name="main",
275+
input=iter([batch]),
276+
has_finalize=False,
277+
)
278+
)
279+
280+
assert len(with_finalize) == 1
281+
assert len(without_finalize) == 1
282+
assert with_finalize[0].column("i").to_pylist() == without_finalize[0].column("i").to_pylist()

‎vgi/client/client.py‎

Lines changed: 23 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1915,6 +1915,7 @@ def table_in_out_function(
19151915
settings: dict[str, Any] | None = None,
19161916
transaction_opaque_data: bytes | None = None,
19171917
parent_row_callback: Callable[[list[int]], None] | None = None,
1918+
has_finalize: bool = True,
19181919
) -> Generator[pa.RecordBatch]:
19191920
"""Invoke a table-in-out function on the worker and stream results.
19201921
@@ -1956,6 +1957,20 @@ def table_in_out_function(
19561957
`FunctionInfo.input_from_args`); leave unset for ordinary
19571958
table-in-out functions, which have no provenance concept and
19581959
may legitimately change row count.
1960+
has_finalize: Whether this function declares a FINALIZE stage
1961+
(`FunctionInfo.has_finalize`). Defaults to `True` — every
1962+
caller before this parameter existed got a FINALIZE-phase
1963+
`init()` unconditionally, so this preserves that exactly.
1964+
Pass `False` for a function known to have no finalize (every
1965+
blended `RowTransformFunction` — `has_finalize` is always
1966+
false for those, enforced at `resolve_metadata()`) to skip
1967+
the FINALIZE `init()` entirely, not just send one expecting
1968+
an empty reply. Confirmed load-bearing, not cosmetic: some
1969+
worker SDKs (e.g. the TypeScript one) actively reject an
1970+
unexpected FINALIZE `init()` for a function that never
1971+
advertised `has_finalize`, rather than silently no-op'ing
1972+
it — the C++ DuckDB extension avoids this the same way, by
1973+
conditionally registering `in_out_function_final` at all.
19591974
19601975
Yields:
19611976
Output `RecordBatch`es from the function. In single-worker mode, output
@@ -2019,7 +2034,14 @@ def table_in_out_function(
20192034
# Close secondary workers
20202035
self._close_secondary_workers()
20212036

2022-
# Finalize on primary worker
2037+
# Finalize on primary worker — skipped entirely (not just
2038+
# sent expecting an empty reply) when the function has no
2039+
# finalize stage. See has_finalize's docstring: some worker
2040+
# SDKs reject an unadvertised FINALIZE init() outright.
2041+
if not has_finalize:
2042+
_logger.debug("skipping_finalize_no_finalize_stage")
2043+
return
2044+
20232045
_logger.debug("finalizing_primary_worker")
20242046
yield from self._finalize_primary_worker(
20252047
bind_request,

0 commit comments

Comments
 (0)