Re: sql_type_cast small inconsistency and bugs

Tim Bunce wrote:
> On Thu, Nov 26, 2009 at 07:47:45PM +0000, Martin J. Evans wrote:
>> Martin Evans wrote:
>>> Tim,
>>>
>>> I'm not sure if you are bothered by this but there appears to be a small
>>> inconsistency between SQL_INTEGER/SQL_NUMERIC and SQL_DOUBLE handling in
>>> sql_type_cast:
>>>
>>> sql_type_cast("aa", SQL_INTEGER, 0)
>>>   returns 1 (no cast not strict)
>>> sql_type_casr("aa", SQL_INTEGER, DBIstcf_STRICT)
>>>   returns 0 (no cast, strict)
>>> SQL_NUMERIC works as above (with a fix to the grok stuff)
>>>
>>> but
>>>
>>> sql_type_cast("aa", SQL_DOUBLE, 0)
>>>   returns 2 (cast ok) I expected 1
>>> sql_type_cast("aa", SQL_DOUBLE, DBIstcf_STRICT)
>>>   returns 2 (cast ok) I expected 0
>>>
>>> As you point out in the code if warnings are enabled you get a warning
>>> but you don't get the expected return.
>> Would you have any objections to me changing:
>>
>>     case SQL_DOUBLE:
>>         sv_2nv(sv);
>>         /* SvNOK should be set but won't if sv is not numeric (in which
>>          * case perl would have warn'd already if -w or warnings are in
>> effect)
>>          */
>>         cast_ok = SvNOK(sv);
>>         break;
>>
>> to
>>
>>     case SQL_DOUBLE:
>>       if (looks_like_number(sv)) {
>>         sv_2nv(sv);
>>         /* SvNOK should be set but won't if sv is not numeric (in which
>>          * case perl would have warn'd already if -w or warnings are in
>> effect)
>>          */
>>         cast_ok = SvNOK(sv);
>>       } else {
>>           cast_ok = 0;
>>       }
>>         break;
>>
>> as this fixes the inconsistency I mentioned above i.e., sv's cast to
>> doubles which are not numbers return 0 or 1 (depending on STRICT)
>> instead of always returning 2 (cast ok). I worried a little about this
>> as you end up with 0 in the NV for a non-numeric and a return of 2 which
>> looked like the cast worked.
> 
> What does 
>     perl -V:.*|grep nv_preserves_uv
> say for you?

$ perl -V:.*|grep nv_preserves_uv
d_nv_preserves_uv='define';
nv_preserves_uv_bits='32';

> My reading of Perl_sv_2nv() in sv.c is that ifdef NV_PRESERVES_UV
> then SvNOK is not set (but SvNOKp is) if grok_number() returns 0
> into numtype.  The else NV_PRESERVES_UV branch ends with
>         if (!numtype)
>             SvFLAGS(sv) &= ~(SVf_IOK|SVf_NOK);
> So either way, if grok_number() returns 0 then SvNOK() should be false.
> 
> And since looks_like_number is just a wrapper around grok_number I'm not
> sure what's going on.
> 
> Perhaps check it in without the change above (so with a failing test)
> and I might get a change to dig into it.

ok, I'll check it in as you described later this afternoon and if you
get a chance to look at it that will be good but in the mean time I'll
let you know if I get any further with it.

>>> Also, there are a couple of bugs in sql_type_cast which I will fix when
>>> the dbi repository is available again - it is locked by some problem at
>>> svn.perl.org. I mailed [email protected] but no response or resolution as yet.
>> I fixed these.
> 
> Thanks.

checked in now.

>>> My test code so far which demonstrates the above attached in just in
>>> case (not tested on a system without JSON::XS or Data::Peek as yet and
>>> not using DBI::neat because it is too clever at recognising numbers -
>>> SvNIOK).
> 
> The "/* already has string version of the value, so use it */" block?

yes indeed.

>> I have now tested this on a system without JSON::XS and Data::Peek and
>> fixed the problems and added more tests.
>>
>> If you are ok with this I'll commit the change to DBI.xs, add the test
>> and document sql_type_cast_svpv in DBI::DBD then move on to making this
>> work in DBD::Oracle again.
> 
> That would be great. Many thanks Martin.

DBI::DBD pod changes checked in now although I may come back to it once
I've re-implemented this in DBD::Oracle.

>> BTW, not sure whether you prefer a mail to you or to continue this type
>> of discussion on the dbi-dev list - let me know and I'll do so in the
>> future.
> 
> dbi-dev is best - CC'd - as it keeps other driver developers in the loop
> and more eyeballs can help spot/fix problems more quickly.
> 
> Tim.
> 
> 

Martin
-- 
Martin J. Evans
Easysoft Limited
http://www.easysoft.com