Skip to content

[MAINTENANCE] Declare and register the credential-gated SQL backends - #12022

Open
joshua-stauffer wants to merge 10 commits into
f/sql-harness/backend-frameworkfrom
f/sql-harness/credential-gated-backends
Open

[MAINTENANCE] Declare and register the credential-gated SQL backends#12022
joshua-stauffer wants to merge 10 commits into
f/sql-harness/backend-frameworkfrom
f/sql-harness/credential-gated-backends

Conversation

@joshua-stauffer

Copy link
Copy Markdown
Collaborator

Stacked on #12021 — review that one first. This PR's base is the foundation branch, so it shows
only its own commit; GitHub will retarget it to the feature branch when #12021 merges.

Converts BigQuery, Databricks, Redshift, and Snowflake onto the declared record, the same
conversion the foundation PR applied to the four locally runnable backends. Each config now
states its identity once and enrols in the registry; the hand-written label and mark properties
are deleted. Every behavioral override is kept — the credential settings classes, Snowflake's
engine and keyword-argument asset construction, Redshift's typed connection-string return.

The records remain inert. Nothing reads them yet, and the shared setup is untouched, so this
changes no behavior.

Details worth a reviewer's attention

Databricks declares autocommit and its insert parameter limit. Both facts currently live in
the shared setup as a dialect-keyed set and an inlined constant. This PR only declares them —
the shared setup still derives them from the live dialect, and the deletions happen in the PRs
that make it read the declaration instead.

Redshift's lane token differs from its pytest marker, deliberately. Its marker is redshift,
but the workflow matrix selects gx-redshift, which the task runner translates back to
redshift. Recording them as one value would be wrong in one place or the other, so the record
carries both.

Redshift declares no tier, because it is absent from today's data-source list. The other
three declare STANDARD_SQL, matching that list exactly, so the lists derived from tier
membership later select the same backends they do now.

Three modules move pytest and BatchTestSetup under TYPE_CHECKING. Deleting the
pytest_mark property left them used only in annotations, which ruff flags. This is the class
of change that passes lint, type-check, and collection and then fails at runtime, so it was
verified by calling create_batch_setup on each module rather than by importing: no NameError,
with the credential-gated backends failing at credential validation, i.e. execution reached the
constructor body.

Verification

These four are credential-gated and cannot run here, so the evidence is identity preservation:
label, pytest_mark, test_id, and hash() were compared against the pre-change classes
loaded side by side in one interpreter, and are bit-identical for all four.

Collection unchanged at 3332 with no import errors; the SQLite suite re-run as a collateral
check (539 passed) and the registry and marker suites pass. ruff clean; mypy adds no
diagnostics over an untouched-file baseline. No module dereferences its dialect package at
import, so the harness still imports where no dialect is installed.

Note that the BigQuery lane carries an unconditional skip for the duration of a CI
infrastructure transition, so a green lane there is not positive evidence for this change.

Introduces SqlBackendSpec plus its supporting enums and CI lane reference:
a frozen record holding every dialect fact that is data rather than
behavior. The module imports nothing from the harness and no runtime
SQLAlchemy symbol, so it sits to the left of its consumers in the
dependency graph and imports cleanly in lanes with no dialect installed.
Registration enrols a config class and validates its declaration at
decoration time: unique label and marker, container provisioning
consistency, non-empty identity fields, a positive insert parameter
limit, a recorded reason for every per-case tier exclusion, and a
ceiling on how many a single backend may declare. A reason makes one
exclusion answerable; only a count makes the set of them answerable.

The table-schema-item factory is checked for callability but never
called, since calling it would require the backend's dialect package,
which registration must not assume is installed.

The registry is process-global, so tests reach it through a seam that
clears on entry and restores on exit. Clearing rather than only
restoring is what lets a test assert whole-registry equality exactly,
without the assertion depending on how many real backends exist.
SqlDatasourceTestConfig carries the backend's declaration as a class
variable and derives the test label and pytest mark from it, so a
backend states its identity once instead of hand-writing two properties
that must agree with each other.

An optional per-instance override backs the ad-hoc escape hatch, whose
identity varies per connection string rather than per class; one
property resolves the two sources so every consumer reads identity
through a single name.

The decorator passes eq=False so the base class's hand-written
__eq__/__hash__ survive. They reduce extra_column_types to a hashable
tuple first; the hash a bare @DataClass(frozen=True) would generate
hashes that raw dict and raises TypeError on every instance.
PostgreSQL, MySQL, SQL Server, and SQLite each state their identity once
as a declared record and enrol in the registry, replacing the pair of
hand-written label and mark properties that had to agree with each
other. Every behavioral override is kept; the records are inert, since
nothing reads them yet.

Tier membership reproduces today's data-source list exactly, so the
lists derived from it later select the same backends they do now.
BigQuery, Databricks, Redshift, and Snowflake each state their identity
once as a declared record and enrol in the registry, replacing the pair
of hand-written label and mark properties. Every behavioral override is
kept and the records stay inert; nothing reads them yet.

Databricks declares autocommit and its insert parameter limit, matching
what the shared setup still derives from the dialect today. Redshift and
Snowflake each declare their dedicated CI lane. Redshift's lane token
differs from its pytest marker because the workflow selects it through a
translated alias, so the two are recorded separately.
_make_spec takes **overrides: object, so passing a str for a callable
field is not a type error and there is nothing to suppress. The unused
suppression fails the type check, which treats them as errors.
…ate the override contract

Registration now rejects an empty CI lane workflow job as well as an
empty marker token. A lane naming no job cannot be located in the
workflow file, so its wiring cannot be checked at all.

A test asserts every SQL config subclass still inherits the base
__eq__/__hash__. This is a test rather than an __init_subclass__ hook
because a class decorator runs after class creation: the hook observes
the class before @DataClass has replaced anything, so it cannot see the
regeneration it would be trying to prevent.

The declaration override's docstring now states that an override must
also vary the label. The field takes no part in equality, so instances
differing only in their override share one cached batch setup, which
surfaces as wrong data rather than an error.

The MySQL type override records that it currently restates a live value
and which one, since nothing reads the declared field yet.
# Conflicts:
#	tests/test_sql_backend_registry.py
…dential-gated-backends

The foundation work landed on the integration branch as a squash commit, so
this branch's copies of those commits no longer shared an ancestor with it and
the PR diff redundantly re-showed all of the already-reviewed foundation
changes.

Both conflicts are add/add artifacts of that squash:
  - registry.py: took the integration branch's copy (carries the enrol/enroll
    docstring fix applied at merge time); this branch made no changes to it.
  - test_sql_backend_registry.py: took this branch's copy, which is the
    integration branch's plus this branch's new registration tests.

No source change on this branch's own work.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant