From 01342d3e68a92d2b9936ce922bea174c2c530073 Mon Sep 17 00:00:00 2001 From: Dimitri Yatsenko Date: Sat, 17 Oct 2026 19:15:45 -0500 Subject: [PATCH] fix(alter): change and rename attributes on PostgreSQL On PostgreSQL, `Table.alter()` could add and drop attributes but not change them. A change of type, nullability or default emitted MySQL's `MODIFY`, a syntax error. A change of comment alone was ignored: PostgreSQL keeps comments out of the column declaration, and the declarations were what alter() compared. alter() now compares the parts of each column (type, nullability, default and comment) instead of the compiled declarations, and a new adapter method, `alter_table_ddl`, turns the differences into statements. MySQL still gets one ALTER TABLE with `MODIFY`, `CHANGE`, `ADD` and `DROP`, and unit tests pin those statements to what master produced. PostgreSQL gets `ALTER COLUMN ... TYPE`, `SET`/`DROP NOT NULL` and `SET`/`DROP DEFAULT`; `RENAME COLUMN`; and `COMMENT ON COLUMN` for each new or changed comment, carrying the core-type marker (`:float64:`) that describe() reads back. A changed table comment becomes `COMMENT ON TABLE` in place of MySQL's `COMMENT=`, which was also a syntax error there. The statements run in one transaction, so a failure in any of them leaves the table as it was. A type change adds a `USING` expression where PostgreSQL has no assignment conversion: strings and enum labels are parsed as the new type, numbers become booleans by comparison with zero, booleans become 0 or 1, and other values become JSON through `to_jsonb`. A conversion with no meaning, such as a date to a number, raises a DataJointError before anything runs. A default is dropped before the type changes and set again after it, because PostgreSQL would otherwise try to convert the default as well. A change to an enum creates the new type first, as adding an enum attribute already did. Renames now work on PostgreSQL as `RENAME COLUMN`. The rename marker is read from the comment as written (#1634, which this builds on); where #1634 refused a rename on PostgreSQL, this carries it out. The MySQL statements are unchanged. describe() on PostgreSQL wrote defaults with the cast the server reports, as `value='1.5'::real` or `label="'abc'::character varying"`. alter() parses describe() for the current definition, so the first made it fail on any table with such a default, and the second reads as a different default. The adapter now reports the value alone, as MySQL does. New integration tests run on both backends. Each alters a table that holds data and checks that the table then describes itself exactly as a table declared from the new definition does, that alter() finds nothing left to change, and that the values survived. They cover a change of type (including to and from strings, booleans and enums), nullability, default, attribute comment and table comment; several changes in one alter; a rename alone and with other changes; a failed change, which leaves the table unchanged; and a refused conversion. Closes #1625. --- src/datajoint/adapters/base.py | 56 +++ src/datajoint/adapters/mysql.py | 44 +++ src/datajoint/adapters/postgres.py | 199 +++++++++- src/datajoint/declare.py | 247 ++++++++----- src/datajoint/table.py | 32 +- tests/integration/test_alter_attributes.py | 404 ++++++++++++++++++++ tests/integration/test_alter_rename.py | 26 +- tests/unit/test_alter_sql.py | 410 +++++++++++++++++++++ 8 files changed, 1291 insertions(+), 127 deletions(-) create mode 100644 tests/integration/test_alter_attributes.py create mode 100644 tests/unit/test_alter_sql.py diff --git a/src/datajoint/adapters/base.py b/src/datajoint/adapters/base.py index 5b4a7ab72..1b423f346 100644 --- a/src/datajoint/adapters/base.py +++ b/src/datajoint/adapters/base.py @@ -437,6 +437,46 @@ def alter_table_sql( """ ... + @abstractmethod + def alter_table_ddl( + self, + full_table_name: str, + drop: list[str], + changes: list[dict[str, Any]], + table_comment: str | None = None, + ) -> list[str]: + """ + Generate the statements that apply ``Table.alter()`` changes. + + Parameters + ---------- + full_table_name : str + Quoted name of the table. + drop : list[str] + Names of the attributes to drop. + changes : list[dict] + Attributes to add or change, in declaration order. Each has keys + ``new`` and ``old`` (the parts of the column after and before the + change, ``old`` None for an added column), ``renamed_from`` (the + name in a rename marker, or None) and ``after`` (the attribute to + place it after, or None). The parts are dicts with keys ``name``, + ``type``, ``nullable``, ``default`` (an SQL expression or None), + ``comment`` and ``sql`` (the compiled column declaration). + table_comment : str, optional + The new table comment, or None when it is unchanged. + + Returns + ------- + list[str] + Statements to run in order; empty when there is nothing to change. + + Raises + ------ + DataJointError + If the backend cannot convert an attribute to its new type. + """ + ... + @abstractmethod def add_comment_sql( self, @@ -652,6 +692,22 @@ def supports_column_position(self) -> bool: """ return True # Default for MySQL, override in PostgreSQL + @property + def supports_transactional_ddl(self) -> bool: + """ + Whether DDL statements can run inside a transaction and roll back. + + MySQL commits implicitly before and after each DDL statement. + PostgreSQL runs DDL inside transactions, so a change made of several + statements can be applied completely or not at all. + + Returns + ------- + bool + False for MySQL, True for PostgreSQL. + """ + return False # Default for MySQL, override in PostgreSQL + @property def auto_indexes_foreign_keys(self) -> bool: """ diff --git a/src/datajoint/adapters/mysql.py b/src/datajoint/adapters/mysql.py index 2ef9baf84..5e272d460 100644 --- a/src/datajoint/adapters/mysql.py +++ b/src/datajoint/adapters/mysql.py @@ -490,6 +490,50 @@ def alter_table_sql( return f"ALTER TABLE {table_name} {', '.join(clauses)}" + def alter_table_ddl( + self, + full_table_name: str, + drop: list[str], + changes: list[dict[str, Any]], + table_comment: str | None = None, + ) -> list[str]: + """ + Generate the ALTER TABLE statement that applies ``Table.alter()`` changes. + + MySQL makes every change in one statement: ``MODIFY`` restates a + column's whole declaration, comment included, and ``CHANGE`` also + renames it. + + Parameters + ---------- + full_table_name : str + Quoted name of the table. + drop : list[str] + Names of the attributes to drop. + changes : list[dict] + Attributes to add or change; see ``DatabaseAdapter.alter_table_ddl``. + table_comment : str, optional + The new table comment, or None when it is unchanged. + + Returns + ------- + list[str] + One ALTER TABLE statement, or none when there is nothing to change. + """ + clauses = [f"DROP {self.quote_identifier(name)}" for name in drop] + for change in changes: + if change["old"] is None: + command = "ADD" + elif change["renamed_from"] is None: + command = "MODIFY" + else: + command = f"CHANGE {self.quote_identifier(change['renamed_from'])}" + after = "" if change["after"] is None else f"AFTER {self.quote_identifier(change['after'])}" + clauses.append(f"{command} {change['new']['sql']} {after}") + if table_comment is not None: + clauses.append(f'COMMENT="{table_comment}"') + return [f"ALTER TABLE {full_table_name}\n\t" + ",\n\t".join(clauses)] if clauses else [] + def add_comment_sql( self, object_type: str, diff --git a/src/datajoint/adapters/postgres.py b/src/datajoint/adapters/postgres.py index 3a55d8eb8..e7bbf5df5 100644 --- a/src/datajoint/adapters/postgres.py +++ b/src/datajoint/adapters/postgres.py @@ -51,6 +51,27 @@ "date": "date", } +# Groups of PostgreSQL types, for choosing how ALTER COLUMN ... TYPE converts +# values. Patterns are matched against the whole type, ignoring case. +TYPE_FAMILIES = { + "numeric": r"(smallint|integer|int[248]?|bigint|real|float[48]?|double precision|numeric|decimal)(\s*\([\d\s,]*\))?", + "boolean": r"bool(ean)?", + "string": r"(varchar|character varying|char|character|bpchar|text)(\s*\(\d+\))?", + "datetime": r"date|timestamp(\s*\(\d+\))?(\s+without time zone)?", + "uuid": r"uuid", + "bytes": r"bytea", + "json": r"jsonb?", + "enum": r'.*"enum_\w+"', # schema-qualified type created for an enum attribute +} + + +def _type_family(sql_type: str) -> str | None: + """Family of a PostgreSQL type in TYPE_FAMILIES, or None if it is in none.""" + return next( + (family for family, pattern in TYPE_FAMILIES.items() if re.fullmatch(pattern, sql_type.strip(), re.I)), + None, + ) + class PostgreSQLAdapter(DatabaseAdapter): """PostgreSQL database adapter implementation.""" @@ -573,6 +594,162 @@ def alter_table_sql( return f"ALTER TABLE {table_name} {', '.join(clauses)}" + def alter_table_ddl( + self, + full_table_name: str, + drop: list[str], + changes: list[dict[str, Any]], + table_comment: str | None = None, + ) -> list[str]: + """ + Generate the statements that apply ``Table.alter()`` changes. + + PostgreSQL changes a column one property at a time (``ALTER COLUMN ... + TYPE``, ``SET``/``DROP NOT NULL``, ``SET``/``DROP DEFAULT``), renames + it in a statement of its own, and keeps comments, which carry the + core-type marker, outside the table definition (``COMMENT ON``). The + statements run in this order: drops, renames, then one ALTER TABLE for + the added and changed columns, then comments. Dropping first frees a + dropped name for a rename; renaming before the changes lets them name + the column as it is now. Columns cannot be positioned, so ``after`` is + ignored. + + Parameters + ---------- + full_table_name : str + Quoted name of the table. + drop : list[str] + Names of the attributes to drop. + changes : list[dict] + Attributes to add or change; see ``DatabaseAdapter.alter_table_ddl``. + table_comment : str, optional + The new table comment, or None when it is unchanged. + + Returns + ------- + list[str] + Statements to run in order, inside one transaction. + + Raises + ------ + DataJointError + If PostgreSQL cannot convert an attribute to its new type. + """ + statements = [] + if drop: + drops = [f"DROP COLUMN {self.quote_identifier(name)}" for name in drop] + statements.append(f"ALTER TABLE {full_table_name}\n\t" + ",\n\t".join(drops)) + for change in changes: + source, name = change["renamed_from"], change["new"]["name"] + if source is not None and source != name: + source, name = self.quote_identifier(source), self.quote_identifier(name) + statements.append(f"ALTER TABLE {full_table_name} RENAME COLUMN {source} TO {name}") + clauses, comments = [], [] + for change in changes: + new, old = change["new"], change["old"] + if old is None: + clauses.append(f"ADD COLUMN {new['sql']}") + else: + clauses.extend(self._alter_column_clauses(old, new)) + if new["comment"] != (old["comment"] if old else None): + # An empty comment removes it + comments.append(self.column_comment_ddl(full_table_name, new["name"], new["comment"] or "")) + if clauses: + statements.append(f"ALTER TABLE {full_table_name}\n\t" + ",\n\t".join(clauses)) + statements.extend(comments) + if table_comment is not None: + statements.append(self.table_comment_ddl(full_table_name, table_comment)) + return statements + + def _alter_column_clauses(self, old: dict[str, Any], new: dict[str, Any]) -> list[str]: + """ + ALTER COLUMN clauses that change a column from ``old`` to ``new``. + + Parameters + ---------- + old, new : dict + The parts of the column before and after; see ``DatabaseAdapter.alter_table_ddl``. + The clauses name the column by its new name. + + Returns + ------- + list[str] + Clauses for one ALTER TABLE statement. + """ + column = self.quote_identifier(new["name"]) + # PostgreSQL stores no default for DEFAULT NULL: it is the same as none + old_default = None if old["default"] in (None, "NULL") else old["default"] + new_default = None if new["default"] in (None, "NULL") else new["default"] + retyped = old["type"] != new["type"] + clauses = [] + if retyped: + # A default is converted with the column, and may not convert, so it + # is dropped and set again. Within one statement PostgreSQL drops + # defaults before it changes types and sets defaults after. + if old_default is not None: + clauses.append(f"ALTER COLUMN {column} DROP DEFAULT") + using = self._type_conversion(new["name"], old["type"], new["type"]) + clauses.append(f"ALTER COLUMN {column} TYPE {new['type']}" + ("" if using is None else f" USING {using}")) + if old["nullable"] != new["nullable"]: + clauses.append(f"ALTER COLUMN {column} {'DROP' if new['nullable'] else 'SET'} NOT NULL") + if new_default is not None and (retyped or new_default != old_default): + clauses.append(f"ALTER COLUMN {column} SET DEFAULT {new_default}") + elif new_default is None and old_default is not None and not retyped: + clauses.append(f"ALTER COLUMN {column} DROP DEFAULT") + return clauses + + def _type_conversion(self, name: str, old_type: str, new_type: str) -> str | None: + """ + The USING expression that converts a column to a new type, if one is needed. + + Without USING, PostgreSQL converts the way it does on assignment, which + it can do between types of one family and from any type to a string + type. Other conversions need an explicit expression: strings and enum + labels are parsed as the new type, numbers become booleans by + comparison with zero and booleans become 0 or 1, and values become JSON + through ``to_jsonb``. When a conversion fails on a value, PostgreSQL + reports it and nothing is changed. + + Parameters + ---------- + name : str + Column name. + old_type, new_type : str + PostgreSQL types before and after. + + Returns + ------- + str or None + The expression, or None where an assignment conversion exists or a + type is not one this method knows (PostgreSQL then decides). + + Raises + ------ + DataJointError + If there is no conversion between the two types, e.g. between a date + and a number. + """ + old_family, new_family = _type_family(old_type), _type_family(new_type) + column = self.quote_identifier(name) + if new_family == "string" or (old_family == new_family and old_family != "enum"): + return None + if old_family is None or new_family is None: + return None + if "enum" in (old_family, new_family): + return f"{column}::text::{new_type}" + if old_family == "string" or (old_family == "json" and new_family in ("numeric", "boolean")): + return f"{column}::{new_type}" + if (old_family, new_family) == ("numeric", "boolean"): + return f"{column} <> 0" + if (old_family, new_family) == ("boolean", "numeric"): + return f"{column}::integer" + if new_family == "json": + return f"to_jsonb({column})" + raise errors.DataJointError( + f"PostgreSQL cannot convert attribute `{name}` from {old_type} to {new_type}. " + "Add an attribute of the new type, copy the values into it, and drop the old attribute instead." + ) + def add_comment_sql( self, object_type: str, @@ -728,6 +905,14 @@ def supports_column_position(self) -> bool: """ return False + @property + def supports_transactional_ddl(self) -> bool: + """ + PostgreSQL runs DDL inside transactions, so the several statements of + one alter apply together or not at all. + """ + return True + @property def auto_indexes_foreign_keys(self) -> bool: """ @@ -1001,11 +1186,19 @@ def parse_column_info(self, row: dict[str, Any]) -> dict[str, Any]: else: data_type = f"decimal({num_precision})" + # PostgreSQL reports a literal default cast to the column type, e.g. + # 'abc'::character varying. Report the value alone, as MySQL does, so + # that describe() writes the default the definition declared. + default = row["column_default"] + literal = re.fullmatch(r"'((?:[^']|'')*)'::[\w\s\".\[\]()]+", default or "") + if literal: + default = literal[1].replace("''", "'") + return { "name": row["column_name"], "type": data_type, "nullable": row["is_nullable"] == "YES", - "default": row["column_default"], + "default": default, "comment": row.get("column_comment"), # Retrieved via col_description() "key": "", # PostgreSQL key info retrieved separately "extra": "", # PostgreSQL doesn't have auto_increment in same way @@ -1257,7 +1450,7 @@ def table_options_clause(self, comment: str | None = None) -> str: """ return "" # PostgreSQL uses COMMENT ON TABLE separately - def table_comment_ddl(self, full_table_name: str, comment: str) -> str | None: + def table_comment_ddl(self, full_table_name: str, comment: str) -> str: """ Generate COMMENT ON TABLE statement for PostgreSQL. @@ -1270,7 +1463,7 @@ def table_comment_ddl(self, full_table_name: str, comment: str) -> str | None: escaped_comment = comment.replace("'", "''") return f"COMMENT ON TABLE {full_table_name} IS '{escaped_comment}'" - def column_comment_ddl(self, full_table_name: str, column_name: str, comment: str) -> str | None: + def column_comment_ddl(self, full_table_name: str, column_name: str, comment: str) -> str: """ Generate COMMENT ON COLUMN statement for PostgreSQL. diff --git a/src/datajoint/declare.py b/src/datajoint/declare.py index 438bb72b0..b4c862688 100644 --- a/src/datajoint/declare.py +++ b/src/datajoint/declare.py @@ -192,6 +192,48 @@ def is_foreign_key(line: str) -> bool: return arrow_position >= 0 and not any(c in line[:arrow_position] for c in "\"#'") +def _column( + name: str, + sql_type: str, + nullable: bool, + default: str | None, + comment: str | None, + sql: str, + renamed_from: str | None = None, +) -> dict: + """ + The parts of a declared column, as ``alter`` compares them. + + Parameters + ---------- + name : str + Column name. + sql_type : str + Backend type, e.g. ``"real"`` or a schema-qualified enum type. + nullable : bool + Whether the column accepts NULL. + default : str or None + Default as an SQL expression (``"NULL"`` for a nullable attribute, + ``"'1.5'"``, ``"CURRENT_TIMESTAMP"``), or None when there is none. + comment : str or None + Comment as stored, including the ``:type:`` marker of a core type. + sql : str + The compiled column declaration. + renamed_from : str, optional + The attribute this one renames, from its rename marker (see + ``_renamed_from``). + + Returns + ------- + dict + Keys ``name``, ``type``, ``nullable``, ``default``, ``comment``, ``sql`` + and ``renamed_from``. + """ + return dict( + name=name, type=sql_type, nullable=nullable, default=default, comment=comment, sql=sql, renamed_from=renamed_from + ) + + def compile_foreign_key( line: str, context: dict, @@ -204,6 +246,7 @@ def compile_foreign_key( fk_attribute_map: dict[str, tuple[str, str]] | None = None, fk_index_candidates: list[list[str]] | None = None, column_comments: dict[str, str] | None = None, + columns: dict[str, dict] | None = None, ) -> None: """ Parse a foreign key line and update declaration components. @@ -239,6 +282,9 @@ def compile_foreign_key( out of line (PostgreSQL's ``COMMENT ON COLUMN``). Each copied column's comment, with its core-type marker (e.g. ``:int32:``), is added so the child keeps the parent's declared type. Updated in place. + columns : dict, optional + Mapping of ``column -> parts`` (see ``_column``) for ``alter``. + Updated in place. Raises ------ @@ -305,6 +351,8 @@ def compile_foreign_key( attr_sql.append(col_def) if comment and column_comments is not None: column_comments[attr] = comment + if columns is not None: + columns[attr] = _column(attr, sql_type, is_nullable, None, comment or None, col_def) # Track FK attribute mapping for lineage: child_attr -> (parent_table, parent_attr) if fk_attribute_map is not None: @@ -382,7 +430,7 @@ def _tier_attributes(table_name: str | None, config) -> tuple[str, ...]: def prepare_declare( - definition: str, context: dict, adapter, *, table_name: str | None = None, config=None + definition: str, context: dict, adapter, *, table_name: str | None = None, config=None, columns: dict | None = None ) -> tuple[str, list[str], list[str], list[str], list[str], list[str], dict[str, tuple[str, str]], dict[str, str]]: """ Parse a table definition into its components. @@ -407,6 +455,9 @@ def prepare_declare( declares them. config : Config, optional Read for ``jobs.add_job_metadata`` and ``provenance.capture``. + columns : dict, optional + Filled in place with ``column -> parts`` (see ``_column``) for every + attribute, in declaration order. Returns ------- @@ -440,7 +491,8 @@ def prepare_declare( fk_index_candidates = [] # PostgreSQL: FK column-lists that may need a support index (#1512) def add_attribute(line: str, in_key: bool) -> None: - name, sql, store, comment = compile_attribute(line, in_key, foreign_key_sql, context, adapter) + column = {} + name, sql, store, comment = compile_attribute(line, in_key, foreign_key_sql, context, adapter, column=column) if store: external_stores.append(store) if in_key and name not in primary_key: @@ -450,6 +502,8 @@ def add_attribute(line: str, in_key: bool) -> None: attribute_sql.append(sql) if comment: column_comments[name] = comment + if columns is not None: + columns[name] = column for line in definition: if not line or line.startswith("#"): # ignore additional comments @@ -469,6 +523,7 @@ def add_attribute(line: str, in_key: bool) -> None: fk_attribute_map, fk_index_candidates, column_comments, + columns, ) elif re.match(r"^(unique\s+)?index\s*\(.*\)\s*(#.*)?$", line, re.I): # index compile_index(re.sub(r"\s*#.*$", "", line), index_sql, adapter) @@ -648,50 +703,67 @@ def declare( return sql, external_stores, primary_key, fk_attribute_map, pre_ddl, post_ddl +def _renamed_from(comment: str) -> str | None: + """ + Name of the attribute an attribute renames, if its comment says so. + + A comment that opens with an attribute name in braces renames that + attribute: ``reading : float32 # {value} the value`` renames ``value`` to + ``reading``. + + Parameters + ---------- + comment : str + The comment written in the definition, without the ``:type:`` marker + a core type adds to the stored comment. + + Returns + ------- + str or None + The name in braces, or None. + """ + match = re.match(r"{\s*(?P\w+)\s*}", comment) + return match["name"] if match else None + + def _make_attribute_alter( - new: list[str], old: list[str], primary_key: list[str], adapter, comments: dict[str, str] | None = None -) -> list[str]: + new: dict[str, dict], old: dict[str, dict], primary_key: list[str], adapter +) -> tuple[list[str], list[dict]]: """ - Generate SQL ALTER commands for attribute changes. + Find the attributes to drop, add and change between two definitions. Parameters ---------- - new : list[str] - New attribute SQL declarations. - old : list[str] - Old attribute SQL declarations. + new : dict[str, dict] + Columns of the new definition, ``name -> parts`` (see ``_column``), + in declaration order. + old : dict[str, dict] + Columns of the current definition, likewise. primary_key : list[str] Primary key attribute names (cannot be altered). adapter : DatabaseAdapter - Database adapter for backend-specific SQL generation. - comments : dict, optional - The new attributes' comments as declared, ``name -> comment``. A comment - that opens with ``{old_name}`` renames that attribute. + Database adapter, consulted on whether a column can be positioned. Returns ------- - list[str] - SQL ALTER commands (ADD, MODIFY, CHANGE, DROP). + tuple + Two-element tuple: + + - drop : list[str] - Names of the attributes to drop + - changes : list[dict] - One per attribute to add or change, in + declaration order, with keys ``new`` (its parts), ``old`` (the parts + of the column it changes, or None when it is added), + ``renamed_from`` (the name in its rename marker, or None) and + ``after`` (the attribute to place it after, or None to leave it). Raises ------ DataJointError If an attribute is renamed twice or renamed from non-existent attribute. """ - # parse attribute names - use adapter's quote character - quote_char = re.escape(adapter.quote_identifier("x")[0]) - name_regexp = re.compile(rf"^{quote_char}(?P\w+){quote_char}") - # The rename marker opens the comment as declared. A core type or codec puts - # its `:type:` marker in front of it, and PostgreSQL keeps comments out of the - # column SQL, so the marker is read from the declared comment, not the SQL. - original_regexp = re.compile(r"^(?::[^:]*:)?{\s*(?P\w+)\s*}") - comments = comments or {} - new_names = {} - for d in new: - name = name_regexp.match(d).group("name") - original = original_regexp.match(comments.get(name, "")) - new_names[name] = original and original.group("name") - old_names = [name_regexp.search(d).group("name") for d in old] + new_names = {name: column["renamed_from"] for name, column in new.items()} + old_names = list(old) + old_sql = [column["sql"] for column in old.values()] # verify that original names are only used once renamed = set() @@ -713,12 +785,13 @@ def _make_attribute_alter( # dropping attributes to_drop = [n for n in old_names if n not in renamed and n not in new_names] - sql = [f"DROP {adapter.quote_identifier(n)}" for n in to_drop] old_names = [name for name in old_names if name not in to_drop] # add or change attributes in order + changes = [] prev = None - for new_def, (new_name, old_name) in zip(new, new_names.items()): + for new_name, old_name in new_names.items(): + column = new[new_name] if new_name not in primary_key: after = None # if None, then must include the AFTER clause if prev: @@ -733,29 +806,14 @@ def _make_attribute_alter( # Without an AFTER clause a reorder-only change has nothing to # emit, so drop the position before it can force a statement. after = None - if new_def not in old or after: - # Determine command type - if (old_name or new_name) not in old_names: - command = "ADD" - elif not old_name: - command = "MODIFY" - elif adapter.backend != "mysql": - # Without this check the rename went unnoticed on PostgreSQL and - # became DROP + ADD, discarding the attribute's values. - raise DataJointError( - f"Renaming attribute `{old_name}` to `{new_name}` with alter() is not supported " - f"on {adapter.backend}. No change was made." - ) - else: - command = f"CHANGE {adapter.quote_identifier(old_name)}" - - # Build after clause - after_clause = "" if after is None else f"AFTER {adapter.quote_identifier(after)}" - - sql.append(f"{command} {new_def} {after_clause}") + source = old[old_name or new_name] if (old_name or new_name) in old_names else None + # The comment is compared on its own because PostgreSQL leaves it + # out of the column declaration. + if column["sql"] not in old_sql or after or (source is not None and source["comment"] != column["comment"]): + changes.append(dict(new=column, old=source, renamed_from=old_name, after=after)) prev = new_name, old_name - return sql + return to_drop, changes def alter( @@ -764,10 +822,10 @@ def alter( context: dict, adapter, *, - schema_name: str | None = None, -) -> tuple[list[str], list[str], list[str], dict]: + full_table_name: str, +) -> tuple[list[str], list[str], list[str]]: """ - Generate SQL ALTER commands for table definition changes. + Generate SQL statements for table definition changes. Parameters ---------- @@ -779,79 +837,82 @@ def alter( Namespace for resolving foreign key references. adapter : DatabaseAdapter Database adapter for backend-specific SQL generation. - schema_name : str, optional - Schema the table lives in. Required to collect pre-DDL for backends that - declare column types separately (PostgreSQL enums); omitting it yields an - empty ``pre_ddl``. + full_table_name : str + Quoted name of the table to alter. Its schema also qualifies the + pre-DDL of backends that declare column types separately (PostgreSQL + enums). Returns ------- tuple - Four-element tuple: + Three-element tuple: - - sql : list[str] - SQL ALTER commands + - sql : list[str] - Statements to run in order (see + ``DatabaseAdapter.alter_table_ddl``); empty when nothing changes - new_stores : list[str] - New external stores used - pre_ddl : list[str] - DDL to run before the ALTER (e.g. CREATE TYPE) - - column_comments : dict - Comments to reapply after the ALTER. On - backends that store them out of line these carry the ``:type:`` - prefix that ``heading`` reads back as ``original_type``, so skipping - them silently loses the declared type of an added attribute. Raises ------ NotImplementedError If attempting to alter primary key, foreign keys, or indexes. + DataJointError + If the backend cannot convert an attribute to its new type. """ + schema_name, _ = adapter.split_full_table_name(full_table_name) + columns = {} ( table_comment, primary_key, - attribute_sql, + _attribute_sql, foreign_key_sql, index_sql, external_stores, _fk_attribute_map, - column_comments, - ) = prepare_declare(definition, context, adapter) + _column_comments, + ) = prepare_declare(definition, context, adapter, columns=columns) # prepare_declare registers backend types (PostgreSQL enums) on the adapter # as a side effect, so each parse must be drained separately to tell the two # apart. Type names are content hashes, making the statements comparable. - new_type_ddl = adapter.get_pending_enum_ddl(schema_name) if schema_name else [] + new_type_ddl = adapter.get_pending_enum_ddl(schema_name) + old_columns = {} ( table_comment_, primary_key_, - attribute_sql_, + _attribute_sql_, foreign_key_sql_, index_sql_, external_stores_, _fk_attribute_map_, _column_comments_, - ) = prepare_declare(old_definition, context, adapter) + ) = prepare_declare(old_definition, context, adapter, columns=old_columns) # Whatever the old definition registered already exists in the database, so # only the difference needs creating. Draining both also leaves nothing # behind to leak into the next declare() on this adapter, including on the # NotImplementedError paths below. - old_type_ddl = set(adapter.get_pending_enum_ddl(schema_name)) if schema_name else set() + old_type_ddl = set(adapter.get_pending_enum_ddl(schema_name)) pre_ddl = [ddl for ddl in new_type_ddl if ddl not in old_type_ddl] # analyze differences between declarations - sql = list() if primary_key != primary_key_: raise NotImplementedError("table.alter cannot alter the primary key (yet).") if foreign_key_sql != foreign_key_sql_: raise NotImplementedError("table.alter cannot alter foreign keys (yet).") if index_sql != index_sql_: raise NotImplementedError("table.alter cannot alter indexes (yet)") - if attribute_sql != attribute_sql_: - sql.extend(_make_attribute_alter(attribute_sql, attribute_sql_, primary_key, adapter, column_comments)) - if table_comment != table_comment_: - # For MySQL: COMMENT="new comment" - # For PostgreSQL: would need COMMENT ON TABLE, but that's not an ALTER TABLE clause - # Keep MySQL syntax for now (ALTER TABLE ... COMMENT="...") - sql.append(f'COMMENT="{table_comment}"') - return sql, [e for e in external_stores if e not in external_stores_], pre_ddl, column_comments + drop, changes = [], [] + if list(columns.values()) != list(old_columns.values()): # a list, so that order counts + drop, changes = _make_attribute_alter(columns, old_columns, primary_key, adapter) + sql = adapter.alter_table_ddl( + full_table_name, + drop, + changes, + table_comment=table_comment if table_comment != table_comment_ else None, + ) + return sql, [e for e in external_stores if e not in external_stores_], pre_ddl def _parse_index_args(args: str) -> list[str]: @@ -989,7 +1050,7 @@ def substitute_special_type(match: dict, category: str, foreign_key_sql: list[st def compile_attribute( - line: str, in_key: bool, foreign_key_sql: list[str], context: dict, adapter + line: str, in_key: bool, foreign_key_sql: list[str], context: dict, adapter, *, column: dict | None = None ) -> tuple[str, str, str | None, str | None]: """ Convert an attribute definition from DataJoint format to SQL. @@ -1006,6 +1067,10 @@ def compile_attribute( Namespace for codec lookup. adapter : DatabaseAdapter Database adapter for backend-specific SQL generation. + column : dict, optional + Filled in place with the parts of the column: see ``_column``. + ``alter`` compares these rather than the compiled declarations, which + differ by backend (PostgreSQL keeps comments out of them). Returns ------- @@ -1036,9 +1101,12 @@ def compile_attribute( match = {k: v.strip() for k, v in match.items()} match["nullable"] = match["default"].lower() == "null" + # `default` is the SQL expression; match["default"] becomes the whole clause + default = None if match["nullable"]: if in_key: raise DataJointError('Primary key attributes cannot be nullable in line "%s"' % line) + default = "NULL" match["default"] = "DEFAULT NULL" # nullable attributes default to null else: if match["default"]: @@ -1047,17 +1115,18 @@ def compile_attribute( if base_val in CONSTANT_LITERALS: # SQL constants like NULL, CURRENT_TIMESTAMP - use as-is - match["default"] = f"NOT NULL DEFAULT {default_val}" + default = default_val elif default_val.startswith('"') and default_val.endswith('"'): # Double-quoted string - convert to single quotes for PostgreSQL inner = default_val[1:-1].replace("'", "''") # Escape single quotes - match["default"] = f"NOT NULL DEFAULT '{inner}'" + default = f"'{inner}'" elif default_val.startswith("'"): # Already single-quoted - use as-is - match["default"] = f"NOT NULL DEFAULT {default_val}" + default = default_val else: # Unquoted value - wrap in single quotes - match["default"] = f"NOT NULL DEFAULT '{default_val}'" + default = f"'{default_val}'" + match["default"] = f"NOT NULL DEFAULT {default}" else: match["default"] = "NOT NULL" @@ -1065,6 +1134,7 @@ def compile_attribute( if match["comment"].startswith(":"): raise DataJointError('An attribute comment must not start with a colon in comment "{comment}"'.format(**match)) + renamed_from = _renamed_from(match["comment"]) category = match_type(match["type"]) @@ -1103,4 +1173,7 @@ def compile_attribute( default=match["default"] if match["default"] else None, comment=match["comment"] if match["comment"] else None, ) - return match["name"], sql, match.get("store"), match["comment"] if match["comment"] else None + comment = match["comment"] if match["comment"] else None + if column is not None: + column.update(_column(match["name"], match["type"], match["nullable"], default, comment, sql, renamed_from)) + return match["name"], sql, match.get("store"), comment diff --git a/src/datajoint/table.py b/src/datajoint/table.py index 9f6784fb3..e570112ca 100644 --- a/src/datajoint/table.py +++ b/src/datajoint/table.py @@ -1,4 +1,5 @@ import collections +import contextlib import csv import inspect import itertools @@ -323,12 +324,13 @@ def alter(self, prompt=True, context=None): context = dict(frame.f_globals, **frame.f_locals) del frame old_definition = self.describe(context=context) - sql, _external_stores, pre_ddl, column_comments = alter( + adapter = self.connection.adapter + sql, _external_stores, pre_ddl = alter( self.definition, old_definition, context, - self.connection.adapter, - schema_name=self.database, + adapter, + full_table_name=self.full_table_name, ) if not sql: if prompt: @@ -336,13 +338,9 @@ def alter(self, prompt=True, context=None): else: # Same two steps declare() performs on its own output: substitute the # adapter's schema placeholder, and issue any pre-DDL the attribute - # types depend on. The attribute SQL is joined in after the format - # call, so it never passes through str.format. - sql = _substitute_database( - "ALTER TABLE {tab}\n\t".format(tab=self.full_table_name) + ",\n\t".join(sql), - self.database, - ) - if not prompt or user_choice(sql + "\n\nExecute?") == "yes": + # types depend on. + sql = [_substitute_database(statement, self.database) for statement in sql] + if not prompt or user_choice(";\n".join(sql) + "\n\nExecute?") == "yes": try: for ddl in pre_ddl: try: @@ -353,15 +351,11 @@ def alter(self, prompt=True, context=None): # type may already exist. Logged rather than dropped: # a genuine failure surfaces on the ALTER below. logger.debug("pre-DDL skipped (%s): %s", error, ddl) - self.connection.query(sql) - # Reapply comments. Where they are stored out of line they - # carry the `:type:` prefix heading reads back as - # original_type, without which describe() loses an added - # attribute's declared type and cannot re-parse the table. - for col_name, comment in column_comments.items(): - comment_ddl = self.connection.adapter.column_comment_ddl(self.full_table_name, col_name, comment) - if comment_ddl: - self.connection.query(_substitute_database(comment_ddl, self.database)) + # Where the change takes several statements (PostgreSQL), a + # failure in any of them leaves the table as it was. + with self.connection.transaction if adapter.supports_transactional_ddl else contextlib.nullcontext(): + for statement in sql: + self.connection.query(statement) except AccessError: # skip if no create privilege pass diff --git a/tests/integration/test_alter_attributes.py b/tests/integration/test_alter_attributes.py new file mode 100644 index 000000000..8684e9988 --- /dev/null +++ b/tests/integration/test_alter_attributes.py @@ -0,0 +1,404 @@ +""" +Integration tests for changing attributes with ``Table.alter()`` on both backends. + +Each test alters a table that holds data and checks that the table now describes +itself exactly as a table declared directly from the new definition does, that +``alter()`` then finds nothing left to change, and that the data survived. +MySQL makes every change with one ALTER TABLE statement; PostgreSQL uses +``ALTER COLUMN``, ``RENAME COLUMN`` and ``COMMENT ON`` statements inside one +transaction (#1625). +""" + +import itertools +import time + +import pytest + +import datajoint as dj +from datajoint.declare import alter +from datajoint.errors import DataJointError + + +REFERENCE_NUMBERS = itertools.count() + + +@pytest.fixture +def schema_by_backend(connection_by_backend, backend, prefix): + """A fresh schema on each backend, dropped afterward.""" + test_id = str(int(time.time() * 1000))[-8:] + schema = dj.Schema(f"{prefix}_alter_attr_{backend}_{test_id}", connection=connection_by_backend) + yield schema + schema.drop(prompt=False) + + +def declare_measurement(schema, definition): + class Measurement(dj.Manual): + pass + + Measurement.definition = definition + return schema(Measurement) + + +def alter_to(schema, table, definition): + """Alter ``table`` to ``definition`` and check that the result matches a direct declaration.""" + table.definition = definition + table.alter(prompt=False) + altered = table.describe(context={}) + + # The reference tables stay until the schema is dropped: on PostgreSQL, + # dropping one would also drop the enum types it shares with the table. + reference = type(f"Reference{next(REFERENCE_NUMBERS)}", (dj.Manual,), dict(definition=definition)) + assert altered == schema(reference)().describe(context={}) + + # describe() reads back what the definition declares, so there is nothing left to alter + statements, _, _ = alter(definition, altered, {}, schema.connection.adapter, full_table_name=table.full_table_name) + assert statements == [] + return altered + + +@pytest.mark.backend_agnostic +def test_change_type(schema_by_backend): + Measurement = declare_measurement( + schema_by_backend, + """ + measurement_id : int32 + --- + value : float32 # the value + count : int16 # a count + code : varchar(8) # a numeric code + """, + ) + Measurement.insert1(dict(measurement_id=1, value=2.5, count=3, code="12")) + + alter_to( + schema_by_backend, + Measurement, + """ + measurement_id : int32 + --- + value : float64 # the value + count : varchar(8) # a count + code : int32 # a numeric code + """, + ) + assert Measurement.heading["value"].original_type == "float64" + assert Measurement.fetch1() == dict(measurement_id=1, value=2.5, count="3", code=12) + + +@pytest.mark.backend_agnostic +def test_change_between_number_and_bool(schema_by_backend): + Measurement = declare_measurement( + schema_by_backend, + """ + measurement_id : int32 + --- + flag : int8 # a flag + """, + ) + Measurement.insert([dict(measurement_id=1, flag=1), dict(measurement_id=2, flag=0)]) + + alter_to( + schema_by_backend, + Measurement, + """ + measurement_id : int32 + --- + flag : bool # a flag + """, + ) + assert [bool(f) for f in Measurement.to_arrays("flag", order_by="measurement_id")] == [True, False] + + alter_to( + schema_by_backend, + Measurement, + """ + measurement_id : int32 + --- + flag : int32 # a flag + """, + ) + assert list(Measurement.to_arrays("flag", order_by="measurement_id")) == [1, 0] + + +@pytest.mark.backend_agnostic +def test_change_nullability(schema_by_backend): + Measurement = declare_measurement( + schema_by_backend, + """ + measurement_id : int32 + --- + value : float32 # the value + """, + ) + Measurement.insert1(dict(measurement_id=1, value=2.5)) + + alter_to( + schema_by_backend, + Measurement, + """ + measurement_id : int32 + --- + value = null : float32 # the value + """, + ) + Measurement.insert1(dict(measurement_id=2)) + assert (Measurement & dict(measurement_id=2)).fetch1("value") is None + + (Measurement & dict(measurement_id=2)).delete_quick() + alter_to( + schema_by_backend, + Measurement, + """ + measurement_id : int32 + --- + value : float32 # the value + """, + ) + assert Measurement.fetch1("value") == 2.5 + + +@pytest.mark.backend_agnostic +def test_change_default(schema_by_backend): + Measurement = declare_measurement( + schema_by_backend, + """ + measurement_id : int32 + --- + value : float32 # the value + label : varchar(16) # a label + """, + ) + Measurement.insert1(dict(measurement_id=1, value=2.5, label="first")) + + alter_to( + schema_by_backend, + Measurement, + """ + measurement_id : int32 + --- + value = 1.5 : float32 # the value + label = "it's unset" : varchar(16) # a label + """, + ) + Measurement.insert1(dict(measurement_id=2)) + assert (Measurement & dict(measurement_id=2)).fetch1("value", "label") == (1.5, "it's unset") + + alter_to( + schema_by_backend, + Measurement, + """ + measurement_id : int32 + --- + value : float32 # the value + label : varchar(16) # a label + """, + ) + assert Measurement.heading["value"].default is None + assert len(Measurement()) == 2 + + +@pytest.mark.backend_agnostic +def test_change_comment(schema_by_backend): + Measurement = declare_measurement( + schema_by_backend, + """ + measurement_id : int32 + --- + value : float32 # the value + """, + ) + Measurement.insert1(dict(measurement_id=1, value=2.5)) + + altered = alter_to( + schema_by_backend, + Measurement, + """ + measurement_id : int32 + --- + value : float32 # another comment + """, + ) + assert "# another comment" in altered + # the core-type marker is kept with the new comment + assert Measurement.heading["value"].original_type == "float32" + assert Measurement.fetch1("value") == 2.5 + + +@pytest.mark.backend_agnostic +def test_change_table_comment(schema_by_backend): + Measurement = declare_measurement( + schema_by_backend, + """ + measurement_id : int32 + --- + value : float32 # the value + """, + ) + altered = alter_to( + schema_by_backend, + Measurement, + """ + # measurements of the value + measurement_id : int32 + --- + value : float32 # the value + """, + ) + assert altered.startswith("# measurements of the value\n") + + +@pytest.mark.backend_agnostic +def test_combined_changes(schema_by_backend): + Measurement = declare_measurement( + schema_by_backend, + """ + measurement_id : int32 + --- + value : float32 # the value + label : varchar(16) # a label + obsolete = null : int32 # to be dropped + """, + ) + Measurement.insert1(dict(measurement_id=1, value=2.5, label="first")) + + alter_to( + schema_by_backend, + Measurement, + """ + measurement_id : int32 + --- + value = 0.5 : float64 # a wider value + label = null : varchar(32) # a longer label + added = null : int16 # an added attribute + """, + ) + assert Measurement.fetch1() == dict(measurement_id=1, value=2.5, label="first", added=None) + + +@pytest.mark.backend_agnostic +def test_rename(schema_by_backend): + Measurement = declare_measurement( + schema_by_backend, + """ + measurement_id : int32 + --- + value : float32 # the value + """, + ) + Measurement.insert1(dict(measurement_id=1, value=2.5)) + + alter_to( + schema_by_backend, + Measurement, + """ + measurement_id : int32 + --- + reading : float32 # {value} the value + """, + ) + assert Measurement.fetch1() == dict(measurement_id=1, reading=2.5) + + +@pytest.mark.backend_agnostic +def test_rename_and_change(schema_by_backend): + Measurement = declare_measurement( + schema_by_backend, + """ + measurement_id : int32 + --- + value = 1.5 : float32 # the value + """, + ) + Measurement.insert1(dict(measurement_id=1, value=2.5)) + + alter_to( + schema_by_backend, + Measurement, + """ + measurement_id : int32 + --- + reading = null : float64 # {value} the value, now optional + """, + ) + assert Measurement.fetch1() == dict(measurement_id=1, reading=2.5) + assert Measurement.heading["reading"].original_type == "float64" + + +@pytest.mark.backend_agnostic +def test_change_to_enum(schema_by_backend): + Measurement = declare_measurement( + schema_by_backend, + """ + measurement_id : int32 + --- + species = "mouse" : varchar(16) # the species + """, + ) + Measurement.insert([dict(measurement_id=1, species="mouse"), dict(measurement_id=2, species="rat")]) + + alter_to( + schema_by_backend, + Measurement, + """ + measurement_id : int32 + --- + species = "mouse" : enum('mouse', 'rat') # the species + """, + ) + alter_to( + schema_by_backend, + Measurement, + """ + measurement_id : int32 + --- + species = "mouse" : enum('mouse', 'rat', 'vole') # the species + """, + ) + Measurement.insert1(dict(measurement_id=3, species="vole")) + assert list(Measurement.to_arrays("species", order_by="measurement_id")) == ["mouse", "rat", "vole"] + + +@pytest.mark.backend_agnostic +def test_failed_change_leaves_table_unchanged(schema_by_backend): + definition = """ + measurement_id : int32 + --- + code : varchar(8) # a code + """ + Measurement = declare_measurement(schema_by_backend, definition) + Measurement.insert1(dict(measurement_id=1, code="abc")) + before = Measurement.describe(context={}) + + # "abc" is not a number; on PostgreSQL the rename runs before the type + # change, and the transaction undoes it. + Measurement.definition = """ + measurement_id : int32 + --- + number : int32 # {code} a code + """ + with pytest.raises(Exception): + Measurement.alter(prompt=False) + Measurement.definition = definition + assert Measurement.describe(context={}) == before + assert Measurement.fetch1() == dict(measurement_id=1, code="abc") + + +@pytest.mark.backend_agnostic +def test_unconvertible_type_is_refused(schema_by_backend, backend): + if backend != "postgresql": + pytest.skip("MySQL converts a date to a number") + definition = """ + measurement_id : int32 + --- + taken : date # when it was taken + """ + Measurement = declare_measurement(schema_by_backend, definition) + before = Measurement.describe(context={}) + Measurement.definition = """ + measurement_id : int32 + --- + taken : int32 # when it was taken + """ + with pytest.raises(DataJointError, match="cannot convert attribute `taken` from date to integer"): + Measurement.alter(prompt=False) + assert Measurement.describe(context={}) == before diff --git a/tests/integration/test_alter_rename.py b/tests/integration/test_alter_rename.py index cbf809b5b..ef44b315b 100644 --- a/tests/integration/test_alter_rename.py +++ b/tests/integration/test_alter_rename.py @@ -1,9 +1,8 @@ """ Renaming an attribute with ``alter()`` keeps its values. -A comment that opens with ``{old_name}`` renames that attribute. On MySQL the -rename is a CHANGE, which keeps the values; PostgreSQL refuses the rename -instead of dropping the attribute and adding an empty one. +A comment that opens with ``{old_name}`` renames that attribute: a CHANGE on +MySQL and a RENAME COLUMN on PostgreSQL, both of which keep the values. """ import time @@ -11,7 +10,6 @@ import pytest import datajoint as dj -from datajoint.errors import DataJointError @pytest.fixture(scope="function") @@ -43,17 +41,9 @@ class Measurement(dj.Manual): reading{default} : float32 # {{value}} the value """ - if db_creds_by_backend["backend"] == "mysql": - Measurement.alter(prompt=False) - assert Measurement.heading.names == ["measurement_id", "reading"] - assert Measurement.to_dicts(order_by="measurement_id") == [ - {"measurement_id": 1, "reading": 2.5}, - {"measurement_id": 2, "reading": 4.0}, - ] - else: - with pytest.raises(DataJointError, match="Renaming attribute `value` to `reading`"): - Measurement.alter(prompt=False) - assert dj.FreeTable(schema_by_backend.connection, Measurement.full_table_name).to_dicts(order_by="measurement_id") == [ - {"measurement_id": 1, "value": 2.5}, - {"measurement_id": 2, "value": 4.0}, - ] + Measurement.alter(prompt=False) + assert Measurement.heading.names == ["measurement_id", "reading"] + assert Measurement.to_dicts(order_by="measurement_id") == [ + {"measurement_id": 1, "reading": 2.5}, + {"measurement_id": 2, "reading": 4.0}, + ] diff --git a/tests/unit/test_alter_sql.py b/tests/unit/test_alter_sql.py new file mode 100644 index 000000000..72847cb12 --- /dev/null +++ b/tests/unit/test_alter_sql.py @@ -0,0 +1,410 @@ +""" +Unit tests for the statements ``alter()`` generates on each backend (#1625). + +The MySQL statements are pinned to what ``alter()`` produced before PostgreSQL +learned to change attributes, so that work leaves MySQL as it was. The one +exception is renaming an attribute of a core type, which used to drop the +column and add a new one. +""" + +import pytest + +from datajoint.adapters import get_adapter +from datajoint.declare import _renamed_from, alter +from datajoint.errors import DataJointError + +MYSQL_TABLE = "`djtest_alter`.`measurement`" +PG_TABLE = '"djtest_alter"."measurement"' + +BASE = """ +measurement_id : int32 +--- +value : float32 # the value +label : varchar(16) # a label +""" + +CHANGES = { + "type": """ + measurement_id : int32 + --- + value : float64 # the value + label : varchar(16) # a label + """, + "nullable": """ + measurement_id : int32 + --- + value = null : float32 # the value + label : varchar(16) # a label + """, + "default": """ + measurement_id : int32 + --- + value = 1.5 : float32 # the value + label = "none" : varchar(16) # a label + """, + "comment": """ + measurement_id : int32 + --- + value : float32 # another comment + label : varchar(16) # a label + """, + "combined": """ + measurement_id : int32 + --- + value = 2.5 : float64 # a wider value + label = null : varchar(32) # a longer label + """, + "add": """ + measurement_id : int32 + --- + value : float32 # the value + extra = null : int16 # an added attribute + label : varchar(16) # a label + """, + "drop": """ + measurement_id : int32 + --- + label : varchar(16) # a label + """, + "reorder": """ + measurement_id : int32 + --- + label : varchar(16) # a label + value : float32 # the value + """, + "table_comment": """ + # measurements + measurement_id : int32 + --- + value : float32 # the value + label : varchar(16) # a label + """, + "enum": """ + measurement_id : int32 + --- + value : float32 # the value + label : varchar(16) # a label + status = "active" : enum('active', 'retired') # status + """, + "rename": """ + measurement_id : int32 + --- + reading : float32 # {value} the value + label : varchar(16) # a label + """, + "rename_and_change": """ + measurement_id : int32 + --- + reading = null : float64 # {value} the value + label : varchar(16) # a label + """, +} + +DEFAULTED = """ +measurement_id : int32 +--- +value = 1.5 : float32 # the value +""" + +UNDEFAULTED = """ +measurement_id : int32 +--- +value : float32 # the value +""" + +NATIVE = """ +measurement_id : int32 +--- +value : float # the value +""" + +NATIVE_RENAMED = """ +measurement_id : int32 +--- +reading = null : double # {value} the value +""" + +# What alter() produced for MySQL before #1625, statement by statement. +MYSQL_BEFORE = { + "type": 'ALTER TABLE `djtest_alter`.`measurement`\n\tMODIFY `value` double NOT NULL COMMENT ":float64:the value" ', + "nullable": 'ALTER TABLE `djtest_alter`.`measurement`\n\tMODIFY `value` float DEFAULT NULL COMMENT ":float32:the value" ', + "default": ( + "ALTER TABLE `djtest_alter`.`measurement`\n" + "\tMODIFY `value` float NOT NULL DEFAULT '1.5' COMMENT \":float32:the value\" ,\n" + "\tMODIFY `label` varchar(16) NOT NULL DEFAULT 'none' COMMENT \":varchar(16):a label\" " + ), + "comment": 'ALTER TABLE `djtest_alter`.`measurement`\n\tMODIFY `value` float NOT NULL COMMENT ":float32:another comment" ', + "combined": ( + "ALTER TABLE `djtest_alter`.`measurement`\n" + "\tMODIFY `value` double NOT NULL DEFAULT '2.5' COMMENT \":float64:a wider value\" ,\n" + '\tMODIFY `label` varchar(32) DEFAULT NULL COMMENT ":varchar(32):a longer label" ' + ), + "add": ( + "ALTER TABLE `djtest_alter`.`measurement`\n" + '\tADD `extra` smallint DEFAULT NULL COMMENT ":int16:an added attribute" AFTER `value`,\n' + '\tMODIFY `label` varchar(16) NOT NULL COMMENT ":varchar(16):a label" AFTER `extra`' + ), + "drop": "ALTER TABLE `djtest_alter`.`measurement`\n\tDROP `value`", + "reorder": ( + "ALTER TABLE `djtest_alter`.`measurement`\n" + '\tMODIFY `label` varchar(16) NOT NULL COMMENT ":varchar(16):a label" AFTER `measurement_id`,\n' + '\tMODIFY `value` float NOT NULL COMMENT ":float32:the value" AFTER `label`' + ), + "table_comment": 'ALTER TABLE `djtest_alter`.`measurement`\n\tCOMMENT="measurements"', + "enum": ( + "ALTER TABLE `djtest_alter`.`measurement`\n" + "\tADD `status` enum('active', 'retired') NOT NULL DEFAULT 'active' " + "COMMENT \":enum('active', 'retired'):status\" AFTER `label`" + ), +} + + +@pytest.fixture(scope="module") +def mysql(): + return get_adapter("mysql") + + +@pytest.fixture(scope="module") +def postgres(): + pytest.importorskip("psycopg2") + return get_adapter("postgresql") + + +@pytest.mark.parametrize("change", sorted(MYSQL_BEFORE)) +def test_mysql_statement_unchanged(mysql, change): + sql, _, pre_ddl = alter(CHANGES[change], BASE, {}, mysql, full_table_name=MYSQL_TABLE) + assert sql == [MYSQL_BEFORE[change]] + assert pre_ddl == [] + + +def test_mysql_default_drop_unchanged(mysql): + sql, _, _ = alter(UNDEFAULTED, DEFAULTED, {}, mysql, full_table_name=MYSQL_TABLE) + assert sql == ['ALTER TABLE `djtest_alter`.`measurement`\n\tMODIFY `value` float NOT NULL COMMENT ":float32:the value" '] + + +def test_mysql_native_rename_unchanged(mysql): + sql, _, _ = alter(NATIVE_RENAMED, NATIVE, {}, mysql, full_table_name=MYSQL_TABLE) + assert sql == [ + 'ALTER TABLE `djtest_alter`.`measurement`\n\tCHANGE `value` `reading` double DEFAULT NULL COMMENT "{value} the value" ' + ] + + +def test_mysql_rename_of_core_type_keeps_the_column(mysql): + # Before #1625 the `:float32:` marker hid the rename, and this became + # DROP `value`, ADD `reading`: the values were lost. + sql, _, _ = alter(CHANGES["rename_and_change"], BASE, {}, mysql, full_table_name=MYSQL_TABLE) + assert sql == [ + "ALTER TABLE `djtest_alter`.`measurement`\n" + '\tCHANGE `value` `reading` double DEFAULT NULL COMMENT ":float64:{value} the value" ' + ] + + +def test_mysql_nothing_to_alter(mysql): + assert alter(BASE, BASE, {}, mysql, full_table_name=MYSQL_TABLE)[0] == [] + + +def comment(column, text): + escaped = text.replace("'", "''") + return f"COMMENT ON COLUMN {PG_TABLE}.\"{column}\" IS '{escaped}'" + + +PG_EXPECTED = { + "type": [ + f'ALTER TABLE {PG_TABLE}\n\tALTER COLUMN "value" TYPE double precision', + comment("value", ":float64:the value"), + ], + "nullable": [f'ALTER TABLE {PG_TABLE}\n\tALTER COLUMN "value" DROP NOT NULL'], + "default": [ + f"ALTER TABLE {PG_TABLE}\n\tALTER COLUMN \"value\" SET DEFAULT '1.5',\n\tALTER COLUMN \"label\" SET DEFAULT 'none'" + ], + "comment": [comment("value", ":float32:another comment")], + "combined": [ + f"ALTER TABLE {PG_TABLE}\n" + '\tALTER COLUMN "value" TYPE double precision,\n' + "\tALTER COLUMN \"value\" SET DEFAULT '2.5',\n" + '\tALTER COLUMN "label" TYPE varchar(32),\n' + '\tALTER COLUMN "label" DROP NOT NULL', + comment("value", ":float64:a wider value"), + comment("label", ":varchar(32):a longer label"), + ], + "add": [ + f'ALTER TABLE {PG_TABLE}\n\tADD COLUMN "extra" smallint DEFAULT NULL', + comment("extra", ":int16:an added attribute"), + ], + "drop": [f'ALTER TABLE {PG_TABLE}\n\tDROP COLUMN "value"'], + "reorder": [], # PostgreSQL cannot position columns + "table_comment": [f"COMMENT ON TABLE {PG_TABLE} IS 'measurements'"], + "rename": [ + f'ALTER TABLE {PG_TABLE} RENAME COLUMN "value" TO "reading"', + comment("reading", ":float32:{value} the value"), + ], + "rename_and_change": [ + f'ALTER TABLE {PG_TABLE} RENAME COLUMN "value" TO "reading"', + f'ALTER TABLE {PG_TABLE}\n\tALTER COLUMN "reading" TYPE double precision,\n\tALTER COLUMN "reading" DROP NOT NULL', + comment("reading", ":float64:{value} the value"), + ], +} + + +@pytest.mark.parametrize("change", sorted(PG_EXPECTED)) +def test_postgres_statements(postgres, change): + sql, _, pre_ddl = alter(CHANGES[change], BASE, {}, postgres, full_table_name=PG_TABLE) + assert sql == PG_EXPECTED[change] + assert pre_ddl == [] + + +def test_postgres_add_enum(postgres): + sql, _, pre_ddl = alter(CHANGES["enum"], BASE, {}, postgres, full_table_name=PG_TABLE) + (type_ddl,) = pre_ddl + assert type_ddl.startswith('CREATE TYPE "djtest_alter"."enum_') and type_ddl.endswith("AS ENUM ('active', 'retired')") + enum_type = '"{database}".' + type_ddl.split(".")[1].split()[0] + assert sql == [ + f"ALTER TABLE {PG_TABLE}\n\tADD COLUMN \"status\" {enum_type} NOT NULL DEFAULT 'active'", + comment("status", ":enum('active', 'retired'):status"), + ] + + +def test_postgres_drop_default(postgres): + sql, _, _ = alter(UNDEFAULTED, DEFAULTED, {}, postgres, full_table_name=PG_TABLE) + assert sql == [f'ALTER TABLE {PG_TABLE}\n\tALTER COLUMN "value" DROP DEFAULT'] + + +def test_postgres_type_change_resets_default(postgres): + """A default is dropped before the type changes, then set again.""" + old = """ + measurement_id : int32 + --- + code = "7" : varchar(8) # a code + """ + new = """ + measurement_id : int32 + --- + code = 7 : int32 # a code + """ + sql, _, _ = alter(new, old, {}, postgres, full_table_name=PG_TABLE) + assert sql == [ + f"ALTER TABLE {PG_TABLE}\n" + '\tALTER COLUMN "code" DROP DEFAULT,\n' + '\tALTER COLUMN "code" TYPE integer USING "code"::integer,\n' + "\tALTER COLUMN \"code\" SET DEFAULT '7'", + comment("code", ":int32:a code"), + ] + + +def test_postgres_change_to_enum(postgres): + old = """ + measurement_id : int32 + --- + species : varchar(16) # the species + """ + new = """ + measurement_id : int32 + --- + species : enum('mouse', 'rat') # the species + """ + sql, _, pre_ddl = alter(new, old, {}, postgres, full_table_name=PG_TABLE) + (type_ddl,) = pre_ddl + enum_type = '"{database}".' + type_ddl.split(".")[1].split()[0] + assert sql == [ + f'ALTER TABLE {PG_TABLE}\n\tALTER COLUMN "species" TYPE {enum_type} USING "species"::text::{enum_type}', + comment("species", ":enum('mouse', 'rat'):the species"), + ] + + +def test_postgres_unconvertible_type_is_refused(postgres): + old = """ + measurement_id : int32 + --- + taken : date # when it was taken + """ + new = """ + measurement_id : int32 + --- + taken : int32 # when it was taken + """ + with pytest.raises(DataJointError, match="cannot convert attribute `taken` from date to integer"): + alter(new, old, {}, postgres, full_table_name=PG_TABLE) + + +ENUM = '"{database}"."enum_0123abcd"' + + +@pytest.mark.parametrize( + "old_type, new_type, expected", + [ + # an assignment conversion exists + ("real", "double precision", None), + ("integer", "smallint", None), + ("double precision", "numeric(8,2)", None), + ("varchar(16)", "varchar(64)", None), + ("varchar(64)", "char(8)", None), + ("integer", "varchar(8)", None), + ("timestamp", "varchar(32)", None), + ("boolean", "varchar(8)", None), + (ENUM, "varchar(16)", None), + ("date", "timestamp(3)", None), + ("timestamp", "date", None), + # an explicit conversion is needed + ("varchar(8)", "integer", '"x"::integer'), + ("varchar(8)", "boolean", '"x"::boolean'), + ("varchar(36)", "uuid", '"x"::uuid'), + ("varchar(32)", "date", '"x"::date'), + ("varchar(64)", "jsonb", '"x"::jsonb'), + ("varchar(16)", ENUM, f'"x"::text::{ENUM}'), + ('"other"."enum_99999999"', ENUM, f'"x"::text::{ENUM}'), + (ENUM, "integer", '"x"::text::integer'), + ("smallint", "boolean", '"x" <> 0'), + ("boolean", "bigint", '"x"::integer'), + ("jsonb", "double precision", '"x"::double precision'), + ("integer", "jsonb", 'to_jsonb("x")'), + # a type this conversion does not know: left to PostgreSQL + ("interval", "time", None), + ], +) +def test_postgres_type_conversion(postgres, old_type, new_type, expected): + assert postgres._type_conversion("x", old_type, new_type) == expected + + +@pytest.mark.parametrize( + "old_type, new_type", + [("date", "integer"), ("integer", "timestamp"), ("uuid", "bigint"), ("bytea", "uuid"), ("boolean", "date")], +) +def test_postgres_type_conversion_refused(postgres, old_type, new_type): + with pytest.raises(DataJointError, match="cannot convert"): + postgres._type_conversion("x", old_type, new_type) + + +@pytest.mark.parametrize( + "reported, default", + [ + ("'1.5'::real", "1.5"), + ("'abc'::character varying", "abc"), + ("'it''s'::character varying", "it's"), + ("''::character varying", ""), + ("'a'::\"some_schema\".enum_0123abcd", "a"), + ("'2020-01-01 00:00:00'::timestamp without time zone", "2020-01-01 00:00:00"), + ("5", "5"), + ("true", "true"), + ("CURRENT_TIMESTAMP", "CURRENT_TIMESTAMP"), + ("nextval('t_id_seq'::regclass)", "nextval('t_id_seq'::regclass)"), + (None, None), + ], +) +def test_postgres_reports_default_without_cast(postgres, reported, default): + row = dict(column_name="x", data_type="text", udt_name="text", is_nullable="NO", column_default=reported) + assert postgres.parse_column_info(row)["default"] == default + + +@pytest.mark.parametrize( + "comment, name", + [ + ("{value} the value", "value"), + ("{ value } the value", "value"), + ("{old_name}", "old_name"), + ("the value {value}", None), + ("the value", None), + ("", None), + ], +) +def test_renamed_from(comment, name): + assert _renamed_from(comment) == name