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