search

vert-x3 / vertx-mysql-postgresql-client

This client is deprecated - use instead
https://github.com/eclipse-vertx/vertx-sql-client
Apache License 2.0
117 stars 59 forks source link

Add support for TIME columns #94

Closed Narigo closed 6 years ago

Narigo commented 6 years ago

This addresses #92.

It will add support for TIME types. PostgreSQL returns a LocalTime and MySQL returns a FiniteDuration. I let LocalTime result in a string containing HH:MM:SS.<millis> and FiniteDuration results a long value of milliseconds.

WDYT @pmlopes ?

Narigo commented 6 years ago

Changed it to have both result in HH:MM:SS.<ms> as the suggestion from @mslenc in https://github.com/vert-x3/vertx-mysql-postgresql-client/issues/92#issuecomment-350279302 made sense.

pmlopes commented 6 years ago

This is tricky since postgres supports TIME WITH TIMEZONE this would truncate all TZ data right?

mslenc commented 6 years ago

@pmlopes I think that's a separate/additional issue. Merging this in would at least let the LocalTime and FiniteDuration cases work, which is far better than the current state of things, where things simply "explode"..

pmlopes commented 6 years ago

I'll leave the decision to the module owner (@Narigo).

Narigo commented 6 years ago

Maybe we can add another test for that then @pmlopes - I wasn't aware of the TIME WITH TIMEZONE issue for PostgreSQL.

mslenc commented 6 years ago

Could this please be merged in and released? It's been over half a year..

vietj commented 6 years ago

I think there is a missing test for TIME WITH TIMEZONE as said in the comment, if you add this test, I'll be happy to merge it

mslenc commented 6 years ago

I'm pretty sure that comment is misguided - I don't really know what TIME WITH TIMEZONE translates into, but if it's Duration or LocalTime, which are the only two types this PR affects/enables to work, any timezone information is already lost. So any handling of timezone information would have to be done upstream (AFAIK, that means in the https://github.com/mauricio/postgresql-async project).

(and if TIME WITH TIMEZONE becomes something else, this PR has nothing do with it)

vietj commented 6 years ago

@pmlopes can you provide guidance here ?

mslenc commented 6 years ago

I'd like to again encourage you to merge this - it's a real pain not to be able to normally query TIME columns.. Last time I could change the schema to avoid using TIME, but on my current project I can't do so.. To reiterate, the original concern by @pmlopes was that timezone information is somehow lost by this change, but that is not the case - the problem it's solving is simply this: