chore(c/validation): use explicit column lists in the suite's INSERT statements - #30
Open
fornwall wants to merge 3 commits into
Open
chore(c/validation): use explicit column lists in the suite's INSERT statements#30fornwall wants to merge 3 commits into
fornwall wants to merge 3 commits into
Conversation
…quirk Several statement tests hardcode SQL that is not portable across SQL dialects: CREATE TABLE statements with INT/INTEGER/TEXT columns and no primary key, INSERT statements without a column list, SELECT * readbacks after create-mode ingest (which break on drivers that add a synthetic key column), ORDER BY ... NULLS FIRST/LAST, and bare parameter SELECTs whose types some engines cannot infer. Route each of these through the DriverQuirks::RewriteSql hook (added in apache#4496) with a stable query id, so driver test harnesses can substitute an equivalent query in their own dialect. The TestSqlIngestType query id carries the ingested Arrow type (ArrowTypeString) as a suffix, so quirks can rewrite per type — e.g. a dialect that cannot ORDER BY a list-typed column. No behavior changes for drivers that do not override RewriteSql: the default implementation returns the given SQL unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011hsYQVXuHhnuiBD7XTVo65
Pure line-rewrapping of the RewriteSql call sites to the pinned clang-format (v18.1.7 via pre-commit); no functional change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011hsYQVXuHhnuiBD7XTVo65
…statements
Three statement tests issue INSERT INTO <table> VALUES (...) without an
explicit column list. The implicit form is ISO/IEC 9075-standard (an
omitted column list means all columns in ordinal position), but the
explicit list is equally standard and strictly more portable:
- These tests exercise prepared-statement parameter binding, not
INSERT column-list-omission semantics, so naming the columns loses
no intended coverage.
- Real engines diverge: GoogleSQL (Spanner) rejects INSERT without a
column list outright, and the suite exists for cross-engine
conformance.
- For TestSqlPrepareUpdate{,Stream} the target table is created by
create-mode bulk ingest, and ADBC does not guarantee the created
table has exactly the ingested columns - a driver may legitimately
add a synthetic key column (e.g. Spanner mandates a primary key), in
which case the implicit-column-list INSERT has the wrong arity even
where the form is supported. The implicit form bakes in an assumption
the ADBC spec does not make.
The RewriteSql routing and query ids are unchanged; only the default
SQL strings gain column lists (quoted via QuoteIdentifier where the
surrounding statement already quotes the table).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011hsYQVXuHhnuiBD7XTVo65
fornwall
force-pushed
the
validation-insert-column-lists
branch
from
July 14, 2026 11:46
d334e9a to
8dd2b42
Compare
fornwall
force-pushed
the
validation-rewrite-sql-sites
branch
3 times, most recently
from
July 14, 2026 12:08
62d8b0c to
6590897
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
1 participant
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.
Verdict on the standards question
INSERT INTO t VALUES (...)with an omitted column list is ISO/IEC 9075-standard: the standard defines the omitted list as an implicit column list of all columns of the table in ordinal position. So the current form is not wrong per the standard — but the explicit column list is equally standard, and here it is strictly better:TestSqlPrepareUpdate{,Stream}the target table is created by create-mode bulk ingest, and ADBC does not guarantee the created table has exactly the ingested columns. A driver may legitimately add a synthetic key column (the Spanner driver addsadbc_ingest_key, since Spanner mandates a primary key), which makes the implicit-column-list INSERT have the wrong arity even on engines where the form is supported. The implicit form bakes in an assumption the ADBC spec does not make.The fix
Only the three default SQL strings change; the
RewriteSqlrouting and query ids are untouched, so drivers can still override:TestSqlBind:INSERT INTO bindtest (col1, col2) VALUES (@p0, @p1)(bare identifiers, matching the rest of that test's statements)TestSqlPrepareUpdate:INSERT INTO <q>bulk_ingest<q> (<q>int64s<q>) VALUES (@p0)(viaquirks()->QuoteIdentifier, matching the already-quoted table)TestSqlPrepareUpdateStream:INSERT INTO <q>bulk_ingest<q> (<q>ints<q>) VALUES (@p0)Verification (spanner-adbc harness, emulator)
Pointed spanner-adbc's
adbc-validationat this commit and deleted the three insert-rewritekRewritesentries (the defaults are now valid GoogleSQL, which is the point).scripts/run-adbc-validation.shexited 0:SpannerStatementTest.SqlBindpassing on the unrewritten default INSERTSqlPrepareUpdate/SqlPrepareUpdateStream(still excluded for an unrelated reason) now fail only at the final readback row-order assertion (CompareArrayatadbc_validation_statement.cc:2078/:2177), not at the INSERT — i.e. the unrewritten INSERT executes fine.Notes
Stacks on #28 (
validation-rewrite-sql-sites, the RewriteSql routing) — the base of this PR is that branch so the diff shows only this fix. Like #28, this is an upstreaming candidate for apache/arrow-adbc.🤖 Generated with Claude Code
https://claude.ai/code/session_011hsYQVXuHhnuiBD7XTVo65