Java 8 Time Support for Postgres Arrays

[geekbeast]

I've been looking around and it looks like Java 8 Time Support for Postgres Arrays is not yet implemented and there aren't any other issues so opening a new one.

At first glance it looks like the required change would be in PgArray.buildArray(...) as it currently defaults to marshalling DATE, TIME, and TIMESTAMP to java.sql.* classes.

Looking at the existing methods called by ResultSet.getObject(colName, Class<T>) its straightforward to get the value. What's not clear to me is (1) whether the binary case that is handled getObject needs to be handled for arrays and (2) how to maintain backward compatibility. One way to possibly support in a backwards compatible is to implement the version of getArray that takes a map.

For now, I'm manually doing the conversion as I'm on tight deadline and it's easy enough to do manual conversions.

[bokken]

Are you wanting to create an array to provide as input to a statement, read an array being returned, or both?

I would suggest taking a look at PRs #1194 and #1184 which propose some refactoring to array handling to make it easier to add array support (including binary support) for new data types.

[geekbeast]

Mostly for read as I get an array due to an array_agg(...) on the read.

I worked around it for now with:

                case Date:
                    objects = Stream
                            .of( (Date[]) arr.getArray() )
                            .map( Date::toLocalDate )
                            .collect( Collectors.toList() );
                    break;
                case TimeOfDay:
                    objects = Stream
                            .of( (Time[]) arr.getArray() )
                            .map( Time::toLocalTime )
                            .collect( Collectors.toList() );
                    break;
                case DateTimeOffset:
                    objects = Stream
                            .of( (Timestamp[]) arr.getArray() )
                            .map( ts -> OffsetDateTime
                                    .ofInstant( Instant.ofEpochMilli( ts.getTime() ), ZoneId.of( "UTC" ) ) )
                            .collect( Collectors.toList() );

[geekbeast]

Also, I've been eyeing those PRs as I having to work around reading byte[][]. Same thing as above an array_agg on byte[] column.

[guyco33]

I've also encountered similar issue with timestamp[]:

create table test_ts(ts_col timestamp, ts_arr_col timestamp[]);

insert into test_ts values (
      '1970-01-01 00:12:34.567'::timestamp, 
array['1970-01-01 00:12:34.567'::timestamp]);

select * from public.test_ts;
         ts_col          |         ts_arr_col
-------------------------+-----------------------------
 1970-01-01 00:12:34.567 | {"1970-01-01 00:12:34.567"}
(1 row)

rs.getTimestamp("ts_col")); --> 1970-01-01 01:12:34.567 (wrong value)
rs.getObject("ts_col", LocalDateTime.class); --> 1970-01-01T00:12:34.567

Array.get(rs.getArray("ts_arr_col").getArray(),0)); --> 1970-01-01 01:12:34.567 (wrong value)
Array.get(rs.getArray("ts_arr_col").getArray(),0).getClass() --> class java.sql.Timestamp

As you can see, it's not possible to retrieve the correct value from timestamp[], since there is no way to use java.time.LocalDateTime

[davecramer]

Thanks for this report. Anyone care to provide a PR ?

[findepi]

@davecramer let's discuss API first.

When reading one could use rs.getObject(column, LocalDateTime[].class) to request lossless representation of PG's timestamp[].
However, to handle multi-dimensional arrays, this could get cumbersome and rs.getArray(column) doesn't provide a way to request a specific representation. (I am convinced changing existing behavior is no-go.)

• do we need an initialization property?
• or a PG-specific method?

When writing it should be easier. We can accept (multi-dim) arrays of LocalDateTime. We still want #1390

[davecramer]

@findepi
an initialization property ?
yes, I would like to discuss how to handle this overall. The question becomes do we want to use the server timezone for everything or the client timezone. Unfortunately the docs don't provide this direction

[findepi]

@davecramer AFAIU, PostgreSQL timestamp [without time zone] and Java LocalDateTime are both unrelated to any time zone (session, client's, server's).

Time zones are implicated only because JDBC wants you to use java.sql.Timestamp which (AFAICT) is always interpreted in correspondence to JVM zone. (This has inherent pitfalls: java.sql.Timestamp cannot represent (zone-less) date/time where JVM has forward DST change or forward policy change.)

