Thread: 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
/*-------------------------------------------------------------------------
*
* Copyright (c) 2004-2005, PostgreSQL Global Development Group
*
* IDENTIFICATION
* $PostgreSQL: pgjdbc/org/postgresql/jdbc2/AbstractJdbc2Array.java,v 1.19 2007/09/10 08:34:31 jurka Exp $
*
*-------------------------------------------------------------------------
*/
package org.postgresql.jdbc2;
import org.postgresql.core.*;
import org.postgresql.util.PSQLException;
import org.postgresql.util.PSQLState;
import org.postgresql.util.GT;
import java.math.BigDecimal;
import java.sql.ResultSet;
import java.sql.SQLException;
import java.sql.Types;
import java.util.ArrayList;
import java.util.Map;
import java.util.Vector;
/**
* Array is used collect one column of query result data.
*
* <p>Read a field of type Array into either a natively-typed
* Java array object or a ResultSet. Accessor methods provide
* the ability to capture array slices.
*
* <p>Other than the constructor all methods are direct implementations
* of those specified for java.sql.Array. Please refer to the javadoc
* for java.sql.Array for detailed descriptions of the functionality
* and parameters of the methods of this class.
*
* @see ResultSet#getArray
*/
public abstract class AbstractJdbc2Array {
/**
* Array list implementation specific for storing PG array elements.
*/
private static class PgArrayList extends ArrayList {
private static final long serialVersionUID = 2052783752654562677L;
/**
* Whether multi-dimensional array.
*/
boolean isMultiDimensional = false;
}
/**
* A database connection.
*/
private BaseConnection connection = null;
/**
* The ResultSet from which to get the data for this Array.
*/
private BaseResultSet resultSet;
/**
* The Field descriptor for the field to load into this Array.
*/
private Field field = null;
/**
* 1-based index of the query field to load into this Array.
*/
private int fieldIndex = 0;
/**
* Field value as String.
*/
private String fieldString = null;
/**
* Whether Object[] should be used instead primitive arrays. Object[] can
* contain null elements. It should be set to <Code>true</Code> if
* {@link BaseConnection#haveMinimumCompatibleVersion(String)} returns
* <Code>true</Code> for argument "8.2".
*/
private final boolean useObjects;
/**
* Create a new Array.
*
* @param connection a database connection
* @param index 1-based index of the query field to load into this Array
* @param field the Field descriptor for the field to load into this Array
* @param result the ResultSet from which to get the data for this Array
*/
public AbstractJdbc2Array (BaseConnection connection, int index, Field field, BaseResultSet result) throws
SQLException{
this.connection = connection;
this.field = field;
this.resultSet = result;
this.fieldIndex = index;
this.fieldString = result.getFixedString(index);
this.useObjects = connection.haveMinimumCompatibleVersion("8.2");
}
public Object getArray() throws SQLException {
return getArrayImpl(1, 0, null);
}
public Object getArray(long index, int count) throws SQLException {
return getArrayImpl(index, count, null);
}
public Object getArrayImpl(Map map) throws SQLException {
return getArrayImpl(1, 0, map);
}
public Object getArrayImpl (long index, int count, Map map) throws SQLException {
// for now maps aren't supported.
if (map != null && !map.isEmpty()) throw org.postgresql.Driver.notImplemented(this.getClass(),
"getArrayImpl(long,int,Map)");
// array index is out of range
if (index < 1) throw new PSQLException(GT.tr("The array index is out of range: {0}", new Long(index)),
PSQLState.DATA_ERROR);
PgArrayList array = buildArrayList(fieldString);
if (count == 0) count = array.size();
// array index out of range
if ((--index) + count > array.size()) throw new PSQLException(GT.tr("The array index is out of range: {0},
numberof elements: {1}.", new Object[]{new Long(index + count), new Long(array.size())}), PSQLState.DATA_ERROR);
return buildArray(array, (int) index, count);
}
/**
* Build {@link ArrayList} from String input.
*
* @param list String representation of an array
*/
private PgArrayList buildArrayList (String list) {
PgArrayList array = new PgArrayList();
if (list != null) {
char[] chars = list.toCharArray();
StringBuffer buffer = null;
boolean insideString = false;
boolean wasInsideString = false; // needed for checking if NULL value occured
Vector dims = new Vector(); // array dimension arrays
PgArrayList curArray = array; // currently processed array
// Starting with 8.0 non-standard (beginning index
// isn't 1) bounds the dimensions are returned in the
// data formatted like so "[0:3]={0,1,2,3,4}".
// Older versions simply do not return the bounds.
//
// Right now we ignore these bounds, but we could
// consider allowing these index values to be used
// even though the JDBC spec says 1 is the first
// index. I'm not sure what a client would like
// to see, so we just retain the old behavior.
int startOffset = 0;
{
if (chars[0] == '[') {
while (chars[startOffset] != '=') {
startOffset++;
}
startOffset++; // skip =
}
}
for ( int i = startOffset; i < chars.length; i++ ) {
//escape character that we need to skip
if (chars[i] == '\\') i++;
// subarray start
else if (!insideString && chars[i] == '{' ) {
if (dims.size() == 0) dims.add(array);
else {
PgArrayList a = new PgArrayList();
PgArrayList p = ((PgArrayList) dims.lastElement());
{
p.add(a);
p.isMultiDimensional = true;
}
dims.add(a);
}
curArray = (PgArrayList) dims.lastElement();
buffer = new StringBuffer();
continue;
}
// quoted element
else if (chars[i] == '"') {
insideString = !insideString;
wasInsideString = true;
continue;
}
// white space
else if (!insideString && Character.isWhitespace(chars[i])) {
continue;
}
// array end or element end
else if ((!insideString && (chars[i] == ',' || chars[i] == '}')) || i == chars.length - 1) {
if ( chars[i] != '"' && chars[i] != '}' && chars[i] != ',' && buffer != null)
buffer.append(chars[i]);
String b = buffer == null ? null : buffer.toString();
if (b != null && (b.length() > 0 || wasInsideString)) curArray.add(useObjects && !wasInsideString
&&b.equals("NULL") ? null : b);
wasInsideString = false;
buffer = new StringBuffer();
if (chars[i] == '}') {
dims.remove(dims.size() - 1);
if (dims.size() > 0) curArray = (PgArrayList) dims.lastElement();
buffer = null;
}
continue;
}
if (buffer != null) buffer.append( chars[i] );
}
}
return array;
}
/**
* Convert {@link ArrayList} to array.
*
* @param input list to be converted into array
*/
private Object buildArray (PgArrayList input, int index, int count) throws SQLException {
if (count == -1) count = input.size();
// array to be returned
Object ret = null;
// whether multi-dimensional array
boolean multi = input.isMultiDimensional;
// array elements counter
int length = 0;
// array type
final int type = getBaseType();
if (type == Types.BIT) {
boolean[] pa = null;
Object[] oa = null;
if (multi || useObjects) ret = oa = (multi ? new Object[count] : new Boolean[count]);
else ret = pa = new boolean[count];
for ( ; count > 0; count--) {
Object o = input.get(index++);
if (multi || useObjects) oa[length++] = o == null ? null : (multi ? buildArray((PgArrayList) o, 0, -1)
:new Boolean(AbstractJdbc2ResultSet.toBoolean((String) o)));
else pa[length++] = o == null ? false : AbstractJdbc2ResultSet.toBoolean((String) o);
}
}
else if (type == Types.SMALLINT || type == Types.INTEGER) {
int[] pa = null;
Object[] oa = null;
if (multi || useObjects) ret = oa = (multi ? new Object[count] : new Integer[count]);
else ret = pa = new int[count];
for ( ; count > 0; count--) {
Object o = input.get(index++);
if (multi || useObjects) oa[length++] = o == null ? null : (multi ? buildArray((PgArrayList) o, 0, -1)
:new Integer(AbstractJdbc2ResultSet.toInt((String) o)));
else pa[length++] = o == null ? 0 : AbstractJdbc2ResultSet.toInt((String) o);
}
}
else if (type == Types.BIGINT) {
long[] pa = null;
Object[] oa = null;
if (multi || useObjects) ret = oa = (multi ? new Object[count] : new Long[count]);
else ret = pa = new long[count];
for ( ; count > 0; count--) {
Object o = input.get(index++);
boolean p = false;
if (multi || useObjects) oa[length++] = o == null ? null : (multi ? buildArray((PgArrayList) o, 0, -1)
:new Long(AbstractJdbc2ResultSet.toLong((String) o)));
else pa[length++] = o == null ? 0l : AbstractJdbc2ResultSet.toLong((String) o);
}
}
else if (type == Types.NUMERIC) {
Object[] oa = null;
ret = oa = (multi ? new Object[count] : new BigDecimal[count]);
for ( ; count > 0; count--) {
Object v = input.get(index++);
oa[length++] = multi && v != null ? buildArray((PgArrayList) v, 0, -1) : (v == null ? null :
AbstractJdbc2ResultSet.toBigDecimal((String)v, -1));
}
}
else if (type == Types.REAL) {
float[] pa = null;
Object[] oa = null;
if (multi || useObjects) ret = oa = (multi ? new Object[count] : new Float[count]);
else ret = pa = new float[count];
for ( ; count > 0; count--) {
Object o = input.get(index++);
boolean p = false;
if (multi || useObjects) oa[length++] = o == null ? null : (multi ? buildArray((PgArrayList) o, 0, -1)
:new Float(AbstractJdbc2ResultSet.toFloat((String) o)));
else pa[length++] = o == null ? 0f : AbstractJdbc2ResultSet.toFloat((String) o);
}
}
else if (type == Types.DOUBLE) {
double[] pa = null;
Object[] oa = null;
if (multi || useObjects) ret = oa = (multi ? new Object[count] : new Double[count]);
else ret = pa = new double[count];
for ( ; count > 0; count--) {
Object o = input.get(index++);
boolean p = false;
if (multi || useObjects) oa[length++] = o == null ? null : (multi ? buildArray((PgArrayList) o, 0, -1)
:new Double(AbstractJdbc2ResultSet.toDouble((String) o)));
else pa[length++] = o == null ? 0d : AbstractJdbc2ResultSet.toDouble((String) o);
}
}
else if (type == Types.CHAR || type == Types.VARCHAR) {
Object[] oa = null;
ret = oa = (multi ? new Object[count] : new String[count]);
for ( ; count > 0; count--) {
Object v = input.get(index++);
oa[length++] = multi && v != null ? buildArray((PgArrayList) v, 0, -1) : v;
}
}
else if (type == Types.DATE) {
Object[] oa = null;
ret = oa = (multi ? new Object[count] : new java.sql.Date[count]);
for ( ; count > 0; count--) {
Object v = input.get(index++);
oa[length++] = multi && v != null ? buildArray((PgArrayList) v, 0, -1) : (v == null ? null :
connection.getTimestampUtils().toDate(null,(String) v));
}
}
else if (type == Types.TIME) {
Object[] oa = null;
ret = oa = (multi ? new Object[count] : new java.sql.Time[count]);
for ( ; count > 0; count--) {
Object v = input.get(index++);
oa[length++] = multi && v != null ? buildArray((PgArrayList) v, 0, -1) : (v == null ? null :
connection.getTimestampUtils().toTime(null,(String) v));
}
}
else if (type == Types.TIMESTAMP) {
Object[] oa = null;
ret = oa = (multi ? new Object[count] : new java.sql.Timestamp[count]);
for ( ; count > 0; count--) {
Object v = input.get(index++);
oa[length++] = multi && v != null ? buildArray((PgArrayList) v, 0, -1) : (v == null ? null :
connection.getTimestampUtils().toTimestamp(null,(String) v));
}
}
// other datatypes not currently supported
else {
if (connection.getLogger().logDebug()) connection.getLogger().debug("getArrayImpl(long,int,Map) with " +
getBaseTypeName());
throw org.postgresql.Driver.notImplemented(this.getClass(), "getArrayImpl(long,int,Map)");
}
return ret;
}
public int getBaseType () throws SQLException {
return connection.getSQLType(getBaseTypeName());
}
public String getBaseTypeName () throws SQLException {
String t = connection.getPGType(field.getOID());
if (t.charAt(0) == '_') t = t.substring(1);
return t;
}
public java.sql.ResultSet getResultSet () throws SQLException {
return getResultSetImpl(1, 0, null);
}
public java.sql.ResultSet getResultSet (long index, int count) throws SQLException {
return getResultSetImpl(index, count, null);
}
public java.sql.ResultSet getResultSetImpl (Map map) throws SQLException {
return getResultSetImpl(1, 0, map);
}
public ResultSet getResultSetImpl (long index, int count, Map map) throws SQLException {
// for now maps aren't supported.
if (map != null && !map.isEmpty()) throw org.postgresql.Driver.notImplemented(this.getClass(),
"getResultSetImpl(long,int,Map)");
// array index is out of range
if (index < 1) throw new PSQLException(GT.tr("The array index is out of range: {0}", new Long(index)),
PSQLState.DATA_ERROR);
PgArrayList list = buildArrayList(fieldString);
if (count == 0) count = list.size();
// array index out of range
if ((--index) + count > list.size()) throw new PSQLException(GT.tr("The array index is out of range: {0},
numberof elements: {1}.", new Object[]{new Long(index + count), new Long(list.size())}), PSQLState.DATA_ERROR);
Vector rows = new Vector();
// array type
final int type = getBaseType();
Field[] fields = new Field[2];
{
fields[0] = new Field("INDEX", Oid.INT2);
if (type == Types.BIT) fields[1] = new Field("VALUE", Oid.BOOL);
else if (type == Types.SMALLINT) fields[1] = new Field("VALUE", Oid.INT2);
else if (type == Types.INTEGER) fields[1] = new Field("VALUE", Oid.INT4);
else if (type == Types.BIGINT) fields[1] = new Field("VALUE", Oid.INT8);
else if (type == Types.NUMERIC) fields[1] = new Field("VALUE", Oid.NUMERIC);
else if (type == Types.REAL) fields[1] = new Field("VALUE", Oid.FLOAT4);
else if (type == Types.DOUBLE) fields[1] = new Field("VALUE", Oid.FLOAT8);
else if (type == Types.CHAR) fields[1] = new Field("VALUE", Oid.BPCHAR);
else if (type == Types.VARCHAR) fields[1] = new Field("VALUE", Oid.VARCHAR);
else if (type == Types.DATE) fields[1] = new Field("VALUE", Oid.DATE);
else if (type == Types.TIME) fields[1] = new Field("VALUE", Oid.TIME);
else if (type == Types.TIMESTAMP) fields[1] = new Field("VALUE", Oid.TIMESTAMPTZ);
// other data types not currently supported
else {
if (connection.getLogger().logDebug()) connection.getLogger().debug("getResultSetImpl(long,int,Map)
with" + getBaseTypeName());
throw org.postgresql.Driver.notImplemented(this.getClass(), "getResultSetImpl(long,int,Map)");
}
}
if (list.isMultiDimensional) {
for (int i = 0; i < list.size(); i++) {
byte[][] t = new byte[2][0];
Object v = list.get(i);
t[0] = connection.encodeString(Integer.toString(i + 1));
t[1] = connection.encodeString(v == null ? "NULL" : toString((PgArrayList) v));
rows.add(t);
}
}
else {
for (int i = 0; i < list.size(); i++) {
byte[][] t = new byte[2][0];
String v = (String) list.get(i);
t[0] = connection.encodeString(Integer.toString(i + 1));
t[1] = connection.encodeString(v == null ? "NULL" : v);
rows.add(t);
}
}
BaseStatement stat = (BaseStatement) connection.createStatement(ResultSet.TYPE_SCROLL_INSENSITIVE,
ResultSet.CONCUR_READ_ONLY);
return (ResultSet) stat.createDriverResultSet(fields, rows);
}
public String toString () {
return fieldString;
}
/**
* Convert array list to PG String representation (e.g. {0,1,2}).
*/
private String toString (PgArrayList list) throws SQLException {
StringBuffer b = new StringBuffer();
b.append('{');
for (int i = 0; i < list.size(); i++) {
Object v = list.get(i);
if (i > 0) b.append(',');
if (v == null) b.append("NULL");
else if (v instanceof PgArrayList) b.append(toString((PgArrayList) v));
else b.append('"').append(connection.escapeString((String) v)).append('"');
}
b.append('}');
return b.toString();
}
}
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
Attachment
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.
import java.sql.*;
public class ArrayTest {
public static void main(String args[]) throws Exception {
Class.forName("org.postgresql.Driver");
Connection conn = DriverManager.getConnection("jdbc:postgresql://localhost:5830/jurka","jurka","");
Statement stmt = conn.createStatement();
ResultSet rs = stmt.executeQuery("SELECT '{{1,2,3},{4,5,6}}'::int[]");
rs.next();
Array arr = rs.getArray(1);
System.out.println(arr.getBaseType());
System.out.println(arr.getBaseTypeName());
ResultSet arrRS = arr.getResultSet();
arrRS.next();
System.out.println(arrRS.getString(2));
Object o1 = arrRS.getObject(2);
System.out.println(o1);
Array a1 = arrRS.getArray(2);
ResultSet rs1 = a1.getResultSet();
while (rs1.next()) {
System.out.println(rs1.getInt(1));
}
rs1.close();
arrRS.next();
System.out.println(arrRS.getString(2));
Array a2 = arrRS.getArray(2);
System.out.println(a2.getBaseTypeName());
System.out.println(a2.getBaseType());
ResultSet rs2 = a2.getResultSet();
while (rs2.next()) {
System.out.println(rs2.getInt(1));
}
rs2.close();
arrRS.close();
rs.close();
stmt.close();
conn.close();
}
}
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
Attachment
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
Attachment
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
Attachment
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.
Attachment
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
Attachment
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
Attachment
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
Attachment
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