feat(validation): rewrite the suite's dialect-sensitive SQL via RewriteSql — 30 more cases gate-enforced - #283
Merged
Merged
Conversation
…teSql — 30 more cases gate-enforced Point ARROW_ADBC_TAG at the apache/arrow-adbc main revision 1ba7fe66, which carries apache/arrow-adbc#4514: it routes the statement tests' hardcoded SQL — non-Spanner CREATE TABLEs, INSERTs without a column list, `SELECT *` ingest readbacks, NULLS FIRST/LAST — through the DriverQuirks::RewriteSql hook (apache/arrow-adbc#4496) under stable query ids. This started out pinned to a fork while #4514 was in review; now that it is merged, the pin returns to apache/arrow-adbc. SpannerQuirks::RewriteSql grows from the single FLOAT64 rewrite into a table of GoogleSQL substitutions, each pinning the upstream default so a future tag bump that changes a query fails loudly. Alongside it: IngestSelectRoundTripType declares Spanner's readback widenings (small ints -> INT64, large/view strings -> STRING, binary variants -> BYTES), ValidateIngestedTemporalData checks the timestamp readback values (any-unit Arrow timestamps -> TIMESTAMP -> Timestamp(Nanosecond, "UTC")), and PrimaryKeyIngestTableDdl returns nullopt (Spanner has no ordered auto-increment, so the auto-assigned-key case self-skips). EXCLUDED shrinks from 42 to 11 cases: 30 newly passing and gate-enforced (the whole ingest type family incl. list/dictionary/view layouts, Append/Replace/CreateAppend/MultipleConnections/Sample, SqlBind, SqlQueryEmpty, SqlQueryInsertRollback, SqlQueryRowsAffectedDelete{,Stream}, SqlPrepareSelectParams) plus SqlIngestPrimaryKey now self-skipping. The survivors: SqlPrepareUpdate{,Stream} (assert insertion order, unrecoverable from Spanner's PK-ordered tables), the unmapped Arrow types (UInt8/16/32/64, Duration, Interval, FixedSizeBinary), empty-stream ingest, and the SqlQueryCancel ECANCELED errno. Suite totals: 82 pass + 7 self-skip + 11 excluded = 100 cases. 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_01TXzBG1e8nCAUsTarm8sruY
fornwall
force-pushed
the
validation-rewrite-sql
branch
from
July 16, 2026 00:36
93afaad to
192cbdf
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.
What
Now that the
DriverQuirks::RewriteSql(query_id, default_sql)quirk has landed upstream (apache/arrow-adbc#4496), this uses it to enable every validation case whose only blocker was hardcoded non-GoogleSQL SQL.Empirical triage first (full suite run against the emulator at the old pin): of the 42
EXCLUDEDcases, the dominant failures were (1) the emulator rejectingNULLS FIRST/NULLS LAST, (2)SELECT *ingest readbacks surfacing the syntheticadbc_ingest_keycolumn, (3) non-SpannerCREATE TABLEDDL (INT/INTEGER/TEXT, noPRIMARY KEY), (4)INSERTwithout the column list GoogleSQL requires, and (5)SELECT @p0, @p1defeating Spanner's parameter-type inference. All five are pure SQL problems.Upstream side — apache/arrow-adbc#4514 (merged)
Upstream previously routed only the two
CAST(1.5 AS FLOAT)queries throughRewriteSql. apache/arrow-adbc#4514 routes the remaining hardcoded statement-test SQL through the hook under stable query ids; theTestSqlIngestTypeid carries the ingested Arrow type as a suffix so quirks can rewrite per type. Default behavior is unchanged for drivers that don't override the hook.This PR was originally developed against a fork while #4514 was in review. #4514 is now merged, so
ARROW_ADBC_TAGpoints back at anapache/arrow-adbcmainrevision andGIT_REPOSITORYis upstream again — no fork anywhere in the build.Driver-repo side (this PR)
ARROW_ADBC_TAG→ apache/arrow-adbcmain1ba7fe66(contains #4514);GIT_REPOSITORY→apache/arrow-adbc.SpannerQuirks::RewriteSqlgrows into a table of GoogleSQL substitutions (each pinning the upstream default so a tag bump that changes a query fails loudly): Spanner-valid DDL, column-listed INSERTs, readbacks that select the ingested columns explicitly and dropNULLS FIRST/LAST(GoogleSQL's ASC/DESC defaults match exactly),ORDER BY col[SAFE_OFFSET(0)]for the list cases (GoogleSQL can'tORDER BYan ARRAY), and CASTs for the bare-parameter select.IngestSelectRoundTripTypedeclares Spanner's readback widenings (small ints →INT64, large/view strings →STRING, binary variants →BYTES).ValidateIngestedTemporalData(fixture override; the base class FAILs) checks timestamp readbacks: any-unit Arrow timestamps → SpannerTIMESTAMP→Timestamp(Nanosecond, "UTC").PrimaryKeyIngestTableDdl→nullopt:SqlIngestPrimaryKeyappend-ingests rows omitting the PK and expects auto-assigned ascending keys — Spanner has no ordered auto-increment (and its SEQUENCEs are bit-reversed, so even a DEFAULT key can't satisfy the insertion-order readback). The quirk-sanctioned self-skip states that truth.Result
EXCLUDEDshrinks 42 → 11; the suite is now 82 pass + 7 self-skip + 11 excluded = 100 cases, all three checks green locally (gate, expected-failure guard, stale guard).Newly gate-enforced (30): the whole ingest type family — bool/ints/floats/string/binary/date32/timestamps including large/view layouts, dictionary-encoded strings and both
Listcases — plusSqlIngestAppend/Replace/CreateAppend/MultipleConnections/Sample,SqlBind,SqlQueryEmpty,SqlQueryInsertRollback,SqlQueryRowsAffectedDelete{,Stream}, andSqlPrepareSelectParams.Still excluded, none fixable by SQL rewriting:
SqlPrepareUpdate{,Stream}SqlIngestUInt8/16/32/64,Duration,Interval,FixedSizeBinaryTestSqlIngestStreamZeroArraysSqlQueryCancelTesting
main(5537486, v0.7.0) — clean, no conflicts.RewriteSqlcall sites — ids and pinned default SQL — are identical between the old fork rev and upstream1ba7fe66; the only file diff is clang-format reflow. Upstream's suite differs from the fork's base by #4514 alone (the solec/validationchange in the 11 commits since), and all 81StatementTestmethods are unchanged, so the EXCLUDED list and the stale/expected-failure guards stay valid.shellcheck scripts/run-adbc-validation.shclean.🤖 Generated with Claude Code
https://claude.ai/code/session_01TXzBG1e8nCAUsTarm8sruY