Skip to content

test(c/validation): add coverage for option, GetInfo and bind gaps found reviewing adbc-spanner - #21

Closed
fornwall wants to merge 7 commits into
mainfrom
validation/review-gap-tests
Closed

test(c/validation): add coverage for option, GetInfo and bind gaps found reviewing adbc-spanner#21
fornwall wants to merge 7 commits into
mainfrom
validation/review-gap-tests

Conversation

@fornwall

Copy link
Copy Markdown
Owner

What this adds

Five new generic, driver-agnostic test cases for the C++ validation suite (c/validation), each staging coverage for a real bug found during a full-project review of the out-of-tree adbc-spanner Rust driver (its REVIEW.md). None of these behaviors were covered by the suite before. All tests are written against the generic DriverQuirks fixtures, so they auto-enroll for every driver using ADBCV_TEST_CONNECTION / ADBCV_TEST_STATEMENT; they are candidates for later upstream submission to apache/arrow-adbc.

Two of the new tests exposed the same bugs in the in-tree SQLite driver, so this PR also carries two small SQLite fixes (first two commits, one per fix) to keep the suite green:

  • fix(c/driver/sqlite): report the real result schema for a zero-row bound parameter stream — the reader skipped schema inference entirely when the bound parameter stream had zero rows and returned a zero-column schema.
  • fix(c/driver/sqlite): accept adbc.statement.exec.incremental at its spec default — the option framework rejected even ADBC_OPTION_VALUE_DISABLED (the documented default) with NOT_IMPLEMENTED.

No new quirks were needed; every test passes or self-skips on SQLite via existing quirk guards (supports_get_option(), supports_get_sql_info(), supports_dynamic_parameter_binding()). Commits are one-per-test-area so they can be cherry-picked upstream independently.

Per-test summary

Test adbc-spanner finding Spec reference Expected on current adbc-spanner How determined
ConnectionTest.AutocommitIntRoundTrip COR-4 (set-as-int succeeds, get-as-int fails) adbc.h doc for AdbcConnectionGetOptionInt (c/include/arrow-adbc/adbc.h:1745-1752): "For standard options, drivers must always support getting the option value (if they support getting option values at all) via the type specified in the option. (For example, an option set via SetOptionDouble must be retrievable via GetOptionDouble.)" The test only asserts the typed get when the driver accepted the typed set; drivers that reject SetOptionInt (like SQLite) skip. FAILSetOptionInt(autocommit, 1) returns OK, then GetOptionInt fails INVALID_ARGUMENT: option adbc.connection.autocommit value "true" is not an integer verified by actual run against the driver on the Spanner emulator
ConnectionTest.MetadataGetInfoAllCodes SPEC-5 (get_info(None) curated subset vs explicit requests answering more codes) adbc.h doc for AdbcConnectionGetInfo (adbc.h:1550-1552): "info_codes A list of metadata codes to fetch, or NULL to fetch all"; and adbc.h:1539-1541: unrecognized requested codes are omitted from the result. A code returned for an explicit request is therefore recognized by the driver, so it must also appear in the NULL/all-codes result. FAIL — explicit requests for codes 1 (VENDOR_VERSION) and 2 (VENDOR_ARROW_VERSION) return rows, but the all-codes result is {0, 3, 4, 100, 101, 102, 103} verified by actual run
StatementTest.SqlBindZeroRows COR-9 (zero-row bound batch advertises an empty schema) No literal spec text mandates this; it is a consistency invariant (stated as such rather than inventing a citation): the result-set schema of a query must not depend on how many parameter rows were bound — a DBAPI executemany with an empty parameter list otherwise sees a schema disagreeing with every non-empty execution of the same query. FAIL — zero-row bind returns a 0-column schema (real result schema has 1 column) verified by actual run
StatementTest.SqlBindNullType CONV-1 (Null-typed bind columns rejected) adbc.h doc for AdbcStatementGetParameterSchema (adbc.h:2284-2286): "If the type cannot be determined, the type of the corresponding field will be NA (NullType)" — a client that builds its bind batch from exactly the schema the driver advertises (or pyarrow inferring null for an all-None parameter list) produces NA columns, which the driver must accept. FAILINVALID_ARGUMENT: cannot bind parameter "p0": unsupported Arrow type Null verified by actual run
StatementTest.SqlQueryIncrementalDefault SPEC-2 (adbc.statement.exec.incremental rejected even at its spec default) adbc.h doc for ADBC_STATEMENT_OPTION_INCREMENTAL (adbc.h:650-665): "The default is ADBC_OPTION_VALUE_DISABLED." The header documents the default rather than literally mandating a set-to-default no-op; the test encodes the practical consequence that a generic client writing back defaults must not break. Actually enabling may still return NOT_IMPLEMENTED (asserted). FAILNOT_IMPLEMENTED: statement option adbc.statement.exec.incremental is not supported by the Spanner ADBC driver for value "false" verified by actual run

All five adbc-spanner outcomes were verified by an end-to-end run (not derived from source): the harness at adbc-spanner/adbc-validation was built with FETCHCONTENT_SOURCE_DIR_ARROW_ADBC pointed at this branch and run against libadbc_spanner.so (debug build of adbc-spanner main @ c4e1ac9) on the Spanner emulator. All 5 new tests FAILED there with exactly the failure modes predicted by the review findings (messages quoted above).

