Re: jdbc cts final diff for review

Oliver

regarding the real/double muck below

They have a "real table" which ostensibly holds REAL values, however

one test does the following

takes the max value from the int_table which is a large integer, then
stores it into the 'REAL' table and then reads it back as an integer

this was failing due to precision issues until I change the
underlying type of the REAL table to FLOAT8 (which is what Oracle,
and Mysql do )

then they have another test

which registers an out parameter as a REAL and reads from the
'REAL_TABLE' which is now uses the underlying type double

if someone else can find a way to get this to pass, I'm all ears.

Dave
On 29-Jun-05, at 10:28 AM, Dave Cramer wrote:

>
> On 29-Jun-05, at 9:17 AM, Oliver Jowett wrote:
>
>> Dave Cramer wrote:
>>
>>> Attached is the patch for review. QueryExecutor is unchanged now.
>>> The only iffy bit is where I check for the in parameter being
>>> bound to void type. It could be done in checkAllParametersSet
>>> I'd like to commit this shortly.
>>>
>>
>> Ok, comments from just reading the patch.. I like the approach
>> much better than the previous one, but the details need some cleanup.
>>
>> Rather than putting knowledge of parameter direction into
>> ParameterList, how about just allowing access to the type OID, and
>> the caller checks for Oid.VOID? Then the ParameterList interface
>> changes less and the change to use a Parameter class is
>> unnecessary as the list doesn't need to store a direction value.
>>
>> If you must store a direction value in the list, then another
>> array might be better than wrapping everything up in an object
>> (generates less garbage overall). Also, there are a few places
>> that return magic values for the direction that should be using
>> the symbolic constants you've defined elsewhere.
> I thought of this, however even arrays have to be garbage
> collected.. Is there really much difference internally between new
> Parameter and new int?
>
> I'm ambivalent though it's not a big change either way.
>>
>> The change to checkAllParametersSet to not check OUT parameters
>> seems unnecessary -- won't OUT parameters be set to the (non-null)
>> NULL_OBJECT anyway?
>>
>>
>>> ! // this is here for the sole purpose of
>>> passing the cts
>>> ! if ( columnType == Types.DOUBLE &&
>>> functionReturnType[i] == Types.REAL )
>>> ! {
>>> ! // return it as a float
>>> ! if ( callResult[i] != null)
>>> ! callResult[i] = new Float(((Double)
>>> callResult[i]).floatValue());
>>> ! }
>
> I'll go through my notes, but I can tell you it was a catch 22
> problem mostly likely an artifact of Oracle not having a REAL type.
>> We can't do that! If it's failing that's probably because we're
>> not doing the necessary implicit typecasts required by the spec..
>>
>> Please explain what's going on here:
>
> from an SQL point of view FLOAT, and double are FLOAT8 types, REAL
> is the only one that is FLOAT4
>>
>>
>>> + case Types.FLOAT: // TODO: FLOAT and
>>> REAL were FLOAT8 for the cts
>>>
>>
>>
>>> public void setFloat(int parameterIndex, float x) throws
>>> SQLException
>>> {
>>> checkClosed();
>>> ! bindLiteral(parameterIndex, Float.toString(x),
>>> Oid.FLOAT8);
>>> }
>>>
>>>
>>
>> (the bind changed from Oid.FLOAT4 to Oid.FLOAT8..)
>>
>> You seem to have reverted your earlier changes and put back the
>> types/* classes -- why?
> huh ? they should be in there in HEAD, I did remove the creation of
> an object, and went to static methods
>>
>> adjustParamIndex() should be removed entirely if it's now always a
>> no-op.
> yeah
>>
>> Why is type translation for JDBC2 types only done in the JDBC3
>> code (in registerOutParameter)? -- shouldn't that be in the JDBC2
>> code?
>>
> Good point
>> There are a bunch of gratuitous changes to the test code that
>> probably shouldn't be committed, e.g. in BatchExecuteTest:
>>
> agreed, they are for my own testing
>>
>>> + try
>>> + {
>>> + Class.forName("org.postgresql.Driver");
>>> + }
>>> + catch( Exception ex){}
>>>
>>
>> similarly in many other test classes.
>>
>> ....
>>
>> It might be an idea to break this up into "support new-style OUT
>> parameters" and "other fixes necessary to pass the CTS" as
>> currently it's quite unclear which is which..
> Well, they are all related OUT parameters are required to support
> the CTS
>>
>> -O
>>
>>
>