search

trinodb / trino

Official repository of Trino, the distributed SQL query engine for big data, formerly known as PrestoSQL (https://trino.io)
https://trino.io
Apache License 2.0
10.15k stars 2.93k forks source link

Wrong exception when calling ResultSet.getTimestamp on TIME value #5315

Open findepi opened 3 years ago

findepi commented 3 years ago 
java.lang.IllegalArgumentException: Expected column to be a timestamp type but is time(3)
    at io.prestosql.jdbc.AbstractPrestoResultSet.getTimestamp(AbstractPrestoResultSet.java:362)
    at io.prestosql.jdbc.AbstractPrestoResultSet.getTimestamp(AbstractPrestoResultSet.java:327)
    at io.prestosql.jdbc.PrestoResultSet.getTimestamp(PrestoResultSet.java:44)

should be SQLException

rafatmunshi commented 3 years ago

I would like to work on this. You just need the type of exception changed right?

findepi commented 3 years ago

Hm... looking at the javadoc for java.sql.ResultSet#getTimestamp(int) again. The javadoc does not mandate use of SQLException in this case. @rafatmunshi would you be able to check what's eg PostgreSQL's and Oracle's JDBC behavior here?

rafatmunshi commented 3 years ago

Hi, checked Oracle and Postgres JDBC docs. Both mention throws SQLException. Logically also I believe it should throw SQLException only as you suggested @findepi . So I think we just need to change the Exception type in the file and do the relevant change in the tests. Hope I am not missing something?

findepi commented 3 years ago

@jetsasank recently did similar improvement for getDate, @rafatmunshi please check this out: https://github.com/prestosql/presto/pull/5510

rafatmunshi commented 3 years ago

Checked it. Clarifies my earlier query. Will send a PR soon. Thanks!

somayaj commented 3 years ago

hi, is this available for a commit?

rafatmunshi commented 3 years ago

Hi @somayaj , I am working on a PR linked to this now. Thanks