Re: AbstractJdbc2Array - another patch

Hello all,
in the attachment you can find AbstractJdbc2Array with small patch
(empty array was wrongly interpreted). Please test it and if possible
add to cvs.

Regards,
Marek Lewczuk

Marek Lewczuk wrote:
> in the attachment you can find AbstractJdbc2Array with small patch
> (empty array was wrongly interpreted). Please test it and if possible
> add to cvs.

There seems to be a lot of unrelated stylistic changes, whitespace
changes, renaming of fields etc. in that new file. That makes it hard to
see what the problem is that it's fixing, and what was done to fix it.
Please make just the changes required to fix the problem, and if you
want to propose any other changes, send them as separate patches.

Also, please send patches in a "cvs diff -c" format, instead of sending
the whole file.

A small test case demonstrating the problem would also be very nice, to
help with the testing.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

On Thu, 11 Oct 2007, Marek Lewczuk wrote:

> Hello all,
> in the attachment you can find AbstractJdbc2Array with small patch (empty
> array was wrongly interpreted). Please test it and if possible add to cvs.
>

I've finally found some time to test this and...

1) It causes six failures in the regression tests, these need to be fixed.

2) It would be good to have new tests for this new functionality.

3) When determing if NULL in an array string is a null value, you need to
check the server version. Prior to 8.2 an unadorned NULL is the text
"null", not an actual null value.

4) Shouldn't toString(PgArrayList) be PgArrayList.toString() ?

5) Your coding style is a little hard to read because you try to cram a
lot of code into a single line. Clarity is valued more than compactness.

Ex1 (line 214)

if (b != null && (b.length() > 0 || wasInsideString))
curArray.add(useObjects && !wasInsideString && b.equals("NULL") ? null :
b);

Ex2 (line 348)
oa[length++] = multi && v != null ? buildArray((PgArrayList) v, 0, -1) :
(v == null ? null : connection.getTimestampUtils().toTime(null, (String)
v));

6) I was unable to recursively retrieve multidimensional arrays as
ResultSets as I thought I should be able to do (in the attached test).
Shouldn't you retain an array as the base type for the top level of a
multi-dimensional array and expose a dimension at a time via each
ResultSet?

Kris Jurka

Marek Lewczuk wrote:
>
>> 3) When determing if NULL in an array string is a null value, you need
>> to check the server version. Prior to 8.2 an unadorned NULL is the
>> text "null", not an actual null value.
> See line 106 (of the attached AbstractJdbc2Array.java) - I'm checking,
> whether Object[] should be used instead of primitive arrays. It also
> used to check, whether NULL elements can be used. Now, see line 230/231
> - at this point, I'm checking, whether element is a text "NULL" or null
> element - it works just fine.

Perhaps that's OK. If you asked for primitive types on an int[] array
with a NULL value, you could want zero instead of an error trying to
convert the String "NULL" to an int, similar to rs.getInt() on a NULL
value. Let's leave this for now and work on fixing/writing new tests
and #6 and we can revisit this later.

>> 4) Shouldn't toString(PgArrayList) be PgArrayList.toString() ?
> It could be, but see line 598 (of the attached AbstractJdbc2Array.java)
> - I'm using escapeString, that throws SQLException and I cannot declare
> PgArrayList.toString() as throwing SQLException (super class declaration
> doesn't allow for that) - I could of course catch SQLException but it
> think it is better to stay with current implementation.

You're right, it's fine.

>> 6) I was unable to recursively retrieve multidimensional arrays as
>> ResultSets as I thought I should be able to do (in the attached test).
>> Shouldn't you retain an array as the base type for the top level of a
>> multi-dimensional array and expose a dimension at a time via each
>> ResultSet?
> You have made a mistake, please try attached ArrayTest.

Doh! OK, now it mostly works, but there's still an issue with setting
the basetype on a subarray to the base element type instead of an array
type, as attached. rs.getObject() (and metadata) are confused about
what the correct type is.

