Skip to content

Commit 1ba8b9f

Browse files
Merge pull request #1635 from datajoint/fix/alter-rename-keeps-data-2.3
fix(alter): keep an attribute's values when alter() renames it (2.3)
2 parents da431b1 + 308f6be commit 1ba8b9f

2 files changed

Lines changed: 83 additions & 5 deletions

File tree

‎src/datajoint/declare.py‎

Lines changed: 24 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -638,7 +638,9 @@ def declare(
638638
return sql, external_stores, primary_key, fk_attribute_map, pre_ddl, post_ddl
639639

640640

641-
def _make_attribute_alter(new: list[str], old: list[str], primary_key: list[str], adapter) -> list[str]:
641+
def _make_attribute_alter(
642+
new: list[str], old: list[str], primary_key: list[str], adapter, comments: dict[str, str] | None = None
643+
) -> list[str]:
642644
"""
643645
Generate SQL ALTER commands for attribute changes.
644646
@@ -652,6 +654,9 @@ def _make_attribute_alter(new: list[str], old: list[str], primary_key: list[str]
652654
Primary key attribute names (cannot be altered).
653655
adapter : DatabaseAdapter
654656
Database adapter for backend-specific SQL generation.
657+
comments : dict, optional
658+
The new attributes' comments as declared, ``name -> comment``. A comment
659+
that opens with ``{old_name}`` renames that attribute.
655660
656661
Returns
657662
-------
@@ -666,9 +671,16 @@ def _make_attribute_alter(new: list[str], old: list[str], primary_key: list[str]
666671
# parse attribute names - use adapter's quote character
667672
quote_char = re.escape(adapter.quote_identifier("x")[0])
668673
name_regexp = re.compile(rf"^{quote_char}(?P<name>\w+){quote_char}")
669-
original_regexp = re.compile(r'COMMENT "{\s*(?P<name>\w+)\s*}')
670-
matched = ((name_regexp.match(d), original_regexp.search(d)) for d in new)
671-
new_names = dict((d.group("name"), n and n.group("name")) for d, n in matched)
674+
# The rename marker opens the comment as declared. A core type or codec puts
675+
# its `:type:` marker in front of it, and PostgreSQL keeps comments out of the
676+
# column SQL, so the marker is read from the declared comment, not the SQL.
677+
original_regexp = re.compile(r"^(?::[^:]*:)?{\s*(?P<name>\w+)\s*}")
678+
comments = comments or {}
679+
new_names = {}
680+
for d in new:
681+
name = name_regexp.match(d).group("name")
682+
original = original_regexp.match(comments.get(name, ""))
683+
new_names[name] = original and original.group("name")
672684
old_names = [name_regexp.search(d).group("name") for d in old]
673685

674686
# verify that original names are only used once
@@ -717,6 +729,13 @@ def _make_attribute_alter(new: list[str], old: list[str], primary_key: list[str]
717729
command = "ADD"
718730
elif not old_name:
719731
command = "MODIFY"
732+
elif adapter.backend != "mysql":
733+
# Without this check the rename went unnoticed on PostgreSQL and
734+
# became DROP + ADD, discarding the attribute's values.
735+
raise DataJointError(
736+
f"Renaming attribute `{old_name}` to `{new_name}` with alter() is not supported "
737+
f"on {adapter.backend}. No change was made."
738+
)
720739
else:
721740
command = f"CHANGE {adapter.quote_identifier(old_name)}"
722741

@@ -816,7 +835,7 @@ def alter(
816835
if index_sql != index_sql_:
817836
raise NotImplementedError("table.alter cannot alter indexes (yet)")
818837
if attribute_sql != attribute_sql_:
819-
sql.extend(_make_attribute_alter(attribute_sql, attribute_sql_, primary_key, adapter))
838+
sql.extend(_make_attribute_alter(attribute_sql, attribute_sql_, primary_key, adapter, column_comments))
820839
if table_comment != table_comment_:
821840
# For MySQL: COMMENT="new comment"
822841
# For PostgreSQL: would need COMMENT ON TABLE, but that's not an ALTER TABLE clause
Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,59 @@
1+
"""
2+
Renaming an attribute with ``alter()`` keeps its values.
3+
4+
A comment that opens with ``{old_name}`` renames that attribute. On MySQL the
5+
rename is a CHANGE, which keeps the values; PostgreSQL refuses the rename
6+
instead of dropping the attribute and adding an empty one.
7+
"""
8+
9+
import time
10+
11+
import pytest
12+
13+
import datajoint as dj
14+
from datajoint.errors import DataJointError
15+
16+
17+
@pytest.fixture(scope="function")
18+
def schema_by_backend(connection_by_backend, db_creds_by_backend):
19+
backend = db_creds_by_backend["backend"]
20+
schema_name = f"djtest_alter_rename_{backend}_{str(int(time.time() * 1000))[-8:]}"
21+
schema = dj.Schema(schema_name, connection=connection_by_backend)
22+
yield schema
23+
schema.drop(prompt=False)
24+
25+
26+
@pytest.mark.parametrize("nullable", [False, True], ids=["required", "nullable"])
27+
def test_rename_keeps_values(schema_by_backend, db_creds_by_backend, nullable):
28+
"""Rename a core-type attribute of a table that holds rows."""
29+
default = " = null" if nullable else ""
30+
31+
@schema_by_backend
32+
class Measurement(dj.Manual):
33+
definition = f"""
34+
measurement_id : int32
35+
---
36+
value{default} : float32 # the value
37+
"""
38+
39+
Measurement.insert([(1, 2.5), (2, 4.0)])
40+
Measurement.definition = f"""
41+
measurement_id : int32
42+
---
43+
reading{default} : float32 # {{value}} the value
44+
"""
45+
46+
if db_creds_by_backend["backend"] == "mysql":
47+
Measurement.alter(prompt=False)
48+
assert Measurement.heading.names == ["measurement_id", "reading"]
49+
assert Measurement.to_dicts(order_by="measurement_id") == [
50+
{"measurement_id": 1, "reading": 2.5},
51+
{"measurement_id": 2, "reading": 4.0},
52+
]
53+
else:
54+
with pytest.raises(DataJointError, match="Renaming attribute `value` to `reading`"):
55+
Measurement.alter(prompt=False)
56+
assert dj.FreeTable(schema_by_backend.connection, Measurement.full_table_name).to_dicts(order_by="measurement_id") == [
57+
{"measurement_id": 1, "value": 2.5},
58+
{"measurement_id": 2, "value": 4.0},
59+
]

0 commit comments

Comments
 (0)