Support for TIME(p) and TIMESTAMP(p) to SQLAlchemy

[hovaesco]

First point from #107

[hovaesco]

I'm not really sure whether it's a correct approach. Interval in Trino has a different value and data type.
For instance:

• data type: INTERVAL YEAR TO MONTH
• value: INTERVAL '3' MONTH

[andrewdibiasio6]

Is this func supposed to return the data type or value? It seems like it returns a mix of both.

[hovaesco]

It is a part when I'm not sure in which way to go. Other SQLAlchemy dialects which support INTERVAL data type handles it in different way (one return a value, other data type). INTERVAL data type doesn't seem to be supported for other dialects..

• PostgreSQL: https://github.com/sqlalchemy/sqlalchemy/blob/main/lib/sqlalchemy/dialects/postgresql/base.py#L2489-L2495
• Oracle: https://github.com/sqlalchemy/sqlalchemy/blob/main/lib/sqlalchemy/dialects/oracle/base.py#L647-L655

[andrewdibiasio6]

I will move to other PR.

[andrewdibiasio6]

Many checks are failing.

[hovaesco]

I've opened a new PR just with just INTERVAL changes #184

[ebyhr]

Could you please provide simple test script? As you already know, we don't have integration tests for sqlalchemy. I hesitate to merge with confidence.

[hovaesco]

Use the below script to test it.

from sqlalchemy import create_engine
from sqlalchemy.schema import MetaData, Column, Table
from trino.sqlalchemy.datatype import TIMESTAMP, TIME

engine = create_engine('trino://user@localhost:8080/memory/default')

metadata_obj = MetaData()

user = Table('test_time_timestamp', metadata_obj,
    Column('col1', TIME(precision=6, timezone=False)),
    Column('col2', TIMESTAMP(precision=3, timezone=True))
)

metadata_obj.create_all(engine)

As you already know, we don't have integration tests for sqlalchemy.

It's a good candidate to create a new issue.

[ebyhr]

@hovaesco Thanks for providing the code. Could you test with precision=0? It creates time(3) now, but it should be time(0). Also, can we add an assertion to verify the precision is in valid ranges (0, 12)?

[hovaesco]

Thanks for spotting this out @ebyhr. I fixed it and added an assertion.

[andrewdibiasio6]

LGTM pending ebyhr approval.

[ebyhr]

Merge, thanks!