fix(rust/ffi): set rows_affected to -1 on the query path of ExecuteQuery#4469
Merged
Merged
Conversation
fornwall
added a commit
to fornwall/adbc-spanner
that referenced
this pull request
Jul 6, 2026
* Pin adbc_core/adbc_ffi to a fork with two fixes; bump arrow to 59
Switches the ADBC crates from crates.io 0.23 to a git pin on
fornwall/arrow-adbc carrying two fixes absent from the 0.23 release:
1. release_ffi_error is idempotent — it nulls the AdbcError's message,
private_data and release after freeing, so a second release (the
standard `if (error.release) error.release(&error)` idiom) is a safe
no-op rather than a double-free. Already on arrow-adbc main; only
missing from 0.23.
2. AdbcStatementExecuteQuery writes rows_affected = -1 on the query path
instead of leaving the caller's out-parameter untouched. Submitted
upstream as apache/arrow-adbc#4469.
Because the git pin's adbc crates allow arrow >=58,<60, bump arrow to
59.x (was pinned <59 for the 0.23 crates).
Effect on the C++ adbc_validation suite (run against the driver's C ABI):
the fixes remove the process-aborting double-free on error-path tests and
make queries report rows_affected = -1, so four more StatementTest cases
pass cleanly (SqlQueryInts, SqlQueryStrings, SqlPrepareSelectNoParams,
SqlPrepareErrorNoQuery). The gate grows from 31 to 35 passing tests; the
README's follow-up categorization and the now-obsolete "failures abort
the process" note are updated accordingly.
Also carries the one salvageable improvement from the superseded #10:
prepare() now accepts a bulk-ingest target (target set, no SQL) instead
of wrongly returning InvalidState, with a test.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013X531Vx36F5vyUSVidayPd
* Allow the arrow-adbc git source in deny.toml; refresh lockfile
cargo-deny failed with source-not-allowed for the three adbc_* crates now
pinned to git — add https://github.com/fornwall/arrow-adbc to the
[sources] allow-git list (alongside the google-cloud fork).
Also refresh Cargo.lock to the latest compatible versions (`cargo
update`), which moves num-bigint off the yanked 0.4.7 -> 0.4.8 and bumps
a handful of transitive patch releases. All direct dependencies are
already on their latest major (arrow is capped at <60 by the pinned adbc
crates), so there are no Cargo.toml major bumps to make.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013X531Vx36F5vyUSVidayPd
---------
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
lidavidm
approved these changes
Jul 7, 2026
lidavidm
left a comment
lidavidm
left a comment
Member
There was a problem hiding this comment.
Thanks! Just a nit about the redundant comment
`statement_execute_query` wrote `rows_affected` only on the update path (`out == NULL`). On the query path it left the caller's out-parameter untouched, so callers read back whatever they passed in. ADBC's `AdbcStatementExecuteQuery` contract (adbc.h) defines `rows_affected` as the row count if known, else -1; write that sentinel in the query branch. Adds a C-ABI regression test (via the dummy driver's exported vtable, since the Rust driver manager passes a NULL `rows_affected` for queries so the value is not observable through the high-level API). Signed-off-by: Fredrik Fornwall <fredrik@fornwall.net>
fornwall
force-pushed
the
fix-execute-query-rows-affected
branch
from
21b110f to
289ed79
Compare
Contributor
Author
|
Thanks! I applied your suggestions which removed the comment and squashed the commits. |
Contributor
Author
|
CI failures seems unrelated to changes here? https://github.com/apache/arrow-adbc/actions/runs/28850435930/job/85565046218?pr=4469
|
Contributor
Author
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
2 participants
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.
statement_execute_querywroterows_affectedonly on the update path (out == NULL). On the query path it left the caller's out-parameter untouched, so callers read back whatever they passed in. ADBC'sAdbcStatementExecuteQuerycontract (adbc.h) definesrows_affectedas the row count if known, else -1; write that sentinel in the query branch.Adds a C-ABI regression test (via the dummy driver's exported vtable, since the Rust driver manager passes a NULL
rows_affectedfor queries so the value is not observable through the high-level API).Closes #4468.