Kris Jurka pisze:
> Doh! OK, now it mostly works, but there's still an issue with setting
> the basetype on a subarray to the base element type instead of an array
> type, as attached. rs.getObject() (and metadata) are confused about
> what the correct type is.
I see the problem. I assume that we need to add support for array types,
which means that org.postgresql.core.Oid must have oid for every base
type array, e.g. _INT2 = 1005. It will be also required to add
appropriate data within org.postgresql.jdbc2.TypeInfoCache#types. Should
I do it ?

Regards,
Marek Lewczuk

On Fri, 26 Oct 2007, Marek Lewczuk wrote:

> I see the problem. I assume that we need to add support for array types,
> which means that org.postgresql.core.Oid must have oid for every base type
> array, e.g. _INT2 = 1005. It will be also required to add appropriate data
> within org.postgresql.jdbc2.TypeInfoCache#types. Should I do it ?
>

That doesn't sound right to me because we won't be able to put every
possible type (think about user defined) into the Oid class. Perhaps
getResultSet should convert getBaseTypeName() to oid instead of
getBaseType? Then you just need to know if your output is an array or not
(by checking isMultiDimensional) to know whether you want the oid for type
or _type.

Kris Jurka

Kris Jurka pisze:
>
>
> That doesn't sound right to me because we won't be able to put every
> possible type (think about user defined) into the Oid class. Perhaps
I was thinking only about base types (they are already in Oid class),
because it will work much faster if they will be statically written in
Oid class.

> getResultSet should convert getBaseTypeName() to oid instead of
> getBaseType? Then you just need to know if your output is an array or
> not (by checking isMultiDimensional) to know whether you want the oid
> for type or _type.
I still think that we should add base type arrays into Oid class - it
will work much faster and will be appropriate cause Oid class already
contains base types, so it is logical to put there base type arrays too.
For user defined types I would provide a way to fetch oid from pg_type -
but for now user defined types are not supported. However, if you really
think that we should fetch oid for every array type then I'm able to do
it but in my opinion we should stick with Oid class for now (only for
base types).

Regards,
ML

On Fri, 26 Oct 2007, Marek Lewczuk wrote:

> I still think that we should add base type arrays into Oid class - it will
> work much faster and will be appropriate cause Oid class already contains
> base types, so it is logical to put there base type arrays too. For user
> defined types I would provide a way to fetch oid from pg_type - but for now
> user defined types are not supported. However, if you really think that we
> should fetch oid for every array type then I'm able to do it but in my
> opinion we should stick with Oid class for now (only for base types).
>

This makes sense to me, static data for known types, dynamic for unknown.

Kris Jurka

Kris Jurka pisze:
>
> This makes sense to me, static data for known types, dynamic for unknown.
Great, I will prepare appropriate patches and send them tomorrow.

Kris Jurka pisze:
> This makes sense to me, static data for known types, dynamic for unknown.
>
Hi Kris,
in the attachment you can find jdbc.patch (that includes patches for a
few classes). You will see that I've added base types array types and
right now Array implementation works as should (your last test case is
working, which means that when using Array.getResultSet() returns
appropriate Array.getBaseType() and Array.getBaseTypeName()). However
those changes are not enough, there has to be more to be done in order
to provide support for user defined types, but I think that should be
done later. I also think that class TypeInfoCache must be absolutely
rewritten, cause it is not prepared for user defined types.

Regards,
ML

Kris Jurka pisze:
> This makes sense to me, static data for known types, dynamic for unknown.
In the attachment you can find the latest patch (in my previous post
I've attached wrong file).

Regards,
ML

On Tue, 30 Oct 2007, Marek Lewczuk wrote:

> in the attachment you can find jdbc.patch (that includes patches for a few
> classes). You will see that I've added base types array types and right now
> Array implementation works as should (your last test case is working, which
> means that when using Array.getResultSet() returns appropriate
> Array.getBaseType() and Array.getBaseTypeName()). However those changes are
> not enough, there has to be more to be done in order to provide support for
> user defined types, but I think that should be done later. I also think that
> class TypeInfoCache must be absolutely rewritten, cause it is not prepared
> for user defined types.
>

Attached is a revised version of your patch after I worked on it a bit.
Things I've changed:

1) Looking up the array type for a base type by using typelem doesn't work
because typelem is not unique. Since no code was calling getPGTypeArray,
I just removed this code.

