Re: jdbc cts final diff for review - Mailing list pgsql-jdbc

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.

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());
> !                     }

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:

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

adjustParamIndex() should be removed entirely if it's now always a no-op.

Why is type translation for JDBC2 types only done in the JDBC3 code (in
registerOutParameter)? -- shouldn't that be in the JDBC2 code?

There are a bunch of gratuitous changes to the test code that probably
shouldn't be committed, e.g. in BatchExecuteTest:

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

-O