Repository: cogent3/cogent3 (python)

Issue:
Refactor AnnotationDb to not use SQL keywords for column names
`end` is a SQL keyword but has been embedded in the Sqlite3 based AnnotationDb classes, without raising any errors.

We should convert all methods on the `AnnotationDb` classes / subclasses to replace `end` with `stop`. If they method is public, the argument will need to be deprecated (use the `cogent3.util.warning.deprecate_argument` decorator).
On loading a db from disk, if a column named `end` exists it should be renamed as `stop`.

Pull request:
Refactor AnnotationDb to not use SQL keywords for column names, fixes #1750
Fixes #1750.

Replaces `end` as a column name with `stop` and deprecates related arguments.

On loading a database, columns named `end` are replaced with `stop` (though in conjunction with #1777 old databases would not be able to be loaded due to the new numpy format - maybe that's something that needs to be added to that PR?).



Patch:
diff --git a/src/cogent3/core/alignment.py b/src/cogent3/core/alignment.py
index 88072defa..4e8860f78 100644
--- a/src/cogent3/core/alignment.py
+++ b/src/cogent3/core/alignment.py
@@ -18,6 +18,7 @@
     passed in a stream of two-item label, sequence pairs. However, this can
     cause confusion when testing.
 """
+
 from __future__ import annotations
 
 import functools
@@ -5289,7 +5290,7 @@ class Alignment(AlignmentI, SequenceCollection):
             seqname = seqid_to_seqname[seqid]
             seq = self.named_seqs[seqname]
             # we use parent seqid, stored on SeqView
-            parent_id, start, end, _ = seq.data.parent_coordinates()
+            parent_id, start, stop, _ = seq.data.parent_coordinates()
             offset = seq.data.annotation_offset
 
             for feature in self.annotation_db.get_features_matching(
@@ -5299,7 +5300,7 @@ class Alignment(AlignmentI, SequenceCollection):
                 on_alignment=False,
                 allow_partial=allow_partial,
                 start=start,
-                end=end,
+                stop=stop,
             ):
                 if offset:
                     feature["spans"] = (array(feature["spans"]) - offset).tolist()
diff --git a/src/cogent3/core/annotation.py b/src/cogent3/core/annotation.py
index 5f8a4270b..6d7ee6811 100644
--- a/src/cogent3/core/annotation.py
+++ b/src/cogent3/core/annotation.py
@@ -163,7 +163,7 @@ class Feature:
         """generator returns sub-features of self optionally matching biotype"""
         offset = getattr(self.parent, "annotation_offset", 0)
         start = self.map.start + offset
-        end = self.map.end + offset
+        stop = self.map.end + offset
 
         make_feature = self.parent.make_feature
         db = self.parent.annotation_db
@@ -171,7 +171,7 @@ class Feature:
             biotype=biotype,
             name=self.name,
             start=start,
-            end=end,
+            stop=stop,
             **kwargs,
         ):
             record["spans"] = array(record["spans"]) - offset
@@ -187,14 +187,14 @@ class Feature:
         """generator returns parent features of self optionally matching biotype"""
         offset = getattr(self.parent, "annotation_offset", 0)
         start = self.map.start + offset
-        end = self.map.end + offset
+        stop = self.map.end + offset
 
         make_feature = self.parent.make_feature
         db = self.parent.annotation_db
         for record in db.get_feature_parent(
             name=self.name,
             start=start,
-            end=end,
+            stop=stop,
             **kwargs,
         ):
             record["spans"] = array(record["spans"]) - offset
diff --git a/src/cogent3/core/annotation_db.py b/src/cogent3/core/annotation_db.py
index 9e05b2e7f..bfaf6d991 100644
--- a/src/cogent3/core/annotation_db.py
+++ b/src/cogent3/core/annotation_db.py
@@ -14,6 +14,8 @@ import typing
 import numpy
 import typing_extensions
 
+import cogent3.util.warning as c3warn
+
 from cogent3._version import __version__
 from cogent3.util.deserialise import register_deserialiser
 from cogent3.util.misc import extend_docstring_from, get_object_provenance
@@ -85,6 +87,11 @@ class SerialisableType(typing.Protocol):  # pragma: no cover
 @typing.runtime_checkable
 class SupportsQueryFeatures(typing.Protocol):  # pragma: no cover
     # should be defined centrally
+    @c3warn.deprecated_args(
+        "2024.9",
+        reason="replace usage of SQL keyword as column name",
+        old_new=[("end", "stop")],
+    )
     def get_features_matching(
         self,
         *,
@@ -92,20 +99,40 @@ class SupportsQueryFeatures(typing.Protocol):  # pragma: no cover
         biotype: OptionalStr = None,
         name: OptionalStr = None,
         start: OptionalInt = None,
-        end: OptionalInt = None,
+        stop: OptionalInt = None,
         strand: OptionalStr = None,
         attributes: OptionalStr = None,
         on_alignment: OptionalBool = None,
     ) -> typing.Iterator[FeatureDataType]:
         ...
 
+    @c3warn.deprecated_args(
+        "2024.9",
+        reason="replace usage of SQL keyword as column name",
+        old_new=[("end", "stop")],
+    )
     def get_feature_children(
-        self, *, name: str, start: OptionalInt = None, end: OptionalInt = None, **kwargs
+        self,
+        *,
+        name: str,
+        start: OptionalInt = None,
+        stop: OptionalInt = None,
+        **kwargs,
     ) -> typing.List[FeatureDataType]:
         ...
 
+    @c3warn.deprecated_args(
+        "2024.9",
+        reason="replace usage of SQL keyword as column name",
+        old_new=[("end", "stop")],
+    )
     def get_feature_parent(
-        self, *, name: str, start: OptionalInt = None, end: OptionalInt = None, **kwargs
+        self,
+        *,
+        name: str,
+        start: OptionalInt = None,
+        stop: OptionalInt = None,
+        **kwargs,
     ) -> typing.List[FeatureDataType]:
         ...
 
@@ -121,6 +148,11 @@ class SupportsQueryFeatures(typing.Protocol):  # pragma: no cover
     ) -> int:
         ...
 
+    @c3warn.deprecated_args(
+        "2024.9",
+        reason="replace usage of SQL keyword as column name",
+        old_new=[("end", "stop")],
+    )
     def subset(
         self,
         *,
@@ -128,7 +160,7 @@ class SupportsQueryFeatures(typing.Protocol):  # pragma: no cover
         biotype: OptionalStr = None,
         name: OptionalStr = None,
         start: OptionalInt = None,
-        end: OptionalInt = None,
+        stop: OptionalInt = None,
         strand: OptionalStr = None,
         attributes: OptionalStr = None,
     ):
@@ -247,8 +279,8 @@ def _matching_conditions(
     conditions : dict
         column name and values to be matched
     allow_partial : bool, optional
-        if False, only records within start, end are included. If True,
-        all records that overlap the segment defined by start, end are included.
+        if False, only records within start, stop are included. If True,
+        all records that overlap the segment defined by start, stop are included.
 
     Returns
     -------
@@ -256,7 +288,7 @@ def _matching_conditions(
         the SQL statement and the tuple of values
     """
     start = conditions.pop("start", None)
-    end = conditions.pop("end", None)
+    stop = conditions.pop("stop", None)
 
     sql = []
     vals = ()
@@ -278,27 +310,27 @@ def _matching_conditions(
         sql.append(" AND ".join(conds))
         vals = tuple(vals)
 
-    if isinstance(start, int) and isinstance(end, int):
+    if isinstance(start, int) and isinstance(stop, int):
         if allow_partial:
             # allow matches that overlap the segment
             cond = [
-                f"(start >= {start} AND end <= {end})",  # lies within the segment
-                f"(start <= {start} AND end > {start})",  # straddles beginning of segment
-                f"(start < {end} AND end >= {end})",  # straddles end of segment
-                f"(start <= {start} AND end >= {end})",  # includes segment
+                f"(start >= {start} AND stop <= {stop})",  # lies within the segment
+                f"(start <= {start} AND stop > {start})",  # straddles beginning of segment
+                f"(start < {stop} AND stop >= {stop})",  # straddles stop of segment
+                f"(start <= {start} AND stop >= {stop})",  # includes segment
             ]
             cond = " OR ".join(cond)
         else:
             # only matches within bounds
-            cond = f"start >= {start} AND end <= {end}"
+            cond = f"start >= {start} AND stop <= {stop}"
         sql.append(f"({cond})")
     elif isinstance(start, int):
-        # if query has no end, then any feature containing start
-        cond = f"(start <= {start} AND {start} < end)"
+        # if query has no stop, then any feature containing start
+        cond = f"(start <= {start} AND {start} < stop)"
         sql.append(f"({cond})")
-    elif isinstance(end, int):
-        # if query has no start, then any feature containing end
-        cond = f"(start <= {end} AND {end} < end)"
+    elif isinstance(stop, int):
+        # if query has no start, then any feature containing stop
+        cond = f"(start <= {stop} AND {stop} < stop)"
         sql.append(f"({cond})")
 
     sql = f"{' AND '.join(sql)}"
@@ -309,7 +341,7 @@ def _del_records_sql(
     table_name: str,
     conditions: dict,
     start: OptionalInt = None,
-    end: OptionalInt = None,
+    stop: OptionalInt = None,
     allow_partial=True,
 ) -> ReturnType:
     """creates the SQL and values for identifying records to be deleted
@@ -320,12 +352,12 @@ def _del_records_sql(
         table to have records deleted from
     conditions : dict
         column name and values to be matched
-    start, end : OptionalInt
-        select records whose (start, end) values lie between start and end,
+    start, stop : OptionalInt
+        select records whose (start, stop) values lie between start and stop,
         or overlap them if (allow_partial is True)
     allow_partial : bool, optional
-        if False, only records within start, end are included. If True,
-        all records that overlap the segment defined by start, end are included.
+        if False, only records within start, stop are included. If True,
+        all records that overlap the segment defined by start, stop are included.
 
     Returns
     -------
@@ -333,7 +365,7 @@ def _del_records_sql(
         the SQL statement and the tuple of values
     """
     where, vals = _matching_conditions(
-        conditions=conditions, start=start, end=end, allow_partial=allow_partial
+        conditions=conditions, start=start, stop=stop, allow_partial=allow_partial
     )
     sql = f"DELETE FROM {table_name}"
     if not where:
@@ -348,7 +380,7 @@ def _select_records_sql(
     conditions: dict,
     columns: typing.Optional[typing.List[str]] = None,
     start: OptionalInt = None,
-    end: OptionalInt = None,
+    stop: OptionalInt = None,
     allow_partial=True,
 ) -> ReturnType:
     """create SQL select statement and values
@@ -361,12 +393,12 @@ def _select_records_sql(
         values to select
     conditions : dict
         the WHERE conditions
-    start, end : OptionalInt
-        select records whose (start, end) values lie between start and end,
+    start, stop : OptionalInt
+        select records whose (start, stop) values lie between start and stop,
         or overlap them if (allow_partial is True)
     allow_partial : bool, optional
-        if False, only records within start, end are included. If True,
-        all records that overlap the segment defined by start, end are included.
+        if False, only records within start, stop are included. If True,
+        all records that overlap the segment defined by start, stop are included.
 
     Returns
     -------
@@ -391,7 +423,7 @@ def _count_records_sql(
     conditions: dict,
     columns: typing.Optional[typing.List[str]] = None,
     start: OptionalInt = None,
-    end: OptionalInt = None,
+    stop: OptionalInt = None,
     allow_partial=True,
 ) -> ReturnType:
     """create SQL count statement and values
@@ -404,12 +436,12 @@ def _count_records_sql(
         values to select
     conditions : dict
         the WHERE conditions
-    start, end : OptionalInt
-        select records whose (start, end) values lie between start and end,
+    start, stop : OptionalInt
+        select records whose (start, stop) values lie between start and stop,
         or overlap them if (allow_partial is True)
     allow_partial : bool, optional
-        if False, only records within start, end are included. If True,
-        all records that overlap the segment defined by start, end are included.
+        if False, only records within start, stop are included. If True,
+        all records that overlap the segment defined by start, stop are included.
 
     Returns
     -------
@@ -448,6 +480,38 @@ def _compatible_schema(db: sqlite3.Connection, schema: dict[str, set]) -> bool:
     return True
 
 
+def _rename_column_if_exists(
+    db: sqlite3.Connection, table_name: str, old_column: str, new_column: str
+) -> None:
+    """Rename a column in a sqlite3 database only if it exists.
+
+    Parameters
+    ----------
+    db : sqlite3.Connection
+        the sqlite3 connection
+    table_name : str
+        the table name
+    old_column : str
+        the column to rename if it exists
+    new_column : str
+        the new name of old_column
+    """
+    cur = db.cursor()
+    table_info = cur.execute(f"PRAGMA table_info({table_name});").fetchall()
+
+    # Check if the column exists
+    for column_info in table_info:
+        if column_info["name"] == old_column:
+            break
+    else:
+        return  # There is no column to rename
+
+    sql = f'ALTER TABLE {table_name} RENAME COLUMN "{old_column}" TO {new_column}'
+
+    cur.execute(sql)
+    db.commit()
+
+
 class SqliteAnnotationDbMixin:
     # table schema for user provided annotations
     _user_schema = {
@@ -456,7 +520,7 @@ class SqliteAnnotationDbMixin:
         "name": "TEXT",
         "parent_id": "TEXT",
         "start": "INTEGER",
-        "end": "INTEGER",
+        "stop": "INTEGER",
         "strand": "TEXT",
         "spans": "array",
         "attributes": "TEXT",
@@ -578,6 +642,9 @@ class SqliteAnnotationDbMixin:
             )
             self._db.row_factory = sqlite3.Row
 
+            for table_name in self.table_names:
+                _rename_column_if_exists(self._db, table_name, "end", "stop")
+
         return self._db
 
     def _execute_sql(self, cmnd, values=None):
@@ -619,10 +686,10 @@ class SqliteAnnotationDbMixin:
             whether the annotation is an alignment annotation
         """
         spans = numpy.array(sorted(sorted(coords) for coords in spans), dtype=int)
-        # the start, end variables are added into the record via the loop
+        # the start, stop variables are added into the record via the loop
         # on local variables
         start = int(spans.min())
-        end = int(spans.max())
+        stop = int(spans.max())
         # we define record as all defined variables from local name space,
         # excluding "self"
         record = {k: v for k, v in locals().items() if k != "self" and v is not None}
@@ -652,7 +719,7 @@ class SqliteAnnotationDbMixin:
         column: str,
         name: str,
         start: OptionalInt = None,
-        end: OptionalInt = None,
+        stop: OptionalInt = None,
         biotype: OptionalStr = None,
         allow_partial: bool = False,
     ) -> typing.List[FeatureDataType]:
@@ -662,7 +729,7 @@ class SqliteAnnotationDbMixin:
             conditions={column: name, "biotype": biotype},
             columns=columns,
             start=start,
-            end=end,
+            stop=stop,
             allow_partial=allow_partial,
         )
         for result in self._execute_sql(sql, values=vals):
@@ -675,7 +742,7 @@ class SqliteAnnotationDbMixin:
         self, name: str, biotype: OptionalStr = None, **kwargs
     ) -> typing.Iterator[FeatureDataType]:
         """yields children of name"""
-        # kwargs is used because other classes need start / end
+        # kwargs is used because other classes need start / stop
         # just uses search matching
         for table_name in self.table_names:
             columns = "seqid", "biotype", "spans", "strand", "name", "parent_id"
@@ -727,6 +794,11 @@ class SqliteAnnotationDbMixin:
                         )  # remove invalid field for the FeatureDataType
                         yield parent
 
+    @c3warn.deprecated_args(
+        "2024.9",
+        reason="replace usage of SQL keyword as column name",
+        old_new=[("end", "stop")],
+    )
     def get_records_matching(
         self,
         *,
@@ -734,7 +806,7 @@ class SqliteAnnotationDbMixin:
         seqid: str = None,
         name: str = None,
         start: int = None,
-        end: int = None,
+        stop: int = None,
         strand: OptionalStr = None,
         attributes: OptionalStr = None,
         on_alignment: bool = None,
@@ -751,6 +823,11 @@ class SqliteAnnotationDbMixin:
             for result in self._get_records_matching(table_name, **kwargs):
                 yield dict(zip(result.keys(), result))
 
+    @c3warn.deprecated_args(
+        "2024.9",
+        reason="replace usage of SQL keyword as column name",
+        old_new=[("end", "stop")],
+    )
     def get_features_matching(
         self,
         *,
@@ -758,7 +835,7 @@ class SqliteAnnotationDbMixin:
         seqid: str = None,
         name: str = None,
         start: int = None,
-        end: int = None,
+        stop: int = None,
         strand: OptionalStr = None,
         attributes: OptionalStr = None,
         on_alignment: bool = None,
@@ -1041,6 +1118,11 @@ class SqliteAnnotationDbMixin:
             self.db.backup(backup)
         backup.close()
 
+    @c3warn.deprecated_args(
+        "2024.9",
+        reason="replace usage of SQL keyword as column name",
+        old_new=[("end", "stop")],
+    )
     def subset(
         self,
         *,
@@ -1049,7 +1131,7 @@ class SqliteAnnotationDbMixin:
         seqid: str = None,
         name: str = None,
         start: OptionalInt = None,
-        end: OptionalInt = None,
+        stop: OptionalInt = None,
         strand: OptionalStr = None,
         attributes: OptionalStr = None,
         allow_partial: bool = False,
@@ -1057,7 +1139,7 @@ class SqliteAnnotationDbMixin:
         """returns a new db instance with records matching the provided conditions"""
         # make sure python, not numpy, integers
         start = start if start is None else int(start)
-        end = end if end is None else int(end)
+        stop = stop if stop is None else int(stop)
 
         kwargs = {k: v for k, v in locals().items() if k not in {"self", "source"}}
 
@@ -1143,7 +1225,7 @@ class GffAnnotationDb(SqliteAnnotationDbMixin):
         "source": "TEXT",
         "biotype": "TEXT",  # type in GFF
         "start": "INTEGER",
-        "end": "INTEGER",
+        "stop": "INTEGER",
         "score": "TEXT",  # check defn
         "strand": "TEXT",
         "phase": "TEXT",
@@ -1182,12 +1264,12 @@ class GffAnnotationDb(SqliteAnnotationDbMixin):
         # required
         rows = []
         for record in reduced.values():
-            # our Feature code assumes start always < end,
+            # our Feature code assumes start always < stop,
             # we record direction using Strand
             spans = numpy.array(sorted(record["spans"]), dtype=int)  # sorts the rows
             spans.sort(axis=1)
             record["start"] = int(spans.min())
-            record["end"] = int(spans.max())
+            record["stop"] = int(spans.max())
             record["spans"] = spans
             rows.append(tuple(record.get(c) for c in col_order))
 
@@ -1211,6 +1293,7 @@ class GffAnnotationDb(SqliteAnnotationDbMixin):
         # record.
         for record in records:
             record["biotype"] = record.pop("Type")
+            record["stop"] = record.pop("End")
 
             # we force all keys that map to table column names to be lower case
             for key in tuple(record):
@@ -1234,7 +1317,7 @@ class GffAnnotationDb(SqliteAnnotationDbMixin):
                 reduced[record_id]["spans"] = []
 
             # should this just be an append?
-            reduced[record_id]["spans"].append((record["start"], record["end"]))
+            reduced[record_id]["spans"].append((record["start"], record["stop"]))
 
         del records
 
@@ -1267,7 +1350,7 @@ class GenbankAnnotationDb(SqliteAnnotationDbMixin):
         "source": "TEXT",
         "biotype": "TEXT",  # type in GFF
         "start": "INTEGER",
-        "end": "INTEGER",
+        "stop": "INTEGER",
         "strand": "TEXT",
         "comments": "TEXT",
         "spans": "array",  # aggregation of coords across records
@@ -1318,7 +1401,7 @@ class GenbankAnnotationDb(SqliteAnnotationDbMixin):
         rows = []
         exclude = {"translation", "location", "type"}  # location is grabbed directly
         for record in records:
-            # our Feature code assumes start always < end,
+            # our Feature code assumes start always < stop,
             store = {"seqid": seqid}
             # we create the location data directly
             if location := record.get("location", None):
@@ -1326,7 +1409,7 @@ class GenbankAnnotationDb(SqliteAnnotationDbMixin):
                 if strand := location.strand:
                     store["strand"] = "-" if strand == -1 else "+"
                 store["start"] = int(store["spans"].min())
-                store["end"] = int(store["spans"].max())
+                store["stop"] = int(store["spans"].max())
 
             attrs_keys = record.keys() - exclude
             store["biotype"] = record["type"]
@@ -1367,17 +1450,22 @@ class GenbankAnnotationDb(SqliteAnnotationDbMixin):
         self._num_fakeids += 1
         return name
 
+    @c3warn.deprecated_args(
+        "2024.9",
+        reason="replace usage of SQL keyword as column name",
+        old_new=[("end", "stop")],
+    )
     def get_feature_children(
         self,
         name: str,
         biotype: OptionalStr = None,
         exclude_biotype: OptionalStr = None,
         start: OptionalInt = None,
-        end: OptionalInt = None,
+        stop: OptionalInt = None,
     ) -> typing.Iterator[FeatureDataType]:
         """yields children of name"""
         # children must lie within the parent coordinates
-        if start is None or end is None:
+        if start is None or stop is None:
             name = self.__class__.__name__
             msg = f"coordinates required to query children for {name!r}"
             raise ValueError(msg)
@@ -1391,7 +1479,7 @@ class GenbankAnnotationDb(SqliteAnnotationDbMixin):
                 "name",
                 "parent_id",
                 "start",
-                "end",
+                "stop",
             )
             if table_name == "user":
                 columns += ("on_alignment",)
@@ -1403,27 +1491,32 @@ class GenbankAnnotationDb(SqliteAnnotationDbMixin):
                 name=name,
                 biotype=biotype,
                 start=start,
-                end=end,
+                stop=stop,
                 allow_partial=False,
             ):
                 if feat["biotype"] == exclude_biotype:
                     continue
-                cstart, cend = feat.pop("start"), feat.pop("end")
-                if not (start <= cstart < end and start < cend <= end):
+                cstart, cstop = feat.pop("start"), feat.pop("stop")
+                if not (start <= cstart < stop and start < cstop <= stop):
                     continue
 
                 feat.pop("parent_id")  # remove invalid field for the FeatureDataType
                 yield feat
 
+    @c3warn.deprecated_args(
+        "2024.9",
+        reason="replace usage of SQL keyword as column name",
+        old_new=[("end", "stop")],
+    )
     def get_feature_parent(
         self,
         name: str,
         exclude_biotype: OptionalStr = None,
         start: OptionalInt = None,
-        end: OptionalInt = None,
+        stop: OptionalInt = None,
     ) -> typing.Iterator[FeatureDataType]:
         """yields parents of name"""
-        if start is None or end is None:
+        if start is None or stop is None:
             name = self.__class__.__name__
             msg = f"coordinates required to query parent for {name!r}"
             raise ValueError(msg)
@@ -1438,7 +1531,7 @@ class GenbankAnnotationDb(SqliteAnnotationDbMixin):
                 "name",
                 "parent_id",
                 "start",
-                "end",
+                "stop",
             )
             if table_name == "user":
                 columns += ("on_alignment",)
@@ -1448,15 +1541,15 @@ class GenbankAnnotationDb(SqliteAnnotationDbMixin):
                 column="name",
                 name=name,
                 start=start,
-                end=end,
+                stop=stop,
                 allow_partial=False,
             ):
                 # add support for != operation to SQL where clause generation
                 if feat["biotype"] == exclude_biotype:
                     continue
 
-                cstart, cend = feat.pop("start"), feat.pop("end")
-                if cstart > start or end > cend:
+                cstart, cstop = feat.pop("start"), feat.pop("stop")
+                if cstart > start or stop > cstop:
                     continue
 
                 feat.pop("parent_id")  # remove invalid field for the FeatureDataType
diff --git a/src/cogent3/core/sequence.py b/src/cogent3/core/sequence.py
index 14279b9dd..ebe4870c3 100644
--- a/src/cogent3/core/sequence.py
+++ b/src/cogent3/core/sequence.py
@@ -10,6 +10,7 @@ Sequences are intended to be immutable. This is not enforced by the code for
 performance reasons, but don't alter the MolType or the sequence data after
 creation.
 """
+
 from __future__ import annotations
 
 import contextlib
@@ -920,7 +921,7 @@ class Sequence(SequenceI):
         name
             name of the feature
         start, stop
-            start, end positions to search between, relative to offset
+            start, stop positions to search between, relative to offset
             of this sequence. If not provided, entire span of sequence is used.
 
         Notes
@@ -955,11 +956,11 @@ class Sequence(SequenceI):
         query_start = self._seq.absolute_position(start, include_boundary=False)
         # we set include_boundary=True because stop is exclusive indexing,
         # i,e., the stop can be equal to the length of the view
-        query_end = self._seq.absolute_position(stop, include_boundary=True)
+        query_stop = self._seq.absolute_position(stop, include_boundary=True)
 
         rev_strand = strand == -1
         if rev_strand:
-            query_start, query_end = query_end, query_start
+            query_start, query_stop = query_stop, query_start
 
         query_start = max(query_start, 0)
         # in the underlying db, features are always plus strand oriented
@@ -983,7 +984,7 @@ class Sequence(SequenceI):
             name=name,
             biotype=biotype,
             start=query_start,
-            end=query_end,  # todo gah end should be stop
+            stop=query_stop,
             allow_partial=allow_partial,
         ):
             # spans need to be converted from absolute to relative positions


Test patch:
diff --git a/tests/test_core/test_annotation_db.py b/tests/test_core/test_annotation_db.py
index 28e45da99..858aa0675 100644
--- a/tests/test_core/test_annotation_db.py
+++ b/tests/test_core/test_annotation_db.py
@@ -8,6 +8,7 @@ from cogent3.core.annotation_db import (
     GffAnnotationDb,
     SupportsFeatures,
     _matching_conditions,
+    _rename_column_if_exists,
     load_annotations,
 )
 from cogent3.core.sequence import Sequence
@@ -270,7 +271,7 @@ def test_empty_data():
 
 
 # testing GenBank files
-@pytest.fixture(scope="session")
+@pytest.fixture(scope="function")
 def gb_db(DATA_DIR):
     return load_annotations(path=DATA_DIR / "annotated_seq.gb")
 
@@ -292,7 +293,7 @@ def test_gb_get_children(gb_db, parent_biotype, name):
             name=name,
             exclude_biotype=parent_biotype,
             start=coords.min(),
-            end=coords.max(),
+            stop=coords.max(),
         )
     )[0]
     assert child["biotype"] != parent["biotype"]
