Martin Evans wrote:
> Tim Bunce wrote:
>> On Tue, Oct 27, 2009 at 02:54:43PM +0000, Martin Evans wrote:
>>>> The next question is whether overflowing to an NV should be an error.
>>>> I'm thinking we could adopt these semantics for bind_col types:
>>>>
>>>> SQL_INTEGER IV or UV via sv_2iv(sv) with error on overflow
>>> this would be ideal.
>>>
>>>> SQL_DOUBLE NV via sv_2nv(sv)
>>>> SQL_NUMERIC IV else UV else NV via grok_number() with no error
>>>>
>>>> I could sketch out the logic for those cases if you'd be happy to polish
>>>> up and test.
>>> I would be happy to do that.
>> I finally got around to working on this. Here's a first rough draft
>> (which a bunch of issues) I thought I'd post here for discussion.
>>
>> I've implemented it as a hook in the DBIS structure so drivers can call
>> it directly.
>>
>> I've added the idea of optionally discarding the string buffer (which
>> would save space when storing many rows but waste time if just working
>> row-at-a-time). For now I've triggered that based on the sql_type but
>> that feels like a hack we'd regret later. A better approach might be an
>> attribute to bind_col:
>>
>> $sth->bind_col(..., { MinMemory => 1 });
>> $sth->fetchall_...();
>>
>> This code doesn't raise any errors or produce any warnings (directly),
>> it just returns a status that the driver should check if it wants to
>> implement the SQL_INTEGER == error on overflow semantics, which it
>> should if we agree that's what we're going to adopt.
>>
>> Tim.
>>
>>
>> Index: DBI.xs
>> ===================================================================
>> --- DBI.xs (revision 13466)
>> +++ DBI.xs (working copy)
>> @@ -78,6 +78,7 @@
>> static int set_err_char _((SV *h, imp_xxh_t *imp_xxh, const char
>> *err_c, IV err_i, const char *errstr, const char *state, const char
>> *method));
>> static int set_err_sv _((SV *h, imp_xxh_t *imp_xxh, SV *err, SV
>> *errstr, SV *state, SV *method));
>> static int quote_type _((int sql_type, int p, int s, int *base_type,
>> void *v));
>> +static int post_fetch_sv _((pTHX_ SV *h, imp_xxh_t *imp_xxh, SV *sv,
>> int sql_type, U32 flags, void *v));
>> static I32 dbi_hash _((const char *string, long i));
>> static void dbih_dumphandle _((pTHX_ SV *h, const char *msg, int
>> level));
>> static int dbih_dumpcom _((pTHX_ imp_xxh_t *imp_xxh, const char *msg,
>> int level));
>> @@ -439,6 +440,7 @@
>> DBIS->set_err_sv = set_err_sv;
>> DBIS->set_err_char= set_err_char;
>> DBIS->bind_col = dbih_sth_bind_col;
>> + DBIS->post_fetch_sv = post_fetch_sv;
>>
>>
>> /* Remember the last handle used. BEWARE! Sneaky stuff here! */
>> @@ -1714,6 +1718,94 @@
>> }
>>
>>
>> +/* Convert a simple string representation of a value into a more specific
>> + * perl type based on an sql_type value.
>> + * The semantics of SQL standard TYPE values are interpreted _very_ loosely
>> + * on the basis of "be liberal in what you accept and let's throw in some
>> + * extra semantics while we're here" :)
>> + * Returns:
>> + * -1: sv is undef or doesn't
>> + * 0: sv couldn't be converted to requested (strict) type
>> + * 1: sv was handled without a problem
>> + */
>> +int
>> +post_fetch_sv(pTHX_ SV *h, imp_xxh_t *imp_xxh, SV *sv, int sql_type, U32
>> flags, void *v)
>> +{
>> + int discard_pv = 0;
>> +
>> + /* do nothing for undef (NULL) or non-string values */
>> + if (!sv || !SvPOK(sv))
>> + return -1;
>> +
>> + switch(sql_type) {
>> +
>> + /* caller would like IV (but may get UV or NV) */
>> + /* will warn if not numeric. return 0 on overflow */
>> + case SQL_SMALLINT:
>> + discard_pv = 1;
>> + case SQL_INTEGER:
>> + sv_2iv(sv); /* is liberal, may return SvIV, SvUV, or SvNV */
>> + if (SvNOK(sv)) { /* suspicious */
>> + NV nv = SvNV(sv);
>> + /* ignore NV set just to preserve digits after the decimal
>> place */
>> + /* just complain if the value won't fit in an IV or NV */
>> + if (nv > UV_MAX || nv < IV_MIN)
>> + return 0;
>> + }
>> + break;
>> +
>> + /* caller would like SvNOK/SvIOK true if the value is a number */
>> + /* will warn if not numeric */
>> + case SQL_FLOAT:
>> + discard_pv = 1;
>> + case SQL_DOUBLE:
>> + sv_2nv(sv);
>> + break;
>> +
>> + /* caller would like IV else UV else NV */
>> + /* else no error and sv is untouched */
>> + case SQL_NUMERIC:
>> + discard_pv = 1;
>> + case SQL_DECIMAL: {
>> + UV uv;
>> + /* based on the code in perl's toke.c */
>> + int flags = grok_number(SvPVX(sv), SvCUR(sv), &uv);
>> +
>> + if (flags == IS_NUMBER_IN_UV) { /* +ve int */
>> + if (uv <= IV_MAX) /* prefer IV over UV */
>> + sv_2iv(sv);
>> + else sv_2uv(sv);
>> + }
>> + else if (flags == (IS_NUMBER_IN_UV | IS_NUMBER_NEG)
>> + && uv <= IV_MAX
>> + ) {
>> + sv_2iv(sv);
>> + }
>> + else if (flags) /* is numeric */
>> + sv_2nv(sv);
>> + }
>> + break;
>> +
>> +#if 0 /* XXX future possibilities */
>> + case SQL_BIGINT: /* use Math::BigInt if too large for IV/UV */
>> +#endif
>> + default:
>> + return 0; /* value unchanged */
>> + }
>> +
>> + if (discard_pv /* caller wants string buffer discarded */
>> + && SvNIOK(sv) /* we set a numeric value */
>> + && SvPVX(sv) && SvLEN(sv) /* we have a buffer to discard */
>> + ) {
>> + Safefree(SvPVX(sv));
>> + SvPVX(sv) = NULL;
>> + SvPOK_off(sv);
>> + }
>> + return 1;
>> +}
>> +
>> +
>> +
>>
>>
>
> I'm presuming you missed a bit off your patch:
>
> Index: DBIXS.h
> ===================================================================
> --- DBIXS.h (revision 13478)
> +++ DBIXS.h (working copy)
> @@ -431,10 +431,10 @@
> int (*bind_col) _((SV *sth, SV *col, SV *ref, SV
> *attribs));
>
> IO *logfp_ref; /* DAA keep ptr to filehandle for refcounting */
> -
> + int (*post_fetch_sv) _((pTHX_ SV *h, imp_xxh_t *imp_xxh, SV
> *sv, int sql_type, U32 flags, void *v));
> /* WARNING: Only add new structure members here, and reduce pad2 to
> keep */
> /* the memory footprint exactly the same */
> - void *pad2[4];
> + void *pad2[3];
> };
>
>
> Martin
There was an omission in my addition to Tim's example as I forgot to
change DBISTATE_VERSION.
I've implemented this as it stands in DBD::Oracle and it seems to work
out ok and certainly where I was wanting to go (and further).
My own feeling is that if someone asks for something to be bound as an
SQL_INTEGER and it cannot due to over/under flow this should be an error
and that is how I've implemented it. Perhaps it could have been one of
those informationals as the sv is unchanged and still usable but it is
not in the requested format so I'd class that an error. However, I have
a very small concern for people who might have been binding columns with
a type but no destination SV but their DBD did nothing about it (which I
believe is all DBDs up to now). For me, I didn't leave that code in and
just documented it as:
# I was hoping the following would work (according to DBI, it
# might) to ensure the a, b and c
# columns are returned as integers instead of strings saving
# us from having to add 0 to them below. It does not with
# DBD::Oracle.
# NOTE: you don't have to pass a var into bind_col to receive
# the column data as it works on the underlying column and not
# just a particular bound variable.
#$cursor->bind_col(4, undef, { TYPE => SQL_INTEGER });
#$cursor->bind_col(5, undef, { TYPE => SQL_INTEGER });
#$cursor->bind_col(10, undef, { TYPE => SQL_INTEGER });
but if those last 3 lines were left uncommented they would have ended up
a noop before but not now. However, I'd be surprised if anyone was
really doing that as it did nothing.
I think a MinMemory attribute would be ok but I'd use it as in most of
my cases I am retrieving the whole result-set in one go and it can be
very large. How would post_fetch_sv know this attribute?
What was the intention of "void *v" argument at the end of post_fetch_sv?
Martin
--
Martin J. Evans
Easysoft Limited
http://www.easysoft.com
