Add Alembic integration tests and ship the YDB Alembic impl - #118
Open
vgvoleg wants to merge 8 commits into
Open
Add Alembic integration tests and ship the YDB Alembic impl#118vgvoleg wants to merge 8 commits into
vgvoleg wants to merge 8 commits into
Conversation
added 7 commits
September 3, 2026 19:02
Alembic support was documented and exercised only by examples/alembic, with no test covering it. Adding the tests surfaced that the integration code itself was not shipped: env.py had to define a DefaultImpl subclass and override the private MigrationContext._version to get a version table YDB accepts. That now lives in ydb_sqlalchemy.alembic, built on the public version_table_impl hook, so env.py only has to import it. The suite covers the version table, upgrade/downgrade over a revision chain, the operations available inside a revision, autogenerate diffs, and the YDB restrictions migrations have to work around. It caught a bind-type inference crash on sa.table()/sa.column() constructs, which is the form alembic documents for op.bulk_insert(). Documentation claimed alter_column worked for widening a string and for adding NOT NULL. YDB rejects both, so those examples are replaced with add-copy-drop and the limits are now asserted by tests.
The previous commit left the support level implicit: it was spread across test names and scattered notes. Add a Support Status section stating, per command and per operation, what works and what does not. Every row is backed by a test. Filling the gaps between the table and the suite added coverage for op.execute data migrations, unique indexes and two limits that had only been reasoned about: foreign keys are rejected, and a second head cannot be inserted into the version table. The create_table example still declared a ForeignKeyConstraint, which YDB rejects; it now uses a secondary index instead.
The previous commit claimed YDB has no ALTER COLUMN at all. It does: per the YQL reference it changes column options and can DROP NOT NULL. What it cannot do is change a type, which is what Alembic emits for type_=. The claim came from generalising a single parser error instead of reading the reference, and only the failing directions had tests, so nothing contradicted it. op.alter_column(nullable=True) in fact works, and now has a test. Also correct the advice to reach the remaining column options through op.execute: a plain DDL string is not marked as DDL by SQLAlchemy, so the dialect sends it to the query service and YDB rejects it. It has to be wrapped in sa.schema.DDL. Both directions are now tested.
Alembic ships alembic.testing.suite for dialect authors, the same way SQLAlchemy ships its compliance suite, and nothing was running it here. Wiring it needs one requirements class serving both suites, since Alembic reads its feature flags off the same object; test/alembic_requirements.py extends the dialect requirements with the Alembic flags and closes the ones YDB does not have. Two cases no flag covers are skipped explicitly: the FK options class, and the one autogenerate fixture whose table has no primary key. Result on a clean database: 22 passed, 84 skipped, none failing. The main suite is unaffected by the requirements change -- same single pre-existing failure before and after.
create_index(unique=True) compiles to ADD INDEX ... GLOBAL SYNC, without UNIQUE, so the index does not enforce uniqueness; reflection also reports unique: False for an index that is unique. YDB supports unique indexes and rejects duplicates on them, so this is a dialect gap. The earlier test only asserted the index existed, which passed while the guarantee was absent. It now inserts a duplicate and asserts it is accepted, so fixing the dialect will fail this test and force the docs to be updated.
upgrade --sql renders schema statements correctly but emits the version table insert with an unfilled placeholder, because Alembic only supplies version_num and the surrogate key column has no value to render. Removing the surrogate column does not help: version_num would become the primary key and the update that advances a revision cannot run on YDB. The support table said "not covered"; it can now say why it does not work.
A heading at the top of CHANGELOG.md breaks the release. python-publish.yml reads the release notes as everything above the first "## " line, so the "## Unreleased ##" added earlier in this branch made them empty and the publish job would have exited with "CHANGELOG empty". add_changelog_version in increment_version.py would then also have skipped inserting the real version header, since it returns early when the file starts with "##". Pending entries are now bare bullets. The two internal-only ones went away as well: tests and docs do not belong in release notes. Same rule and reasoning as ydb-platform/ydb-python-sdk#892, which put it in AGENTS.md. This repository had no such file, so this adds one.
There was a problem hiding this comment.
🟡 Changes recommended
Several documented command guarantees lack their promised integration coverage, and the migration guide contains an invalid empty directive.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds packaged Alembic integration for YDB, expands migration coverage, and documents supported behavior and limitations.
Changes:
- Adds
YDBImpland fixes lightweight-table bulk inserts. - Adds YDB-specific Alembic integration/compliance tests.
- Updates migration documentation, examples, and release guidance.
File summaries
| File | Description |
|---|---|
ydb_sqlalchemy/sqlalchemy/compiler/base.py |
Handles lightweight columns during bind inference. |
ydb_sqlalchemy/alembic.py |
Adds the YDB Alembic implementation. |
test/test_alembic.py |
Adds migration integration tests. |
test/test_alembic_suite.py |
Enables Alembic’s dialect suite. |
test/alembic_requirements.py |
Defines YDB feature requirements. |
test-requirements.txt |
Adds Alembic 1.14+. |
setup.cfg |
Selects combined test requirements. |
examples/alembic/README.md |
Simplifies integration instructions. |
examples/alembic/migrations/env.py |
Uses the packaged implementation. |
docs/migrations.rst |
Documents support and limitations. |
CHANGELOG.md |
Records user-facing changes. |
AGENTS.md |
Documents repository workflows. |
.gitignore |
Ignores Alembic test scratch files. |
Review details
- Files reviewed: 12/13 changed files
- Comments generated: 3
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+47
to
+51
| * - ``current``, ``history`` | ||
| - Supported | ||
| - | ||
| * - ``revision --autogenerate`` | ||
| - Supported |
The support table promised that every supported row is covered by a test, but nothing invoked command.current(), command.history() or command.revision(autogenerate=True) -- the autogenerate tests went through compare_metadata() and the generated env.py passed target_metadata=None, so the on-disk environment could not autogenerate at all. The test environment now takes target_metadata from config.attributes and scopes autogenerate with include_name, and TestCommands drives the four commands end to end. Capturing their output needs an explicit buffer: Config.stdout defaults to the sys.stdout bound at import time, which pytest's capture never sees. Autogenerate also only checked that an added index is detected; it now checks the removal too. Removes a stray empty code-block directive left in the create_table section when the foreign key example was replaced.
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.
Closes #117.
Alembic support was documented and exercised only by
examples/alembic, with nothing verifying it. Writing the tests surfaced that the integration code was never shipped: everyenv.pyhad to define its ownDefaultImplsubclass and overwrite the privateMigrationContext._version. That now lives inydb_sqlalchemy.alembic, built on Alembic's publicversion_table_implhook, soenv.pyjust imports it.docs/migrations.rstnow opens with a Support Status section stating per command and per operation what works and what does not. Every row in it is backed by a test, in both directions — the unsupported ones are asserted too, so a future YDB release lifting a restriction shows up as a failing test rather than as stale prose.Tests.
test/test_alembic.py(31 cases, ~30s): version table shape and head tracking;upgrade/downgradeover a three-revision chain, stepwise and to base;stamp; the operations usable in a revision body; autogenerate diffs including the empty diff when the model is in sync; YDB types; and the operations YDB rejects. Table names carry a unique suffix, so the suite is safe against a shared database.Bugs found:
op.bulk_insert()withsa.table()/sa.column()— the form Alembic documents — raisedAttributeErrorbecause_is_bound_to_nullable_columnassumed.nullable/.primary_key, whichColumnClausedoes not have. Fixed in the compiler.alter_columnwidening a string as supported, and usedalter_column(..., nullable=False)in two examples. YDB accepts neither. Replaced with add-copy-drop.create_tableexample declared aForeignKeyConstraint. YDB has no foreign keys, so that example could never have run.Where
alter_columnactually stands, since it is the fiddly part: YDB'sALTER COLUMNchanges column options and canDROP NOT NULL, but cannot change a type. Soalter_column(nullable=True)works, whilenullable=Falseandtype_=do not. The remaining options have no Alembic operation and needop.execute(sa.schema.DDL(...))— a plain string is not marked as DDL by SQLAlchemy, so the dialect sends it to the query service and YDB rejects it.Worth a look in review:
version_table_implkeeps the documented layout — a surrogate always-NULLidas primary key — rather than the more obviousversion_numprimary key.version_numcannot be the key because Alembic advances a revision withUPDATE ... SET version_num, and YDB cannot update a key column. The consequence, now tested and documented, is that branched history is unsupported.Alembic also ships
alembic.testing.suitefor third-party dialects, which nothing was running here. It now runs fromtest/test_alembic_suite.pywith the YDB feature flags intest/alembic_requirements.py: 22 passed, 84 skipped, none failing. Wiring it needs one requirements class serving both suites, since Alembic reads its flags off the same object as SQLAlchemy.A third bug, found while comparing against the Liquibase dialect:
create_index(unique=True)compiles toADD INDEX ... GLOBAL SYNCwithoutUNIQUE, so the index does not enforce uniqueness, and reflection reportsunique: Falseeven for one that does. YDB supports unique indexes and rejects duplicates on them. Not fixed here — it is a dialect concern, not an Alembic one — but the test now inserts a duplicate and asserts it is accepted, so fixing it will fail the test and force the docs to follow. Liquibase rejectsuniqueindexes outright rather than downgrading them silently.Offline mode (
upgrade --sql) is untested and the table says so rather than guessing.On a clean database the main suite has one pre-existing failure (
test_ydb_credentials_good, anUnauthorizedfrom the environment), identical before and after, including after therequirement_clschange.Also drops the
## Unreleased ##heading this branch had added toCHANGELOG.md:python-publish.ymlreads the release notes as everything above the first##line, so it would have made them empty and failed the publish job withCHANGELOG empty, andincrement_version.pywould have skipped inserting the real version header. Pending entries are bare bullets now, and the rule is written down in a newAGENTS.md— same reasoning as ydb-platform/ydb-python-sdk#892, which this repository had no equivalent of.