@@ -308,7 +309,7 @@ def test_gb_get_parent(gb_db):
             name=cds_id,
             exclude_biotype="CDS",
             start=coords.min(),
-            end=coords.max(),
+            stop=coords.max(),
         )
     )[0]
     assert parent["biotype"] != cds["biotype"]
@@ -474,8 +475,8 @@ def test_get_features_matching_start_stop(DATA_DIR, seq):
 
 
 def test_matching_conditions():
-    got, _ = _matching_conditions({"start": 1, "end": 5}, allow_partial=True)
-    expect = "((start >= 1 AND end <= 5) OR (start <= 1 AND end > 1) OR (start < 5 AND end >= 5) OR (start <= 1 AND end >= 5))"
+    got, _ = _matching_conditions({"start": 1, "stop": 5}, allow_partial=True)
+    expect = "((start >= 1 AND stop <= 5) OR (start <= 1 AND stop > 1) OR (start < 5 AND stop >= 5) OR (start <= 1 AND stop >= 5))"
     assert got == expect
 
 
@@ -937,6 +938,48 @@ def test_load_anns_with_write(DATA_DIR, tmp_dir):
     assert got_data["tables"] == expect_data["tables"]
 
 
+def test_gff_end_renamed_to_stop(gff_db, tmp_path):
+    bad_col_path = tmp_path / "bad_col.gffdb"
+
+    correct_rich_dict = gff_db.to_rich_dict()
+    del correct_rich_dict["init_args"]
+
+    for table_name in gff_db.table_names:
+        # Convert in memory db stop column to the old end name
+        _rename_column_if_exists(gff_db.db, table_name, "stop", "end")
+
+    gff_db.write(tmp_path / bad_col_path)
+
+    # Test that end column is renamed to stop on construction
+    loaded_gff_db = GffAnnotationDb(source=bad_col_path)
+
+    new_rich_dict = loaded_gff_db.to_rich_dict()
+    del new_rich_dict["init_args"]
+
+    assert new_rich_dict == correct_rich_dict
+
+
+def test_gb_end_renamed_to_stop(gb_db, tmp_path):
+    bad_col_path = tmp_path / "bad_col.gbdb"
+
+    correct_rich_dict = gb_db.to_rich_dict()
+    del correct_rich_dict["init_args"]
+
+    for table_name in gb_db.table_names:
+        # Convert in memory db stop column to the old end name
+        _rename_column_if_exists(gb_db.db, table_name, "stop", "end")
+
+    gb_db.write(tmp_path / bad_col_path)
+
+    # Test that end column is renamed to stop on construction
+    loaded_gb_db = GenbankAnnotationDb(source=bad_col_path)
+
+    new_rich_dict = loaded_gb_db.to_rich_dict()
+    del new_rich_dict["init_args"]
+
+    assert new_rich_dict == correct_rich_dict
+
+
 def test_gbdb_get_children_fails_no_coords(gb_db):
     with pytest.raises(ValueError):
         _ = list(gb_db.get_feature_children(name="CNA00110"))
