fix(flight/flightsql): send bound parameters even when binding has zero rows - #3
Open
fornwall wants to merge 2 commits into
Open
fix(flight/flightsql): send bound parameters even when binding has zero rows#3fornwall wants to merge 2 commits into
fornwall wants to merge 2 commits into
Conversation
…ro rows hasBindParameters() ignored a paramBinding whose record batch had zero rows, so Execute/ExecuteUpdate/ExecutePoll skipped the DoPut entirely. The server therefore never saw the new binding and would execute the query with whatever parameters were previously bound, and it also never received the parameter schema needed to infer the result-set schema. A zero-row binding is meaningful: it replaces any prior binding and carries the parameter schema, so it must be sent. The IPC writer already handles both zero-row paths correctly (a zero-row record batch is written normally, and an empty RecordReader still emits the schema message on Close), so the fix is only to stop gating on NumRows() > 0. Surfaced by the ADBC validation suite's zero-row bind test, where the stale previously-bound parameters caused spurious rows. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01E4FW7ZCSdoHVcbH6tRYN2P
fornwall
added a commit
to fornwall/arrow-adbc
that referenced
this pull request
Jul 15, 2026
Executing a parameterized query with a bound parameter batch of zero rows (e.g. a DBAPI executemany with an empty parameter list) must return a result stream with zero rows - not execute the query with stale (or missing) parameters and return spurious rows. The result schema is deliberately not inspected; only the row count is asserted. Add a supports_bind_zero_rows() quirk (default true) so drivers that cannot satisfy this can opt out. The Flight SQL SQLite tests are enrolled: they pass with the arrow-go client fix that sends bound parameters even when the binding has zero rows (fornwall/arrow-go#3) and the matching example server; against arrow-go v18.6.0 the client silently skips the DoPut and the test fails with "missing argument with index 1". Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JA2PHSMV54nGRReD9zgfTT
fornwall
added a commit
to fornwall/arrow-adbc
that referenced
this pull request
Jul 15, 2026
Executing a parameterized query with a bound parameter batch of zero rows (e.g. a DBAPI executemany with an empty parameter list) must return a result stream with zero rows - not execute the query with stale (or missing) parameters and return spurious rows. The stream must also report a non-empty result schema rather than a zero-column placeholder (this is what the preceding SQLite reader fix addresses), but the schema contents are otherwise not inspected. Add a supports_bind_zero_rows() quirk (default true) so drivers that cannot satisfy this can opt out. The Flight SQL SQLite tests are enrolled: they pass with the arrow-go client fix that sends bound parameters even when the binding has zero rows (fornwall/arrow-go#3) and the matching example server; against arrow-go v18.6.0 the client silently skips the DoPut and the test fails with "missing argument with index 1". Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JA2PHSMV54nGRReD9zgfTT
fornwall
added a commit
to fornwall/arrow-adbc
that referenced
this pull request
Jul 15, 2026
…ult schema Executing a parameterized query with a bound parameter batch of zero rows (e.g. a DBAPI executemany with an empty parameter list) must return a result stream with zero rows and exactly the same schema as a non-empty execution of the same query - not execute the query with stale (or missing) parameters, and not report a placeholder schema. The schema is compared deeply (format, name, flags, children, dictionary) against a reference execution with one bound row. Without the preceding SQLite reader fix this catches the zero-column placeholder schema (format/flags/n_children all mismatch at the root), the same malformed schema that segfaults pyarrow when imported from Python. PostgreSQL passes as-is (verified against a live server). Add a supports_bind_zero_rows() quirk (default true) so drivers that cannot satisfy this can opt out, and opt the Flight SQL SQLite tests out: even with the arrow-go client fix that sends zero-row bindings (fornwall/arrow-go#3), the example server cannot infer concrete result column types without any bound values and reports SQLite's dense-union fallback ("+ud:0,1,2") where an execution with a bound row reports the concrete type ("l"). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JA2PHSMV54nGRReD9zgfTT
Now that the client sends zero-row parameter bindings, the example server must distinguish "a binding with zero rows" from "no binding at all": DoPutPreparedStatementQuery stores an empty (but non-nil) params slice, and DoGetPreparedStatement previously treated len(params) == 0 as "execute once with no arguments", which fails with "missing argument with index 1" for parameterized queries. A zero-row binding now executes the query zero times, yielding zero rows, while still reporting the real result schema. That schema is probed by executing the statement once with a placeholder per parameter and discarding whatever rows the probe produces. The placeholders are typed, non-NULL zero values derived from the bound parameter schema rather than plain NULLs, which is subtle enough to be worth spelling out. SQLite has no static type for the result column of a parameter-only expression such as "SELECT ?": the column type comes from the value bound at execution time, and a NULL leaves the column untyped, which getArrowType reports as the dense-union fallback (+ud:0,1,2). Probing with NULL would therefore advertise one schema for a zero-row binding and another for a non-empty execution of the same query -- exactly the invariant this change exists to preserve. The parameter schema is retained for this purpose even when the binding carries no rows. Parameters with no obvious zero value (a dense-union parameter, say) are still bound as NULL; their result columns are typed by SQLite's declared types where those exist. TestCommandPreparedStatementQueryZeroRowParamsUntypedColumn covers this: it binds "SELECT ?" with one int64 row and then with zero rows, and asserts both report an int64 result column. The existing TestCommandPreparedStatementQueryZeroRowParams cannot catch it, because it selects from a table whose columns carry declared types and so passes with either probe. This is the shape the ADBC validation suite's StatementTest.SqlBindZeroRows exercises (it binds int64 parameters to "SELECT ?" and compares the zero-row result schema against a one-row execution), so a NULL probe leaves that test failing on the schema comparison even with the client fix in place. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SLERSYbuLBv36tzs7u2Mim
fornwall
force-pushed
the
fix-flightsql-zero-row-bind
branch
from
July 15, 2026 12:31
42e5085 to
c27503f
Compare
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.
Rationale for this change
PreparedStatement.hasBindParameters()ignored aparamBindingwhose record batch had zero rows, soExecute/ExecuteUpdate/ExecutePollskipped theDoPutentirely. The server therefore never saw the new binding and would execute the query with whatever parameters were previously bound, and it also never received the parameter schema needed to infer the result-set schema for zero-row executions.This was surfaced by the ADBC validation suite's zero-row bind test (see fornwall/arrow-adbc#24), which had to add a
supports_bind_zero_rows()quirk to work around this client behavior.What changes are included in this PR?
Client:
hasBindParameters()no longer gates onNumRows() > 0— a zero-row binding is meaningful (it replaces any prior binding and carries the parameter schema), so it is now sent. The IPC writer already handles both zero-row paths correctly (a zero-row record batch is written normally, and an emptyRecordReaderstill emits the schema message onClose).Example server: with the client fix in place, the SQLite example server received the zero-row binding but treated
len(params) == 0as "execute once with no arguments", failing withmissing argument with index 1for parameterized queries.DoGetPreparedStatementnow distinguishes a zero-row binding (non-nil empty params) from no binding at all: it executes the query zero times, yielding zero rows, while still reporting the real result schema. The schema is probed by executing the statement once with a placeholder per parameter and discarding any rows the probe produces.Why the schema probe binds typed zero values rather than NULL
This is the subtle part of the change, so it is worth stating explicitly.
SQLite has no static type for the result column of a parameter-only expression such as
SELECT ?— the column type comes from the value bound at execution time. Bind anint64andColumnType.ScanType()reportsint64, sogetArrowTypeyieldsint64. Bind a NULL and the column is left untyped (DatabaseTypeName() == ""andScanType() == nil), sogetArrowTypefalls back to the dense union+ud:0,1,2.A NULL probe would therefore advertise one schema for a zero-row binding and a different one for a non-empty execution of the same query — precisely the invariant this PR exists to preserve. The probe instead binds a typed, non-NULL zero value per parameter, derived from the bound parameter schema (which is retained even when the binding carries no rows). Parameters with no obvious zero value — a dense-union parameter, say — are still bound as NULL; their result columns are typed by SQLite's declared types where those exist.
Note that this means the probe evaluates the query once with a fabricated value. That is confined to the read path (
DoGetPreparedStatement), and every row it produces is discarded; only the schema is kept. It is also unavoidable in the sense that a typed NULL would not help — modernc reports the scan type from the row's actual value, so the probe value has to be non-NULL for SQLite to report a concrete type at all.Are these changes tested?
Yes:
TestPreparedStatementExecuteParamBindingZeroRows/TestPreparedStatementExecuteReaderBindingZeroRows: client-level tests that theDoPuthappens (schema + zero-row batch for a record binding, schema-only for an empty reader binding) and the updated prepared-statement handle is captured.TestCommandPreparedStatementQueryZeroRowParams: end-to-end client+example-server test binding one row, then zero rows, asserting the second execution yields zero rows with the real result schema instead of stale results.TestCommandPreparedStatementQueryZeroRowParamsUntypedColumn: end-to-end coverage for the untyped-column case above — bindsSELECT ?with oneint64row and then with zero rows, asserting both report anint64result column. This is the only test that pins the typed probe:TestCommandPreparedStatementQueryZeroRowParamsselects from a table whose columns carry declared types, so it passes with either a NULL or a typed probe. Reverting the probe to NULL fails the new test (int64vs+ud:0,1,2) while leaving the older one green.All of these fail without the corresponding fix.
go test ./arrow/flight/flightsql/...is green.Relationship to the ADBC zero-row bind test
The ADBC validation suite's
StatementTest.SqlBindZeroRowsbindsint64parameters toSELECT ?and compares the zero-row result schema against a one-row execution, so it exercises exactly the untyped-column shape described above. Both halves of this PR are required for it: without the client fix theDoPutnever happens (missing argument with index 1against v18.6.0), and without the typed probe the rows half passes but the schema comparison still fails with+ud:0,1,2againstl.Verified by replicating that flow against this branch's example server through the real Flight SQL client (prepare
SELECT ?, bind oneint64row, then zero rows): schemas match atint64and the zero-row execution yields no rows. Flipping ADBC'ssupports_bind_zero_rows()quirk is a follow-up in that repo — it additionally needs a released arrow-go, sinceci/docker/golang-flightsql-sqlite.dockerfilebuilds its test server viago install .../sqlite_flightsql_server@latestandgo/adbc/go.modpinsarrow-goat v18.6.0.Are there any user-facing changes?
Yes: binding a zero-row record batch (or an empty record reader) to a prepared statement now sends the binding to the server instead of silently skipping it, and the example server handles such bindings by returning an empty result set with the correct schema.
🤖 Generated with Claude Code
https://claude.ai/code/session_01SLERSYbuLBv36tzs7u2Mim