fix: repair filter providers and adapter regressions#476
Open
cofin wants to merge 10 commits into
Open
Conversation
…d get_columns coverage The postgres indexes query used array_agg(a.attname ...) which returns PostgreSQL's internal name[] array type. asyncpg and psycopg decode it transparently; psqlpy raises "Cannot convert _name into Python type." Casting attname::text yields text[] which all three adapters decode to list[str]. Adds first integration coverage for data_dictionary.get_columns(table=...) and get_columns(schema=...) across asyncpg, psycopg, and psqlpy. The original #361 symptom (server-side syntax error on the introspection query) does not reproduce on master, but no integration test exercised this query path until now. Closes #361.
Two distinct bugs made AdbcConfig(connection_config={"driver_name":
"sqlite", ...}) unusable:
1. detect_dialect() consulted only connection.adbc_get_info(); when the
ADBC SQLite driver's GetInfo did not match a known pattern, the
function warned "Could not determine dialect from driver info.
Defaulting to 'postgres'." and returned "postgres". The user-supplied
driver_name was ignored.
2. AdbcDriver.begin() ran cursor.execute("BEGIN") unconditionally. ADBC
SQLite holds an implicit transaction and rejected the explicit BEGIN
with "cannot start a transaction within a transaction."
detect_dialect now accepts an optional fallback_dialect; AdbcSessionContext
threads the config-resolved dialect through to AdbcDriver, which passes
it down. begin() is a no-op on the sqlite dialect; commit()/rollback()
still close the implicit transaction.
Closes #472.
…_read_session()
SpannerSyncConfig.provide_session() defaulted transaction=False, which
yields a google.cloud.spanner_v1.snapshot.Snapshot context. Any DML
through that session raised SQLConversionError("Cannot execute DML in
a read-only Snapshot context.") with no pointer at the existing
provide_write_session() workaround.
Flips the default to transaction=True so provide_session() matches
every other sqlspec adapter (write-capable). Adds provide_read_session()
for the explicit snapshot path; keeps provide_write_session() as an
alias for callers already on it.
Pulls the default-transaction flag and the snapshot error message into
module-level constants (_DEFAULT_SESSION_TRANSACTION,
_READ_ONLY_SNAPSHOT_ERROR_MESSAGE) so an eventual SpannerAsyncConfig can
mirror them line-for-line when google-cloud-spanner ships high-level
async (currently only the gapic SpannerAsyncClient exists upstream).
The snapshot raise sites now point readers at provide_read_session by
name.
Closes #474.
…_ms knob Two emulator-interop issues: 1. sqlspec/adapters/bigquery/events/store.py:78 returned " CLUSTER BY channel, status, available_at" unconditionally. The driver already detected the emulator via _using_emulator (driver.py:142), but the events store builds DDL from BigQueryConfig and has no driver handle. Extracts is_emulator_active_from_env() alongside detect_emulator() so the events store can branch on the env-var path without a connection. _table_clause() returns "" when the emulator is active. 2. BigQueryConnectionParams declared job_timeout_ms: NotRequired[int] (config.py:51) but nothing read it. The parallel query_timeout_ms field was the only live knob (config.py:262-264 → QueryJobConfig. job_timeout_ms). Wires job_timeout_ms through the same setup so the TypedDict surface is honest; when both are set, job_timeout_ms wins (applied last). Either knob now lets a caller surface emulator unresponsiveness as a timeout error instead of an indefinite block. Closes #473.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #476 +/- ##
==========================================
+ Coverage 70.78% 70.90% +0.12%
==========================================
Files 438 438
Lines 52541 52536 -5
Branches 7370 7370
==========================================
+ Hits 37189 37253 +64
+ Misses 12682 12619 -63
+ Partials 2670 2664 -6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
The previous commit branched _table_clause() on BIGQUERY_EMULATOR_HOST to omit CLUSTER BY when running against goccy/bigquery-emulator. That downgraded production BigQuery DDL to work around an unofficial emulator's gap. Production BigQuery supports CLUSTER BY on the events queue table and benefits from clustering on (channel, status, available_at) for polling queries. The emulator's lack of support is a downstream limitation; emulator-only tests should skip or xfail rather than reshape product behavior. Reverts the events store DDL branch and the is_emulator_active_from_env helper extraction. detect_emulator() goes back to the inline env-var check. The job_timeout_ms wiring on config.py remains — it is unrelated to the CLUSTER BY question and addresses the dead TypedDict field that was declared but never read.
detect_emulator()'s only production consumer was build_retry(), which returned None (no retry policy) when running against the BigQuery emulator. That is an emulator-specific product-behavior fork on the same footing as the just-reverted CLUSTER BY skip. Continues the cleanup direction set by 6de55be ("remove emulator check for simple inserts in BigQuery driver") and c9695de (the bulk load skip it eventually removed). build_retry() now always returns the production Retry policy. BigQueryDriver loses the _using_emulator slot. Retries that misbehave against the emulator should surface as test failures and those tests should pytest.skip / pytest.xfail at the call site, same pattern as test_explain.py and test_exceptions.py already use.
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.
Summary
Correct various bugs and errors:
edc061c3array_agg(a.attname)::text[]compatibility3735b782ef5fe3dcmake lint,make type-check3a07d135provide_session()write-by-default plusprovide_read_session()6acdddb7CLUSTER BYskip plusjob_timeout_mswiringdb1caab5Details
text[].CLUSTER BYwhile wiring the declaredjob_timeout_msparameter through to live query jobs.>=0.19.0, registers RustFS through the upstream pytest plugin, and removes now-redundant local RustFS credential and pgvector service overrides.Validation
make lintmake type-checkFull suite was not run locally.
Closes #361
Closes #472
Closes #473
Closes #474
Closes #475