@@ -955,7 +998,7 @@ def test_load_annotations_invalid_path():
 @pytest.mark.parametrize("integer", (int, numpy.int64))
 def test_subset_gff3_db(gff_db, integer):
     subset = gff_db.subset(
-        seqid="I", start=integer(40), end=integer(70), allow_partial=True
+        seqid="I", start=integer(40), stop=integer(70), allow_partial=True
     )
     # manual inspection of the original GFF3 file indicates 7 records
     # BUT the CDS records get merged into a single row
@@ -963,7 +1006,7 @@ def test_subset_gff3_db(gff_db, integer):
 
 
 def test_subset_empty_db(gff_db):
-    subset = gff_db.subset(seqid="X", start=40, end=70, allow_partial=True)
+    subset = gff_db.subset(seqid="X", start=40, stop=70, allow_partial=True)
     # no records
     assert not len(subset)
 
@@ -973,7 +1016,7 @@ def test_subset_gff3_db_with_user(gff_db):
         seqid="I", name="gene-01", biotype="gene", spans=[(23, 43)], strand="+"
     )
     gff_db.add_feature(**record)
-    subset = gff_db.subset(seqid="I", start=40, end=70, allow_partial=True)
+    subset = gff_db.subset(seqid="I", start=40, stop=70, allow_partial=True)
     # manual inspection of the original GFF3 file indicates 7 records
     # BUT the CDS records get merged into a single row
     assert len(subset) == 7
@@ -988,7 +1031,7 @@ def test_subset_gb_db(gb_db):
 def test_subset_gff3_db_source(gff_db, tmp_dir):
     outpath = tmp_dir / "subset.gff3db"
     subset = gff_db.subset(
-        seqid="I", start=40, end=70, allow_partial=True, source=outpath
+        seqid="I", start=40, stop=70, allow_partial=True, source=outpath
     )
     subset.db.close()