2) Fixed driver code that was relying on the array return type in
AbstractJdbc2DatabaseMetaData.

3) Fixed the regression tests and added new ones to verify that array
handling works.

4) Changed Array.getResultSet to use generic coding rather than hardcoding
type oids for each java.sql.Types value. This allows getResultSet to be
used on types that aren't known to the driver, like aclitem[]. You still
can't call getArray on this type, but we're getting closer.

5) Array.getResultSet didn't respect index or offset parameters.

6) For TypeInfoCache I've added a new column to link the base and array
types instead of duplicating the whole rows.

7) Code did not correctly handle a string value of NULL in arrays for 8.1
and earlier server versions.

So with these changes, I think the code is pretty much ready to go, with
the exception of how multidimensional results are returned. The way you
wrap the arrays in Object[] makes it impossible to cast to things like
Integer[][]. Also I think we should be able to return multidimensional
arrays of primitive type if the "compatible" option is set to 8.2. I
think you should be using java.lang.reflect.Array#newInstance to create
the arrays you are returning to get the correct type instead of building
them up incrementally.

Kris Jurka

On Wed, Nov 21, 2007 at 05:49:28PM -0500, Kris Jurka wrote:
>
>
> On Tue, 30 Oct 2007, Marek Lewczuk wrote:
>
> >in the attachment you can find jdbc.patch (that includes patches for a few
> >classes). You will see that I've added base types array types and right
> >now Array implementation works as should (your last test case is working,
> >which means that when using Array.getResultSet() returns appropriate
> >Array.getBaseType() and Array.getBaseTypeName()). However those changes
> >are not enough, there has to be more to be done in order to provide
> >support for user defined types, but I think that should be done later. I
> >also think that class TypeInfoCache must be absolutely rewritten, cause it
> >is not prepared for user defined types.
> >
>
> Attached is a revised version of your patch after I worked on it a bit.
> Things I've changed:
>
> 1) Looking up the array type for a base type by using typelem doesn't work
> because typelem is not unique. Since no code was calling getPGTypeArray,
> I just removed this code.
>
> 2) Fixed driver code that was relying on the array return type in
> AbstractJdbc2DatabaseMetaData.
>
> 3) Fixed the regression tests and added new ones to verify that array
> handling works.
>
> 4) Changed Array.getResultSet to use generic coding rather than hardcoding
> type oids for each java.sql.Types value. This allows getResultSet to be
> used on types that aren't known to the driver, like aclitem[]. You still
> can't call getArray on this type, but we're getting closer.
>
> 5) Array.getResultSet didn't respect index or offset parameters.
>
> 6) For TypeInfoCache I've added a new column to link the base and array
> types instead of duplicating the whole rows.
>
> 7) Code did not correctly handle a string value of NULL in arrays for 8.1
> and earlier server versions.
>
> So with these changes, I think the code is pretty much ready to go, with
> the exception of how multidimensional results are returned. The way you
> wrap the arrays in Object[] makes it impossible to cast to things like
> Integer[][]. Also I think we should be able to return multidimensional
> arrays of primitive type if the "compatible" option is set to 8.2. I
> think you should be using java.lang.reflect.Array#newInstance to create
> the arrays you are returning to get the correct type instead of building
> them up incrementally.
>
> Kris Jurka

Kris/Marek: I've attached a very small followup patch which addresses a
need I had: I was using java.sql.DatabaseMetaData's getColumns method to
attempt to get the COLUMN_SIZE attribute for a column of type "character
varying(255) []". Without my patch (applied after Kris's patches) the
COLUMN_SIZE would be 2147483647, with the patch it would be 255. I am
by no means a JDBC or Postgresql expert, so maybe I am way off base.
What do you think?

Eric.

On Thu, 22 Nov 2007, Eric Lenio wrote:

