literal_column() in ExcludeConstraint are autogenerated as column(), failing in the migration #1184
Comments
|
Suggested fix is in #1185 |
jankatins
changed the title
|
Hi, thanks for reporting. That looks indeed like a bug. |
CaselIT
added
bug
|
Adding - postgresql.ExcludeConstraint((sa.column('branch_id'), '='), (sa.column('int8range(commit_id_from, commit_id_to)'), '&&'), (sa.column('mrid'), '='), using='gist', name='whatever_branch_id_int8range_mrid_excl'),
+ postgresql.ExcludeConstraint((sa.column('branch_id'), '='), (sa.column('mrid'), '='), using='gist', name='whatever_branch_id_int8range_mrid_excl'),-> The whole element is removed :-( |
|
If I remember correctly, that was due to for (expr, column, strname, add_element), operator in zip(
coercions.expect_col_expression_collection(
roles.DDLConstraintColumnRole, expressions
),
operators,
):
if add_element is not None:
columns.append(add_element) |
|
I'll take a look at how sqlalchemy behaves with text. @zzzeek do you think text should be supported? |
|
One thing which should IMO be changed is that E.g. debugging that part was a real pain: I tried using functions |
|
That's something that indeed should be improved. let's wait for mike's opinion on these other things. currently on sqlalchemy we can improve the documentation indicating that |
I would look to see what rendering for Index and CheckConstraint do. Whatever is implemented for ExcludeConstraint should work for those as well otherwise we are being inconsistent. |
|
Re test cases: the end resulting object in sqlalchemy and the migration SQL via alembic should look the same, shouldn't it? |
|
based on how are currently implemented constraints supporting just In any case what we can do is better document it and error in case of an unsupported case instead behaving like now that results in a more or less broken state |
The rendering seems fine for the most part, but the exclusion constraint does not use literal binding, as evidenced by the wrong function rendereing In both index and exclude constraint using import sqlalchemy as sa
from sqlalchemy.schema import CreateTable, CreateIndex
from sqlalchemy.dialects.postgresql import dialect, ExcludeConstraint
m = sa.MetaData()
t = sa.Table(
"x",
m,
sa.Column("id", sa.Integer),
sa.Column("a", sa.Integer),
sa.Column("b", sa.Integer),
idx := sa.Index(
"my-idx",
"a",
sa.text("a+1"),
sa.column("b"),
sa.literal_column("a+b"),
sa.func.power("a", 2),
),
ex := ExcludeConstraint(
("a", "="),
(sa.text("a+1"), "="),
(sa.column("b"), "&&"),
(sa.literal_column("a+b"), "^^"),
(sa.func.power(sa.column("a"), 2), '!'),
name="my-ex",
),
)
print(CreateTable(t).compile(dialect=dialect()).string)
print(CreateIndex(idx).compile(dialect=dialect()).string)
print("index cols", idx.columns)
print("exclude cols", ex.columns)The following prints CREATE TABLE x (
id INTEGER,
a INTEGER,
b INTEGER,
CONSTRAINT "my-ex" EXCLUDE USING gist (a WITH =, "a+1" WITH =, b WITH &&, a+b WITH ^^, power(a, %(power_1)s) WITH !)
)
CREATE INDEX "my-idx" ON x (a, a+1, b, a+b, power('a', 2))
index cols ReadOnlyColumnCollection(x.a, b, a+b)
exclude cols ReadOnlyColumnCollection(a, "a+1", b, a+b)So there is currently for sure a bug in alembic regarding exclude constraint since it does not consider |
|
Forgot to check |
|
The issue with literal binds was fixed in https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/4462. |
|
@CaselIT Thanks, looks good. As a sqlalachemy beginner, I would appreciate an example what a E.g. A sequence of two tuples of the form ``(column, operator)`` where
"column" is either a :class:`_schema.Column` object, or a
SQL expression element (e.g. ``func.int8range(table.from, table.to)``) or
the name of a column as string. "operator" is a string containing the
operator to use (e.g. `"&&"` or `"="`).
In order to specify a column name when a :class:`_schema.Column`
object is not available, while ensuring that any necessary quoting rules
take effect, an ad-hoc :class:`_schema.Column` or
:func:`_expression.column` object should be used.
``column`` may also be a string SQL expression when
passed as :func:`_expression.literal_column`( I assume |
|
BTW: is #1185 of use or should I close it? It's currently in limbo because the gerrit check is not started. |
I've yet to look at it sorry. |
|
thanks for the suggestion, I'll integrate it in the docs
text seems to work too. We may need to support in in alembic though |
I can add that to my PR. It's should be treated the same as |
yes it should, it seems to behave the same, with the only difference being that Not sure if it makes sense make alembic render them as the same or keep the difference |
|
so the bug is not here, it's sqlalchemy/sqlalchemy#9349 and this can be closed, is that accurate? |
zzzeek
added
external SQLAlchemy issues
|
Well if sqlalchemy accepts literal_columns and text in exclude contraints, alembic should too, so the issue here is valid |
|
to be clear, you mean in the autogenerate Python rendering as I've labeled, is that correct? |
zzzeek
removed
external SQLAlchemy issues
|
Yes, other that that I don't think there are other issues here |
|
Jan Katins has proposed a fix for this issue in the main branch: improve autogen rendering for PG ExcludeConstraint https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/4493 |
Describe the bug
If you add a literal_column in a ExcludeConstraint (e.g. a
literal_column("int8range(col_from, col_to)")), this is rendered ascolumn("int8range(col_from, col_to)")in the autogenerated migration and the migration will fail as such a column does not exist in the table.Expected behavior
literal_columns are passed through in the rendered migration
To Reproduce
Error
The migration code after
alembic revision --autogenerate -m "test2"looks like this:running this migration via
alembic upgrade headresults in:Versions.
The text was updated successfully, but these errors were encountered: