Hi Tom, Gordon, Peter,
Please find attached the final changes that I intend to checkin for your
review. I am working on a test case which I will send out in a short while.
I have added another method to Expression so as to not change the public
API. Added comemnts around type in ParameterExpression to indicate that
it might not be always available.
Adding inline what we discussed over phone for benefit of others....
Peter Krogh wrote:
> Thanks Mitesh,
>
> I have a few concerns with this fix.
> 1. Public API has been changed (getParameter). We can't do that. A new get parameter must be added that takes the third argument.Indicate that it is internal using the INTERNAL: convention in the javadocs
>
Done.
> 2. The next concern is that type is now used everywhere, and people will expect it to be set all the time. I doubt that it will be. I think that this can be derisked by renaming the variable, or comments.
>
Done.
> 3. I am honestly quite concerned about the change that will change what we print everytime we bind a string in DB2 and Derby. CAST (? AS VARCHAR(32672)). I would much rather see a change for this Derby work around issolated to Concat. What is the expected impact on CTS? All strings will now be printed this way, is that expected to work?
>
This workaround was suggested to us by DB2 driver team. We had been
using this kind of CAST for our cmp2.x impl and have not seen any issue
so far.
> 4. Have you tested this fix with binding off? Will the SQL generated look like: "VARCHAR( 'hi ' || 'there')"
>
Yes. The SQL would like above and yes the SQL would execute without any
issues.
Thanks,
Mitesh
>
> I am wondering if non-bound values support the CAST (...) function? (like this: CAST ('hi' AS VARCHAR(32672)) || CAST ('there' AS VARCHAR(32672). ). If so, then maybe issolating this change to the CONCAT function is better than changing the way EVERY string is bound in DB2 and Derby.
>
> If this is the change, you could simply change the concat expression operator to include the CAST in it and not change all the binding of strings. I realize that your fix opens the door for other CASTable types, but I am not sure that this is the right point to be adding this type of wide sweeping change.
>
>
> -----Original Message-----
> From: Mitesh Meswani [mailto:mitesh.meswani_at_Sun.COM]
> Sent: Friday, March 17, 2006 2:54 AM
> To: persistence_at_glassfish.dev.java.net
> Subject: Fix for issue 400
>
>
> Attached code fixes issue 400. I still need to do some work/testing
> before I can send this out for final review.
>
> Michael,
> As you had suspected, the type information of parameter is not
> propagated down to ParamaterExpression. I put together a primitive
> mechanism to propagate this information.
> -ParameterNode#generateExpression now passes getType() to
> builder.getParameter
> - Expression.getParameter now takes an extra argument "Object type"
> which is passed and stored in ParameterExpression
>
> I consume the type in DB2Platform#writeParameterMarker. I am currently
> assuming that type is a Class object and compare it to String.Class to
> generate appropriate cast. Questions to you:
> What are the possible type that a node can take?
> Do you see any possible issues/improvements to the type propagation
> mechanism above ?
>
> Thanks,
> Mitesh
>
>
>