Skip to content

fix(alter): resolve foreign keys in the table's declaration context - #1617

Merged
dimitri-yatsenko merged 1 commit into
masterfrom
fix/alter-declaration-context
Oct 21, 2026
Merged

dimitri-yatsenko merged 1 commit into
masterfrom
fix/alter-declaration-context

Conversation

@dimitri-yatsenko

Copy link
Copy Markdown
Member

With no context argument, Table.alter() resolved the definition's
foreign keys in the caller's namespace. Called from any module but the
one that declared the table, such as a migration script or a notebook
that imports the pipeline module, alter() failed for every table with a
foreign key ("Foreign key reference Item could not be resolved") unless
the caller passed context= explicitly. Part.alter() already fell back to
the declaration context; other tables did not.

alter() now uses the table's declaration context when no context is
given, and the caller's namespace only for a table that has none. An
explicit context still takes precedence. The declaration context is the
namespace the definition was written in, and alter() cannot change
foreign keys, so it resolves the references the table was declared with.

The tests write a pipeline module to a temporary directory, import it,
declare its tables in the test schema, and alter them from the test
module, on MySQL and PostgreSQL: a table that references another, which
fails without the change; its Part table through -> master; and the
same table with an explicit context that names its parent differently.
Each alter adds an attribute, since changing an attribute's type emits
MODIFY, which PostgreSQL does not accept.

Closes #1602.

With no context argument, Table.alter() resolved the definition's
foreign keys in the caller's namespace. Called from any module but the
one that declared the table, such as a migration script or a notebook
that imports the pipeline module, alter() failed for every table with a
foreign key ("Foreign key reference Item could not be resolved") unless
the caller passed context= explicitly. Part.alter() already fell back to
the declaration context; other tables did not.

alter() now uses the table's declaration context when no context is
given, and the caller's namespace only for a table that has none. An
explicit context still takes precedence. The declaration context is the
namespace the definition was written in, and alter() cannot change
foreign keys, so it resolves the references the table was declared with.

The tests write a pipeline module to a temporary directory, import it,
declare its tables in the test schema, and alter them from the test
module, on MySQL and PostgreSQL: a table that references another, which
fails without the change; its Part table through `-> master`; and the
same table with an explicit context that names its parent differently.
Each alter adds an attribute, since changing an attribute's type emits
MODIFY, which PostgreSQL does not accept.

Closes #1602.

@MilagrosMarin MilagrosMarin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @dimitri-yatsenko — this brings Table.alter() in line with Part.alter(), which already fell back to the declaration context.

✅ Resolving against the declaration context is right: alter() can't change foreign keys (declare.py raises NotImplementedError if it would), so the names the table was declared with are the ones that should resolve.
✅ test_alter_from_another_module fails without the change, since the test module has no Item, and the Part case shows -> master still resolves.
✅ CI green, and it merges cleanly onto current master (after #1592, which also touches table.py).

Nit, test_alter_explicit_context: the comment says "Parent resolves only in the explicit context", but test_alter.py imports Parent from tests.schema_alter at the top, so it also resolves in the caller's namespace. The test still discriminates: if the explicit context were ignored and the caller's namespace used, -> Parent would point at a different table and alter() would raise on the foreign-key change. Renaming it to something defined nowhere else (Upstream, say) would make the comment true.

Approving — looks good to me.

@dimitri-yatsenko
dimitri-yatsenko merged commit 1f90243 into master Oct 21, 2026
9 checks passed
@dimitri-yatsenko
dimitri-yatsenko deleted the fix/alter-declaration-context branch October 21, 2026 15:27
@dimitri-yatsenko

Copy link
Copy Markdown
Member Author

Thanks, @MilagrosMarin. Merged. You're right about test_alter_explicit_context: Parent also resolves in the caller's namespace. The test still tells the two contexts apart, but its comment overstates it. I'll make the comment true in #1639, which rewrites the alter tests for PostgreSQL.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Indicates an unexpected problem or unintended behavior

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Table.alter() resolves foreign keys in the caller's namespace, not the table's declaration context

2 participants