Map INTERVAL types to Python types #475
Conversation
| SqlTest(trino_connection) \ | ||
| .add_field(sql="CAST(null AS INTERVAL YEAR TO MONTH)", python=None) \ | ||
| .add_field(sql="INTERVAL '10' YEAR", python=relativedelta(years=10)) \ |
There was a problem hiding this comment.
Please test for min/max valid trino value and min/max valid python values too.
There was a problem hiding this comment.
Does this test verify both directions? i.e. converting from Trino string representation to Python object AND converting from Python object to Trino SQL (e.g. queries with parameters?)?
| .add_field(sql="INTERVAL '-5 23:59:57.000' DAY TO SECOND", python=timedelta(days=-6, seconds=3)) \ | ||
| .add_field(sql="INTERVAL '12 10:45' DAY TO MINUTE", python=timedelta(days=12, seconds=38700)) \ | ||
| .add_field(sql="INTERVAL '45:32.123' MINUTE TO SECOND", python=timedelta(seconds=2732, microseconds=123000)) \ | ||
| .add_field(sql="INTERVAL '32.123' SECOND", python=timedelta(seconds=32, microseconds=123000)) \ |
There was a problem hiding this comment.
For INTERVAL '2147483647 23:59:59.999999' DAY TO SECOND I recieved below error. So it looks like we cannot really use timedelta for INTERVAL DAY TO SECOND type.
> return timedelta(days=days, hours=hours, minutes=minutes, seconds=seconds, milliseconds=milliseconds)
E OverflowError: days=2147483647; must have magnitude <= 999999999
There was a problem hiding this comment.
As discussed offline let's check what the JDBC driver does when a Trino value is too large/small to fit into corresponding Java type. If it throws an error then the existing implementation we have here seems fine to me.
There was a problem hiding this comment.
We do use TrinoDataError in some places where values from Trino cannot be mapped to Python.
There was a problem hiding this comment.
Added test cases for edge scenarios. Raised TrinoDataError when Trino value is too large/small. PTAL @hashhar
damian3031
force-pushed
the
hovaesco/interval-mappings
branch
from
b5f3887
to
585acf1
Compare
…r minimum limit. Add test cases for edge scenarios
damian3031
force-pushed
the
hovaesco/interval-mappings
branch
from
585acf1
to
d5f5e54
Compare
Description
Resolves #356
relativedeltatimedeltaNon-technical explanation
Release notes
( ) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
( ) Release notes are required, with the following suggested text: