Add support for sa.TIME alongside sa.Time, ensuring consistent handling and round-trip behavior

Author: florinutz

src/sqlalchemy_cratedb/compiler.py

@@ -246,7 +246,7 @@ def visit_datetime(self, type_, **kw):
     def visit_date(self, type_, **kw):
         return "TIMESTAMP"
 
-    def visit_time(self, type_, **kw):
+    def visit_TIME(self, type_, **kw):
         """
         CrateDB has no storable `TIME` column type. Plain `TIME` does not exist,
         and `TIME WITH TIME ZONE` (TIMETZ) is literal/cast-only ("does not support

src/sqlalchemy_cratedb/dialect.py

@@ -171,6 +171,7 @@ class Time(sqltypes.Time):
     def bind_processor(self, dialect):
         def process(value):
             if value is not None:
+                assert isinstance(value, time)  # noqa: S101
                 return value.isoformat()
             return None
 
@@ -180,8 +181,6 @@ def result_processor(self, dialect, coltype):
         def process(value):
             if value is None:
                 return None
-            if isinstance(value, time):
-                return value
             return time.fromisoformat(value)
 
         return process

tests/create_table_test.py

@@ -84,19 +84,26 @@ def test_table_time_type(self):
 
         Validates the fix for https://github.com/crate/sqlalchemy-cratedb/issues/206,
         where `sa.Time` previously compiled to `TIME`, which CrateDB rejects with
-        `SQLParseException[Cannot find data type: time]`.
+        `SQLParseException[Cannot find data type: time]`. Both the generic
+        `sa.Time` and the SQL-standard capitalised `sa.TIME` must render `STRING`.
         """
 
         class Schedule(self.Base):
             __tablename__ = "schedule"
             name = sa.Column(sa.String, primary_key=True)
-            time_col = sa.Column(sa.Time)
+            time_lower = sa.Column(sa.Time)
+            time_upper = sa.Column(sa.TIME())
+
+        # Exercise both spellings: `sa.TIME` is a subclass of `sa.Time`.
+        assert isinstance(Schedule.__table__.c.time_lower.type, sa.Time)
+        assert isinstance(Schedule.__table__.c.time_upper.type, sa.TIME)
 
         self.Base.metadata.create_all(bind=self.engine)
         fake_cursor.execute.assert_called_with(
             (
                 "\nCREATE TABLE schedule (\n\tname STRING NOT NULL, "
-                "\n\ttime_col STRING, "
+                "\n\ttime_lower STRING, "
+                "\n\ttime_upper STRING, "
                 "\n\tPRIMARY KEY (name)\n)\n\n"
             ),
             sa.util.immutabledict({}),

tests/datetime_test.py

@@ -106,6 +106,7 @@ class FooBar(Base):
     datetime_notz = sa.Column(DateTime(timezone=False))
     datetime_tz = sa.Column(DateTime(timezone=True))
     time = sa.Column(sa.Time)
+    time_upper = sa.Column(sa.TIME())
 
 
 @pytest.fixture
@@ -234,10 +235,11 @@ def test_datetime_date(session):
 @pytest.mark.skipif(SA_VERSION < SA_1_4, reason="Test case not supported on SQLAlchemy 1.3")
 def test_time(session):
     """
-    An integration test for `sa.Time`.
+    An integration test for `sa.Time` and the SQL-standard `sa.TIME`.
 
     CrateDB has no native `TIME` type, so the dialect stores it as a `STRING`
-    in ISO 8601 format and parses it back into a `dt.time` object on read.
+    in ISO 8601 format and parses it back into a `dt.time` object on read. Both
+    spellings resolve to the same colspec, so both must round-trip.
 
     Validates the fix for https://github.com/crate/sqlalchemy-cratedb/issues/206.
     """
@@ -246,14 +248,21 @@ def test_time(session):
     foo_item = FooBar(
         name="foo",
         time=dt.time(19, 0, 30, 123456),
+        time_upper=dt.time(19, 0, 30, 123456),
     )
     session.add(foo_item)
     session.commit()
     session.execute(sa.text("REFRESH TABLE foobar"))
 
     # query
-    result = session.execute(sa.select(FooBar.name, FooBar.time)).mappings().first()
+    result = (
+        session.execute(sa.select(FooBar.name, FooBar.time, FooBar.time_upper))
+        .mappings()
+        .first()
+    )
 
     # compare
     assert result["time"] == dt.time(19, 0, 30, 123456)
     assert isinstance(result["time"], dt.time)
+    assert result["time_upper"] == dt.time(19, 0, 30, 123456)
+    assert isinstance(result["time_upper"], dt.time)