> Kris/Marek: I've attached a very small followup patch which addresses a
> need I had: I was using java.sql.DatabaseMetaData's getColumns method to
> attempt to get the COLUMN_SIZE attribute for a column of type "character
> varying(255) []". Without my patch (applied after Kris's patches) the
> COLUMN_SIZE would be 2147483647, with the patch it would be 255. I am
> by no means a JDBC or Postgresql expert, so maybe I am way off base.
> What do you think?
>

Looks reasonable, but way too specific. We don't want to do that for each
array type. At the top of each of the methods that have the switch
statements we should have something that checks to see if it is an array
type and converts the oid to the oid of the base type.

Kris Jurka

Kris Jurka pisze:
> Attached is a revised version of your patch after I worked on it a bit.
> Things I've changed:
>
> 1) Looking up the array type for a base type by using typelem doesn't
> work because typelem is not unique. Since no code was calling
> getPGTypeArray, I just removed this code.
Agree.

>
> 3) Fixed the regression tests and added new ones to verify that array
> handling works.
I wanted to do it, but I was waiting for you comments about my code.
Thanks for that.

> 4) Changed Array.getResultSet to use generic coding rather than
> hardcoding type oids for each java.sql.Types value. This allows
> getResultSet to be used on types that aren't known to the driver, like
> aclitem[]. You still can't call getArray on this type, but we're
> getting closer.
Agree.

>
> 5) Array.getResultSet didn't respect index or offset parameters.
Sorry, my mistake.

>
> 6) For TypeInfoCache I've added a new column to link the base and array
> types instead of duplicating the whole rows.
Agree.

>
> 7) Code did not correctly handle a string value of NULL in arrays for
> 8.1 and earlier server versions.
I was concentrate on make it working for >= 8.2 - again my mistake, sorry.

> So with these changes, I think the code is pretty much ready to go, with
> the exception of how multidimensional results are returned. The way you
> wrap the arrays in Object[] makes it impossible to cast to things like
> Integer[][]. Also I think we should be able to return multidimensional
> arrays of primitive type if the "compatible" option is set to 8.2. I
> think you should be using java.lang.reflect.Array#newInstance to create
> the arrays you are returning to get the correct type instead of building
> them up incrementally.
Well, I aware that java.lang.reflect.Array#newInstance can be used
however I didn't know for what versions of JDK reflect package can be
used - if you have mentioned about it so I guess that I can - in the
attachment you can find AbstractJdbc2Array patch - please review it.

I one to discuss one more thing - see below lines in the AbstractJdb2Array:
this.useObjects = connection.haveMinimumCompatibleVersion("8.3");
this.haveMinServer82 = connection.haveMinimumServerVersion("8.2");

We must clarify, when to use objects instead of primitive types ? Can
you describe it ?

Regards,
Marek

Marek Lewczuk pisze:
> Kris Jurka pisze:
Argh... sorry for English mistakes. I type to fast and sometimes I don't
pay attention to typos and grammar mistakes. Sorry...

> I was concentrate on make it working for >= 8.2 - again my mistake, sorry.
I was concentrated on doing it for...

> Well, I aware that java.lang.reflect.Array#newInstance can be used
I'm aware...

> I one to discuss one more thing - see below lines in the AbstractJdb2Array:
I want to discuss one...

Best wishes,
Marek

On Thu, 22 Nov 2007, Marek Lewczuk wrote:

> Well, I aware that java.lang.reflect.Array#newInstance can be used
> however I didn't know for what versions of JDK reflect package can be used -
> if you have mentioned about it so I guess that I can - in the attachment you
> can find AbstractJdbc2Array patch - please review it.

Doesn't seem to work. The attached case fails in two different ways
depending on whether compatible is set or not. The JDK methods you can
use depends on where the code you are adding goes. In this case you're
adding code to a JDBC2 class, so you are restricted to JDK1.2 code. JDBC3
-> 1.4, JDBC3g -> 1.5, JDBC4 -> 1.6.

> I one to discuss one more thing - see below lines in the AbstractJdb2Array:
> this.useObjects = connection.haveMinimumCompatibleVersion("8.3");
> this.haveMinServer82 = connection.haveMinimumServerVersion("8.2");
>
> We must clarify, when to use objects instead of primitive types ? Can you
> describe it ?
>

Right now you use objects if either dims > 1 or useObjects. I'm
suggesting that we should only use objects if useObjects. That way people
who want to get int[][] returned can get that via setting compatible=8.2.

Kris Jurka

Kris Jurka pisze:
> Doesn't seem to work. The attached case fails in two different ways
> depending on whether compatible is set or not. The JDK methods you can
> use depends on where the code you are adding goes. In this case you're
> adding code to a JDBC2 class, so you are restricted to JDK1.2 code.
> JDBC3 -> 1.4, JDBC3g -> 1.5, JDBC4 -> 1.6.
It should work right now (I've tested it using your examples). As for
java.lang.reflect package it was added in JDK1.1, so it should work for
all supported Java versions.

> Right now you use objects if either dims > 1 or useObjects. I'm
> suggesting that we should only use objects if useObjects. That way
> people who want to get int[][] returned can get that via setting
> compatible=8.2.
Well, my implementation works as you described, have a look at
Boolean/boolean:
if (dims > 1 || useObjects) // if more than one dimension or objects
{
// here I check if more than one dimension and if so
// I check for useObjects - if true, then Boolean[] if false then
boolean[]
ret = oa = (dims > 1 ? (Object[])
java.lang.reflect.Array.newInstance(useObjects ? Boolean.class :
boolean.class, dimsLength) : new Boolean[count]);
}

else // if one dimension primitive array
{
ret = pa = new boolean[count];
}

In the attachment you can find appropriate patch. Please review it
again. Btw, you didn't answer about
ResultSetMetaData.getColumnTypeName() - does it mean, that we won't do
anything with that in near future ?

Best wishes,
Marek

On Tue, 27 Nov 2007, Marek Lewczuk wrote:

> [one more patch for multi-dimensional arrays]
>

Applied with one last minor fix. Array elements are escaped differently
than literals or identifiers, so I've added a new escapeArrayElement
method instead of trying to use connection.escapeString.

> Btw, you didn't answer about ResultSetMetaData.getColumnTypeName() - does it
> mean, that we won't do anything with that in near future ?
>

I don't have any immediate plans to work on this issue. The 8.3 release
is coming fast and there are two more large patches that I need to have
another look at (copy + binary transfer). Schema qualifying all types has
been a problem since the 7.3 release and we haven't heard a whole lot of
complaints about it, so I'm not hugely concerned. That's not to say I
don't think we should fix it, just that it's low on my priority list. If
you want to work on it, I'd be happy to look at whatever you come up with.

Kris Jurka

Kris Jurka pisze:
> Applied with one last minor fix. Array elements are escaped differently
> than literals or identifiers, so I've added a new escapeArrayElement
> method instead of trying to use connection.escapeString.
Great, thanks.

> I don't have any immediate plans to work on this issue. The 8.3 release
> is coming fast and there are two more large patches that I need to have
> another look at (copy + binary transfer). Schema qualifying all types
> has been a problem since the 7.3 release and we haven't heard a whole
> lot of complaints about it, so I'm not hugely concerned. That's not to
> say I don't think we should fix it, just that it's low on my priority
> list. If you want to work on it, I'd be happy to look at whatever you
> come up with.
Ok, I will try to prepare my proposal. If there will be other things,
that I could help please write me an email.

Best wishes,
ML

On Thu, 22 Nov 2007, Eric Lenio wrote:

> Kris/Marek: I've attached a very small followup patch which addresses a
> need I had: I was using java.sql.DatabaseMetaData's getColumns method to
> attempt to get the COLUMN_SIZE attribute for a column of type "character
> varying(255) []". Without my patch (applied after Kris's patches) the
> COLUMN_SIZE would be 2147483647, with the patch it would be 255. I am
> by no means a JDBC or Postgresql expert, so maybe I am way off base.
> What do you think?
>

I've applied a patch to CVS to do this along the lines I suggested in the
thread.

Kris Jurka