SQLite verification

cmake -S c -B <build> -DADBC_DRIVER_SQLITE=ON -DADBC_BUILD_TESTS=ON -DCMAKE_BUILD_TYPE=Debug
cmake --build <build> --target adbc-driver-sqlite-test -j8
<build>/driver/sqlite/adbc-driver-sqlite-test

Full suite at the branch head: 126 passed, 16 skipped, 0 failed (the 15 pre-existing skips plus SqliteConnectionTest.AutocommitIntRoundTrip, which skips because the SQLite driver's option framework rejects integer values for the boolean autocommit option — the intended skip path). A filtered run of the five new tests: 4 pass, 1 skip. Without the two SQLite fix commits, SqlBindZeroRows and SqlQueryIncrementalDefault fail on SQLite with the same bugs found in adbc-spanner.

Touched files were formatted with clang-format 18.1.7 (the version pinned in .pre-commit-config.yaml).

Note: the new tests also auto-enroll in the other in-tree driver test suites (e.g. PostgreSQL, which needs a live server); those were not run here since only the SQLite reference driver was in scope.

🤖 Generated with Claude Code

https://claude.ai/code/session_01LGf8PVEe2tYkw8Q6Pd95tq

fornwall and others added 7 commits July 13, 2026 17:03
…und parameter stream

When a query was executed with a bound parameter stream containing zero
rows, the reader skipped schema inference entirely and reported an empty
(zero-column) schema instead of the statement's actual result schema.
Run InferFinalize even when the binder finishes before the first
execution, so the schema has the correct number of columns (matching the
behavior of an ordinary query returning zero rows).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LGf8PVEe2tYkw8Q6Pd95tq
…pec default

ADBC_STATEMENT_OPTION_INCREMENTAL defaults to disabled (adbc.h), so a
generic client that writes back option defaults must be able to set it
to ADBC_OPTION_VALUE_DISABLED as a no-op. Keep returning NOT_IMPLEMENTED
when a caller actually tries to enable incremental execution.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LGf8PVEe2tYkw8Q6Pd95tq
If a driver accepts AdbcConnectionSetOptionInt for the standard
autocommit option, AdbcConnectionGetOptionInt on the same key must
return the value that was set: per adbc.h, drivers must support getting
an option value via the type it was set with. Drivers that reject the
integer set (like the SQLite driver, whose option framework only parses
"true"/"false" strings for booleans) skip cleanly.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LGf8PVEe2tYkw8Q6Pd95tq
… requested codes

AdbcConnectionGetInfo with NULL info_codes fetches all metadata the
driver recognizes (adbc.h). A driver that returns a row for an
explicitly requested info code therefore recognizes that code, so the
same code must also appear in the all-codes result; otherwise the two
forms disagree about what the driver supports.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LGf8PVEe2tYkw8Q6Pd95tq
…al result 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 whose schema matches the schema of any non-empty
execution of the same query, with zero rows - not an empty (zero
column) schema. This is a consistency invariant rather than literal
spec text; requires the preceding SQLite reader fix to pass there.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LGf8PVEe2tYkw8Q6Pd95tq
…s accepted

AdbcStatementGetParameterSchema reports parameters whose type cannot be
determined as NA (adbc.h), and clients like pyarrow infer a null-typed
column for an all-None parameter list. A driver must therefore accept a
bind batch containing a null-typed column, treating every value as NULL.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LGf8PVEe2tYkw8Q6Pd95tq
…spec default

ADBC_STATEMENT_OPTION_INCREMENTAL has a documented default of
ADBC_OPTION_VALUE_DISABLED (adbc.h), so setting it to that value must
succeed as a no-op even in drivers without incremental execution;
generic clients (which may write back defaults unconditionally) break
otherwise. Enabling it may still return NOT_IMPLEMENTED, and a plain
query must keep working afterwards. Requires the preceding SQLite
option fix to pass there.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LGf8PVEe2tYkw8Q6Pd95tq
@fornwall

Copy link
Copy Markdown
Owner Author

Split into five independent PRs, one per validation test (with each SQLite fix riding along with the test that exposed it):

  1. test(c/validation): assert typed option set/get coherence for autocommit #22ConnectionTest.AutocommitIntRoundTrip (typed option set/get coherence)
  2. test(c/validation): assert GetInfo all-codes result covers explicitly requested codes #23ConnectionTest.MetadataGetInfoAllCodes (GetInfo all-codes covers explicitly requested codes)
  3. test(c/validation): assert a zero-row bound batch still yields a result schema #24StatementTest.SqlBindZeroRows + SQLite zero-row bound stream schema fix
  4. test(c/validation): assert a null-typed (NA) bound parameter column is accepted #25StatementTest.SqlBindNullType (null-typed bound parameter column)
  5. test(c/validation): assert the incremental option is accepted at its spec default #26StatementTest.SqlQueryIncrementalDefault + SQLite incremental-option default fix

Closing in favor of those.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant