Re: jdbc cts final diff for review

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
>
>