I've investigated this in Presto SQL (trinodb/trino#37 & related issues and PRs) and my opinion is that we we should use no time zone at all when handling timestamp values and treat them as "year/month/day hour/minute/second/millis" "structs".
(I don't know how this plays with PostgreSQL protocol. This is beyond my knowledge.)

cc @electrum

[davecramer]

@findepi you'd probably be surprised to know that timestamp with timezone doesn't store the timezone either then.

[findepi]

you'd probably be surprised to know that timestamp with timezone doesn't store the timezone either then.

In PostgreSQL -- yes, I know.

Right, I missed the fact the issue is broader than just timestamp[].
I was not as much concerned about timestamptz[] being mapped to java.sql.Timestamp[] because in timestamptz case all I need is point in time. So this mapping at least does not lose information. https://github.com/prestosql/presto/blob/c39358eee752eac13959fd05dc5427502cf03b33/presto-postgresql/src/main/java/io/prestosql/plugin/postgresql/PostgreSqlClient.java#L355-L357

(Or course java.time.Instant would be more appropriate)

For date[], mapping to java.sql.Date[] loses information in one edge case only (DATE '2011-12-30' and Pacific/Apia zone). Of course, it's still desirable to get the date[] data as java.time.LocalDate[].

(Let me explain my point of view. I am concerned mostly about losing information and not about convenience, because in Presto use-cases, not losing information is all we need. Even accessing String would be enough. Others, are probably more interested in getting java.time. classes because in application code there is usually more code that interacts with JDBC in some way.)

[davecramer]

you'd probably be surprised to know that timestamp with timezone doesn't store the timezone either then.

In PostgreSQL -- yes, I know.

Right, I missed the fact the issue is broader than just timestamp[].
I was not as much concerned about timestamptz[] being mapped to java.sql.Timestamp[] because in timestamptz case all I need is point in time. So this mapping at least does not lose information. https://github.com/prestosql/presto/blob/c39358eee752eac13959fd05dc5427502cf03b33/presto-postgresql/src/main/java/io/prestosql/plugin/postgresql/PostgreSqlClient.java#L355-L357

(Or course java.time.Instant would be more appropriate)

For date[], mapping to java.sql.Date[] loses information in one edge case only (DATE '2011-12-30' and Pacific/Apia zone). Of course, it's still desirable to get the date[] data as java.time.LocalDate[].
Can you elaborate on why this one edge case fails

(Let me explain my point of view. I am concerned mostly about losing information and not about convenience, because in Presto use-cases, not losing information is all we need. Even accessing String would be enough. Others, are probably more interested in getting java.time. classes because in application code there is usually more code that interacts with JDBC in some way.)

Sadly the whole date/time/timestamp thing with java and JDBC is beyond repair. We have to chose to be opinionated here. At the moment I'm not sure of the "best" opinion. I'm going to confer with the npgsql guys to see what their driver does to get some level of consistency

[bokken]

I think we could relatively easily allow consumer to override default array type handling once the PR for reworking array decoding is merged.

…

On Tue, Jul 30, 2019 at 9:16 AM Dave Cramer ***@***.***> wrote: you'd probably be surprised to know that timestamp with timezone doesn't store the timezone either then. In PostgreSQL -- yes, I know. Right, I missed the fact the issue is broader than just timestamp[]. I was not as much concerned about timestamptz[] being mapped to java.sql.Timestamp[] because in timestamptz case all I need is point in time. So this mapping at least does not lose information. https://github.com/prestosql/presto/blob/c39358eee752eac13959fd05dc5427502cf03b33/presto-postgresql/src/main/java/io/prestosql/plugin/postgresql/PostgreSqlClient.java#L355-L357 (Or course java.time.Instant would be more appropriate) For date[], mapping to java.sql.Date[] loses information in one edge case only (DATE '2011-12-30' and Pacific/Apia zone). Of course, it's still desirable to get the date[] data as java.time.LocalDate[]. Can you elaborate on why this one edge case fails (Let me explain my point of view. I am concerned mostly about losing information and not about convenience, because in Presto use-cases, not losing information is all we need. Even accessing String would be enough. Others, are probably more interested in getting java.time. classes because in application code there is usually more code that interacts with JDBC in some way.) Sadly the whole date/time/timestamp thing with java and JDBC is beyond repair. We have to chose to be opinionated here. At the moment I'm not sure of the "best" opinion. I'm going to confer with the npgsql guys to see what their driver does to get some level of consistency — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#1225?email_source=notifications&email_token=AAW3U3LOBA5R5MSNNFFKJHDQCBEKHA5CNFSM4FG5ZJFKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD3EDTYI#issuecomment-516438497>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAW3U3PVECBLJ6JNV2QTEP3QCBEKHANCNFSM4FG5ZJFA> .

[davecramer]

Brett, What do we have to do to get that merged? Dave Cramer

…

On Tue, 30 Jul 2019 at 13:12, Brett Okken ***@***.***> wrote: I think we could relatively easily allow consumer to override default array type handling once the PR for reworking array decoding is merged. On Tue, Jul 30, 2019 at 9:16 AM Dave Cramer ***@***.***> wrote: > you'd probably be surprised to know that timestamp with timezone doesn't > store the timezone either then. > > In PostgreSQL -- yes, I know. > > Right, I missed the fact the issue is broader than just timestamp[]. > I was not as much concerned about timestamptz[] being mapped to > java.sql.Timestamp[] because in timestamptz case all I need is point in > time. So this mapping at least does not lose information. > https://github.com/prestosql/presto/blob/c39358eee752eac13959fd05dc5427502cf03b33/presto-postgresql/src/main/java/io/prestosql/plugin/postgresql/PostgreSqlClient.java#L355-L357 > > (Or course java.time.Instant would be more appropriate) > > For date[], mapping to java.sql.Date[] loses information in one edge case > only (DATE '2011-12-30' and Pacific/Apia zone). Of course, it's still > desirable to get the date[] data as java.time.LocalDate[]. > Can you elaborate on why this one edge case fails > > (Let me explain my point of view. I am concerned mostly about losing > information and not about convenience, because in Presto use-cases, not > losing information is all we need. Even accessing String would be enough. > Others, are probably more interested in getting java.time. classes because > in application code there is usually more code that interacts with JDBC in > some way.) > > Sadly the whole date/time/timestamp thing with java and JDBC is beyond > repair. We have to chose to be opinionated here. At the moment I'm not sure > of the "best" opinion. I'm going to confer with the npgsql guys to see what > their driver does to get some level of consistency > > — > You are receiving this because you commented. > Reply to this email directly, view it on GitHub > < #1225?email_source=notifications&email_token=AAW3U3LOBA5R5MSNNFFKJHDQCBEKHA5CNFSM4FG5ZJFKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD3EDTYI#issuecomment-516438497 >, > or mute the thread > < https://github.com/notifications/unsubscribe-auth/AAW3U3PVECBLJ6JNV2QTEP3QCBEKHANCNFSM4FG5ZJFA > > . > — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1225?email_source=notifications&email_token=AADDH5SDDDIUC5JASSIVGVDQCBY6DA5CNFSM4FG5ZJFKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD3EVC5A#issuecomment-516510068>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AADDH5XU7HNBJCIQKDCHXT3QCBY6DANCNFSM4FG5ZJFA> .

[bokken]

You and Vladimir to review, approve and merge. 😀 On Wed, Jul 31, 2019 at 10:42 AM Dave Cramer <notifications@github.com> wrote:

…

Brett, What do we have to do to get that merged? Dave Cramer On Tue, 30 Jul 2019 at 13:12, Brett Okken ***@***.***> wrote: > I think we could relatively easily allow consumer to override default array > type handling once the PR for reworking array decoding is merged. > > On Tue, Jul 30, 2019 at 9:16 AM Dave Cramer ***@***.***> > wrote: > > > you'd probably be surprised to know that timestamp with timezone doesn't > > store the timezone either then. > > > > In PostgreSQL -- yes, I know. > > > > Right, I missed the fact the issue is broader than just timestamp[]. > > I was not as much concerned about timestamptz[] being mapped to > > java.sql.Timestamp[] because in timestamptz case all I need is point in > > time. So this mapping at least does not lose information. > > > https://github.com/prestosql/presto/blob/c39358eee752eac13959fd05dc5427502cf03b33/presto-postgresql/src/main/java/io/prestosql/plugin/postgresql/PostgreSqlClient.java#L355-L357 > > > > (Or course java.time.Instant would be more appropriate) > > > > For date[], mapping to java.sql.Date[] loses information in one edge case > > only (DATE '2011-12-30' and Pacific/Apia zone). Of course, it's still > > desirable to get the date[] data as java.time.LocalDate[]. > > Can you elaborate on why this one edge case fails > > > > (Let me explain my point of view. I am concerned mostly about losing > > information and not about convenience, because in Presto use-cases, not > > losing information is all we need. Even accessing String would be enough. > > Others, are probably more interested in getting java.time. classes > because > > in application code there is usually more code that interacts with JDBC > in > > some way.) > > > > Sadly the whole date/time/timestamp thing with java and JDBC is beyond > > repair. We have to chose to be opinionated here. At the moment I'm not > sure > > of the "best" opinion. I'm going to confer with the npgsql guys to see > what > > their driver does to get some level of consistency > > > > — > > You are receiving this because you commented. > > Reply to this email directly, view it on GitHub > > < > #1225?email_source=notifications&email_token=AAW3U3LOBA5R5MSNNFFKJHDQCBEKHA5CNFSM4FG5ZJFKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD3EDTYI#issuecomment-516438497 > >, > > or mute the thread > > < > https://github.com/notifications/unsubscribe-auth/AAW3U3PVECBLJ6JNV2QTEP3QCBEKHANCNFSM4FG5ZJFA > > > > . > > > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > < #1225?email_source=notifications&email_token=AADDH5SDDDIUC5JASSIVGVDQCBY6DA5CNFSM4FG5ZJFKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD3EVC5A#issuecomment-516510068 >, > or mute the thread > < https://github.com/notifications/unsubscribe-auth/AADDH5XU7HNBJCIQKDCHXT3QCBY6DANCNFSM4FG5ZJFA > > . > — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#1225?email_source=notifications&email_token=AAW3U3MH5F6URWV554CLKGDQCGXGBA5CNFSM4FG5ZJFKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD3HVS2I#issuecomment-516905321>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAW3U3LKHGFXZJBG4DDUPEDQCGXGBANCNFSM4FG5ZJFA> .