Re: sql_type_cast small inconsistency and bugs

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?

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.

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

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

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

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