Skip to content

chore(c/validation): make TestSqlPrepareUpdate{,Stream} readback order deterministic - #29

Open
fornwall wants to merge 3 commits into
validation-rewrite-sql-sitesfrom
validation-prepare-update-ordering
Open

chore(c/validation): make TestSqlPrepareUpdate{,Stream} readback order deterministic#29
fornwall wants to merge 3 commits into
validation-rewrite-sql-sitesfrom
validation-prepare-update-ordering

Conversation

@fornwall

Copy link
Copy Markdown
Owner

Defect

StatementTest::TestSqlPrepareUpdate and StatementTest::TestSqlPrepareUpdateStream read back the bulk_ingest table with a plain SELECT * FROM bulk_ingest — no ORDER BY — and then assert the returned rows positionally via CompareArray ({42, -42, NULL, 42, -42, NULL} and {1, 2, NULL, 3, NULL, 3} respectively), i.e. they assert insertion order.

SQL gives no ordering guarantee without ORDER BY, so a positional assertion on an unordered SELECT relies on engine-specific behavior. SQLite and PostgreSQL happen to return insertion order, which is why these pass upstream; engines that return rows in primary-key order fail the assertion (for Spanner, whose create-mode bulk ingest keys the table on a synthetic adbc_ingest_key STRING(36) DEFAULT (GENERATE_UUID()) column, the readback order is effectively random per run). The suite itself already treats this as a solved problem elsewhere: TestSqlIngestType reads back with ORDER BY "col" ASC NULLS FIRST and TestSqlBind with ORDER BY col1 ASC NULLS FIRST.

Fix

Give both readbacks the suite's existing deterministic-readback convention — ORDER BY <quirks()->QuoteIdentifier(col)> ASC NULLS FIRST — and sort the expected vectors accordingly:

  • TestSqlPrepareUpdate (int64s): {NULL, NULL, -42, -42, 42, 42}
  • TestSqlPrepareUpdateStream (ints): {NULL, NULL, 1, 2, 3, 3}

The RewriteSql routing and query ids from #28 are unchanged — only the default SQL string passed to RewriteSql gains the ORDER BY clause, so drivers whose dialect rejects NULLS FIRST (GoogleSQL) can still substitute an equivalent ordering via the quirk.

Verification

Verified end-to-end against the Spanner ADBC driver's validation harness (scripts/run-adbc-validation.sh in fornwall/spanner-adbc, pinned to this commit, with the two cases removed from its expected-failure list and its RewriteSql quirk substituting ORDER BY ... ASC for the NULLS FIRST form): both SpannerStatementTest.SqlPrepareUpdate and SpannerStatementTest.SqlPrepareUpdateStream pass — 84 passed / 7 skipped of 91, expected-failure gate green.

Stacks on #28 (validation-rewrite-sql-sites), which this PR is based on 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

…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
@fornwall
fornwall force-pushed the validation-prepare-update-ordering branch from 5bcd9c1 to 15f41c9 Compare July 14, 2026 11:21
fornwall and others added 2 commits July 14, 2026 13:41
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
…r deterministic

TestSqlPrepareUpdate and TestSqlPrepareUpdateStream read back
bulk_ingest with a plain "SELECT * FROM bulk_ingest" and then assert
the returned rows positionally via CompareArray — i.e. they assert
insertion order. SQL gives no ordering guarantee without ORDER BY, so
this relies on engine-specific behavior: SQLite and PostgreSQL happen
to return insertion order, but engines that return rows in primary-key
order (e.g. Spanner, whose create-mode ingest keys the table on a
synthetic UUID column) fail the assertion nondeterministically.

Fix it with the suite's existing convention — the deterministic
ORDER BY <col> ASC NULLS FIRST readback that TestSqlIngestType and
TestSqlBind already use — and sort the expected vectors accordingly.
The RewriteSql routing and query ids are unchanged; only the default
SQL string gains the ORDER BY clause.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011hsYQVXuHhnuiBD7XTVo65
@fornwall
fornwall force-pushed the validation-prepare-update-ordering branch from 15f41c9 to f956e97 Compare July 14, 2026 11:43
@fornwall
fornwall force-pushed the validation-rewrite-sql-sites branch 3 times, most recently from 62d8b0c to 6590897 Compare July 14, 2026 12:08
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