Type conversion for arrays

[moea]

Hey - thanks again for this library.

This branch implements conversion of n-dimensional arrays going into and out of the database, with the help of some modified parsing code ripped out of pgjdbc.

I don't know whether this is a feature you're interested in incorporating, however either way I'd very much appreciate some feedback on how best to integrate it, either as a feature or a separate library.

High level stuff:

There's an object used for both insertion and retrieval, PgArray, which is given either an array (for insertion), which it converts to a string using a pretty simple and predictable couple of methods, or is given a string (for parsing) and an OID, and returns an Object[] (arbitrarily nested), where the leaves are the result of delegating the element parsing to DataConverter.

That's the main sticking point as far as integration - the array converter's constructor accepts a DataConverter (which is constructed vanilla in the tests, just to get the thing working), but ideally we want it to have access to the pool's converter, so it can parse arrays of types which are themselves converted by user code.

I wrote this for use from Clojure/postgres.async, but touching DataConverter seemed like the wrong thing to do, and I think I may be missing something as far as how, in Java, it would practical to have the .get which happens during the CLJ map conversion correctly invoke ArrayConverter. Obviously if this feature ends up in your library, it would be possible to add more clauses to the switch in toObject

Anyway, I'm super open minded about making changes, etc. - so please let me know if you can imagine any problems.

Todo: It'll be a quick change, but I think strings are constructed in a few places without passing in a codec, which could cause problems. Will revisit tomorrow.

More notes:

The largest changes I made to the pgjdbc utilities are as follows:

• The JDBC stuff queries the database for type-specific array delimiters before parsing. I wanted to avoid doing that at all costs. I'm also unable to determine in what situations the literal delimiter for an array would be anything other than a comma, so I'm mostly not worrying about it. I checked the catalog in a few varied PG installations for type delimiters, they're all commas, and the only references I can find online are about alternative delimiters with can be used when parsing/constructing strings from/to arrays, which is something different.
• I removed pretty much everything from buildArray - including the facility for constructing primitive arrays, and for declaring the arrays as anything but Object[]. Without having a mapping from OIDs to converter return types (including types constructed by custom converters), I think the only other approach to this would be to defer array creation until we can reflect the return type. I'm open-minded about this.
• PG 8.3+ supports NULL as an array element. I removed the version checking, so the NULL will go to the DB regardless of its version. It works fine both in-and-out post-8.3.

[moea]

Oh, also, not using java.sql.Array was intentional, it didn't seem that relevant

[alaisi]

Thanks for the pr! I'll take a look, array support is definately something I want to add.

[moea]

Is there anything I could do to make it easier for you to take a look at including this?

[alaisi]

Sorry for the delay, I really am interested in getting this merged.

Here are some ideas:

• Method for getting a single-dimension array from Row could be <T> T[] getArray(String column, T typeClass).
• Both single- and multi-dimensional arrays could be accessed with get. Example: int[][] ids = row.get("ids", int[][].class).
• Array support could be added into DataConverter as a core type (not via converter)
• The parsing code looks pretty heavy and complex, I might consider special casing parsing of String arrays and handling other cases with a simpler code path.

DataConverter could maybe look something like this:

// DataConverter.java
public <T> T toArray(Optional<Class<T>> dimensionalArrayType, Oid oid, byte[] value):
    switch(oid) {
        case INT2_ARRAY:
        case INT4_ARRAY:
        case INT8_ARRAY:
            return ArrayConversions.toArray(dimensionalArrayType, oid, value, NumericConversions::toLong);

ArrayConversions would then handle parsing of (nested) '{' and '}' and calling the supplied function to create an object from the string value. The dimensionalArrayType parameter (eg. int[][].class) can be missing since the clojure version doesn't care about types.

How does this sound to you?

[moea]

Hey - I'll need to re-familiarize myself with the code, but it sounds good to me. I'll take a look at making the changes this week.

[moea]

I updated this to be in line with your comments. Thanks a bunch for the feedback.

Some notes/clarifications:

• The implementation uses boxed types everywhere for representing values retrieved from Postgres, due the permissibility of null values in Postgres arrays. You could make an argument for not using Java arrays at all, and using a generic collection - however in general I've erred on the side of not altering pgjdbc's fundamental implementation strategy.
• I've adjusted DataConverter to do as you suggested, and added clauses for the handling of arrays of all of the currently-supported types - with the exception of bytea[]. As the converter for that function returns a byte[], and buildArray gets its depth cues only from the databases values, this would have to be handled with some pretty ugly special-casing, I believe.
• The type passed to DataConverter.toArray, e.g. Long[][] amounts basically to a runtime assertion about the return values of the converter which was delegated to for individual elements. I implemented your example above of delegating INT2, INT4 and INT8 elements to NumericConversions::toLong, however this means that users must call getArray(..., Long[].class), even though it may not be immediately obvious that an INT2 would map to a long (because it doesn't for single values). Maybe the types should be symmetric?
• Internally, the element-converter method reference (passed to ArrayConversions) is typed as returning Object. I think this could only be more specific if toArraywas parameterised over the type of the elements, as well as the type of the array, but I don't think it matters.
• Class.isArray is the clause in fromObject which determines whether to use ArrayConversions - accordingly, primitive arrays can be inserted.
• The only point I did not address was an unquoted code path for buildArrayList. If/when everything else makes sense, I can have a look at simplifying it.

I'm far from comfortable with Java's type system when things get complicated, so feel free to tell me how weird this is.

[moea]

P.S. isArray() as the fromObject guard is too specific, if we want to be able to insert regular collections. There could be another clause which looks at interfaces.

[alaisi]

This looks good.

• Change the numeric types to be symmetric. Delegating all numeric types to long would have worked with primitive types, but since we have to used boxed types (I forgot NULLs in arrays) all numeric types must be handled separately.
• Use UTF-8 with String::getBytes

Commit the fixes and this is ready for merge.

I will probably at some point revisit the decision of using byte[] as the internal data format, the getBytes/fromBytes shuffing is pretty ugly but there is no way around it in the current api.

[moea]

Done. I added a few more tests for the changes.

[alaisi]

Merged, thanks!