Repository navigation
fix(alter): resolve foreign keys in the table's declaration context - #1617
Conversation
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
left a comment
There was a problem hiding this comment.
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.
|
Thanks, @MilagrosMarin. Merged. You're right about |
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 thesame 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.