DBD::Oracle, Support binding of integers so they are returned as IVs

[Martin J. Evans]

Hi,

With reference to the rt I created "Support binding of integers so they
are returned as IVs" at http://rt.cpan.org/Public/Bug/Display.html?id=49818

I am now at the point where being unable to bind columns to results-sets
in DBD::Oracle with a bind type of SQL_INTEGER (or whatever) so they
look like integers in Perl is slowing some code of ours down
dramatically. We convert a fetchall_arrayref returned structure into
JSON with JSON::XS and JSON::XS converts strings to "string" and numbers
to a plain number. Our select returns a number of columns which are
really integer columns and as the result-set is very large the extra
space we use encoding integers as "number" is more than just annoying.
JSON::XS appears to know a perl scalar has been used in the context of a
number as if we add 0 to the integer columns returned in
fetchall_arrayref it encodes them as plain numbers instead of strings
like "number" (see the rt for the snippet from JSON::XS which does
this). As a result, a workaround we are using now is to loop through the
rows adding 0 to all integer columns. I believe DBI allows bind_col to
be called without a destination scalar so you can use it to specify the
type of the bind e.g., SQL_INTEGER but still call fetchall_arrayref.

Does anyone know if it is feasible to make this work with DBD::Oracle
and if so do you have some pointers as to how it may be achieved. I am
not looking for anyone else to do the work but would like to sound
people out about the possibility before I launch into it.

Thanks

Martin

[John Scoles]

Sounds like an easy patch to DBD::Oracle (off the top of my head) I am
not sure how it would fix into the DBI spec though.

If I am reading the question right you want to be able to tell
DBI/DBD::Oracle that col X of a return is an int?

something like

$SQL='select my_id,my_name from my_table'

my $C=$DBH->prepare($SQL,{row1=int})

for lack of a better example

cheers
John Scoles

[Martin Evans]

Thanks for the reply John.

John Scoles wrote:
> Sounds like an easy patch to DBD::Oracle (off the top of my head) I am
> not sure how it would fix into the DBI spec though.
>
> If I am reading the question right you want to be able to tell
> DBI/DBD::Oracle that col X of a return is an int?

Yes

> something like
>
> $SQL='select my_id,my_name from my_table'
>
> my $C=$DBH->prepare($SQL,{row1=int})
>
> for lack of a better example

example from DBI docs below.

> cheers
> John Scoles

DBI for bind_col says:

=====
The \%attr parameter can be used to hint at the data type formatting the
column should have. For example, you can use:

$sth->bind_col(1, undef, { TYPE => SQL_DATETIME });

to specify that you'd like the column (which presumably is some kind of
datetime type) to be returned in the standard format for SQL_DATETIME,
which is 'YYYY-MM-DD HH:MM:SS', rather than the native formatting the
database would normally use.

There's no $var_to_bind in that example to emphasize the point that
bind_col() works on the underlying column and not just a particular
bound variable.
=====

so I think what I want to do is already in the spec but not implemented
by DBD::Oracle. I want to call bind_col saying SQL_INTEGER then call
fetchall_arrayref and get IVs back for those columns instead of SVs.

So DBD::Oracle would need to know bind_col was called with a type and
save the type then at fetch time create an IV instead of an SV.

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

[Greg Sabino Mullane]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: RIPEMD160

> With reference to the rt I created "Support binding of integers so they
> are returned as IVs" at http://rt.cpan.org/Public/Bug/Display.html?id=49818

If I'm understanding you correctly, this was recently 'fixed' in DBD::Pg,
to accomodate JSON::XS as well. For the relevant code, see:

http://svn.perl.org/modules/DBD-Pg/trunk/dbdimp.c

and grep for "cast"

- --
Greg Sabino Mullane gr...@turnstep.com
End Point Corporation
PGP Key: 0x14964AC8 200910231020
http://biglumber.com/x/web?pk=2529DF6AB8F79407E94445B4BC9B906714964AC8
-----BEGIN PGP SIGNATURE-----

iEYEAREDAAYFAkrhu9QACgkQvJuQZxSWSsgQ1gCeNgcbOfG2JXgCGg7GEzER/SI2
+ysAnAstVap3LWFWOYMBmm0MoLAvjvOC
=GKyv
-----END PGP SIGNATURE-----

[Martin Evans]

Greg Sabino Mullane wrote:
>
>> With reference to the rt I created "Support binding of integers so they
>> are returned as IVs" at http://rt.cpan.org/Public/Bug/Display.html?id=49818
>
> If I'm understanding you correctly, this was recently 'fixed' in DBD::Pg,
> to accomodate JSON::XS as well. For the relevant code, see:
>
> http://svn.perl.org/modules/DBD-Pg/trunk/dbdimp.c
>
> and grep for "cast"
>

Thanks Greg, that is exactly what I am talking about.

I'll take a look at that.

[Martin Evans]

Greg Sabino Mullane wrote:
>
>> With reference to the rt I created "Support binding of integers so they
>> are returned as IVs" at http://rt.cpan.org/Public/Bug/Display.html?id=49818
>
> If I'm understanding you correctly, this was recently 'fixed' in DBD::Pg,
> to accomodate JSON::XS as well. For the relevant code, see:
>
> http://svn.perl.org/modules/DBD-Pg/trunk/dbdimp.c
>
> and grep for "cast"
>

I could do what DBD::Pg does here (and have to verify it works) but
Oracle integers can be very large - too big to fit in an IV in some
cases. I think the only person who knows if an integer is small enough
to fit in an IV is the person calling bind_col and in any case, my
situation is in fact that some of the database integers I need back as
strings and some I want as integers. As a result, I think it is
necessary to support the TYPE attribute to bind_col.

1. Does any DBD support TYPE in bind_col (I can't find any so far)?

2. Presumably if a DBD wanted to store the TYPE would it need to
implement the bind_col (dbd_st_bind_col) method itself?

[Greg Sabino Mullane]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: RIPEMD160

> I could do what DBD::Pg does here (and have to verify it works) but
> Oracle integers can be very large - too big to fit in an IV in some
> cases.

Ah yes, I forgot that Oracle doesn't really have an integer type.

> I think the only person who knows if an integer is small enough
> to fit in an IV is the person calling bind_col and in any case, my
> situation is in fact that some of the database integers I need back as
> strings and some I want as integers. As a result, I think it is
> necessary to support the TYPE attribute to bind_col.

Sounds like a foot gun. What happens when the type is declared as int, but
they send back 999999999999? Wouldn't the value get silently changed
to 2147483647? A way around that is to have the driver check the size
to see if it will fit in as IV, but at that point, you don't need the
user-specified casting anymore, perhaps just a separate flag, e.g.

$dbh->{ora_return_iv_when_possible} = 1;

Frankly, it sounds like we're doing a lot of smashing of square pegs into
round holes to make JSON::XS happy: maybe it's better to look for a solution
on that end at this point?

- --
Greg Sabino Mullane gr...@turnstep.com
End Point Corporation

PGP Key: 0x14964AC8 200910261039

http://biglumber.com/x/web?pk=2529DF6AB8F79407E94445B4BC9B906714964AC8
-----BEGIN PGP SIGNATURE-----

iEYEAREDAAYFAkrltQwACgkQvJuQZxSWSsgXkACglr89gAmjiRJEluR5846rATvb
luEAoKsaP7MdQEB3pjUCjh2xYvd2V8D2
=J3V0
-----END PGP SIGNATURE-----

[Martin Evans]

Greg Sabino Mullane wrote:
>
>> I could do what DBD::Pg does here (and have to verify it works) but
>> Oracle integers can be very large - too big to fit in an IV in some
>> cases.
>
> Ah yes, I forgot that Oracle doesn't really have an integer type.
>
>> I think the only person who knows if an integer is small enough
>> to fit in an IV is the person calling bind_col and in any case, my
>> situation is in fact that some of the database integers I need back as
>> strings and some I want as integers. As a result, I think it is
>> necessary to support the TYPE attribute to bind_col.
>
> Sounds like a foot gun. What happens when the type is declared as int, but
> they send back 999999999999? Wouldn't the value get silently changed
> to 2147483647? A way around that is to have the driver check the size
> to see if it will fit in as IV, but at that point, you don't need the
> user-specified casting anymore, perhaps just a separate flag, e.g.
>
> $dbh->{ora_return_iv_when_possible} = 1;

I was never suggesting any integer would be silently changed - why would
anyone do that.

Doing things automatically won't help me as I only want integers back
for some of the columns. Also, I would not want an IV back for a small
integer in row N and then a string back for a large integer in row N+1
(I'd rather have an error). DBI already defines a TYPE on bind_col and
if it was implemented it could be generally useful as well as useful to me.

> Frankly, it sounds like we're doing a lot of smashing of square pegs into
> round holes to make JSON::XS happy: maybe it's better to look for a solution
> on that end at this point?

I cannot say why you changed DBD::Pg for JSON::XS but I can say why I
want integers back from Oracle instead of strings when I ask for
integers and the column is an integer. I don't think it is square pegs
into round holes.

I don't maintain DBD::Oracle but would have been happy to submit any
patch back for consideration. I was only looking for a push in the right
direction before implementing and even if the consensus is that it is a
waste of time it won't stop me implementing it myself as I need it.
However, I'd rather not do that as I am already maintaining 4 other
changes to DBD::Oracle until at least the next DBD::Oracle is released.

[Tim Bunce]

On Mon, Oct 26, 2009 at 03:03:43PM +0000, Martin Evans wrote:
>
> I cannot say why you changed DBD::Pg for JSON::XS but I can say why I
> want integers back from Oracle instead of strings when I ask for
> integers and the column is an integer. I don't think it is square pegs
> into round holes.
>
> I don't maintain DBD::Oracle but would have been happy to submit any
> patch back for consideration. I was only looking for a push in the right
> direction before implementing and even if the consensus is that it is a
> waste of time it won't stop me implementing it myself as I need it.

I'd be happy to see bind_col used to express the desire to have integers
returned as integers (IV normally, UV if that'll fit, else an error).

The grok_number() perl api call will do much of the work for you:
http://search.cpan.org/~nwclark/perl/pod/perlapi.pod#Numeric_functions

Tim.

[Martin Evans]

What follows is a very rough patch (definitely not finished) which
proves you can do what I wanted to do. However, there on no checks on
the column being bound existing and I'm not sure how to save the TYPE
attribute when bind_col is called before execute (that is when the
result-set is not described yet). Basically, I think more is required in
dbd_st_bind_col but I've not sure as yet what that is and it is possible
returning 1 is a total hack. I'd appreciate any advice to complete this.

Index: oci8.c
===================================================================
--- oci8.c (revision 13427)
+++ oci8.c (working copy)
@@ -3279,10 +3279,31 @@
while(datalen && p[datalen - 1]==' ')
--datalen;
}
- sv_setpvn(sv, p, (STRLEN)datalen);
- if (CSFORM_IMPLIES_UTF8(fbh->csform) ){
- SvUTF8_on(sv);
- }
+
+ if ((fbh->req_type == 3) &&
+ ((fbh->dbtype == 2) || (fbh->dbtype == 3))){
+ char *e;
+ char zval[32];
+ long val;
+
+ memcpy(zval, p, datalen);
+ zval[datalen] = '\0';
+ val = strtol(zval, &e, 10);
+
+ if ((val == LONG_MAX) || (val == LONG_MIN) ||
+ (e && (*e != '\0'))) {
+ oci_error(sth, imp_sth->errhp, OCI_ERROR,
+ "invalid number or over/under flow");
+ return Nullav;
+ }
+
+ sv_setiv(sv, val);
+ } else {
+ sv_setpvn(sv, p, (STRLEN)datalen);
+ if (CSFORM_IMPLIES_UTF8(fbh->csform) ){
+ SvUTF8_on(sv);
+ }
+ }
}
}

Index: dbdimp.c
===================================================================
--- dbdimp.c (revision 13427)
+++ dbdimp.c (working copy)
@@ -869,7 +869,16 @@
return 1;
}

+int dbd_st_bind_col(SV *sth, imp_sth_t *imp_sth, SV *col, SV *ref, IV
type, SV *attribs) {
+ dTHX;

+ int field = SvIV(col);
+
+ imp_sth->fbh[field-1].req_type = type;
+
+ return 1;
+}
+
int
dbd_db_disconnect(SV *dbh, imp_dbh_t *imp_dbh)
{
Index: dbdimp.h
===================================================================
--- dbdimp.h (revision 13427)
+++ dbdimp.h (working copy)
@@ -191,6 +191,8 @@
int piece_lob; /*use piecewise fetch for lobs*/
/* Our storage space for the field data as it's fetched */
sword ftype; /* external datatype we wish to get */
+ IV req_type; /* type passed to
bind_col */
+
fb_ary_t *fb_ary ; /* field buffer array */
/* if this is an embedded object we use this */
fbh_obj_t *obj;
@@ -371,6 +373,7 @@
#define dbd_st_FETCH_attrib ora_st_FETCH_attrib
#define dbd_describe ora_describe
#define dbd_bind_ph ora_bind_ph
+#define dbd_st_bind_col ora_st_bind_col
#include "ocitrace.h"

/* end */
Index: Oracle.h
===================================================================
--- Oracle.h (revision 13427)
+++ Oracle.h (working copy)
@@ -67,6 +67,7 @@
int dbd_db_do _((SV *sv, char *statement));
int dbd_db_commit _((SV *dbh, imp_dbh_t *imp_dbh));
int dbd_db_rollback _((SV *dbh, imp_dbh_t *imp_dbh));
+int dbd_st_bind_col(SV *sth, imp_sth_t *imp_sth, SV *col, SV *ref, IV
type, SV *attribs);
int dbd_db_disconnect _((SV *dbh, imp_dbh_t *imp_dbh));
void dbd_db_destroy _((SV *dbh, imp_dbh_t *imp_dbh));
int dbd_db_STORE_attrib _((SV *dbh, imp_dbh_t *imp_dbh, SV *keysv, SV
*valuesv));

[Tim Bunce]

On Mon, Oct 26, 2009 at 05:29:21PM +0000, Martin Evans wrote:
>
> What follows is a very rough patch (definitely not finished) which
> proves you can do what I wanted to do. However, there on no checks on
> the column being bound existing and I'm not sure how to save the TYPE
> attribute when bind_col is called before execute (that is when the
> result-set is not described yet). Basically, I think more is required in
> dbd_st_bind_col but I've not sure as yet what that is and it is possible
> returning 1 is a total hack. I'd appreciate any advice to complete this.
>
> Index: oci8.c
> ===================================================================
> --- oci8.c (revision 13427)
> +++ oci8.c (working copy)
> @@ -3279,10 +3279,31 @@

> +
> + if ((fbh->req_type == 3) &&
> + ((fbh->dbtype == 2) || (fbh->dbtype == 3))){

Best to avoid 'magic numbers'.

> + char *e;
> + char zval[32];
> + long val;
> +
> + memcpy(zval, p, datalen);
> + zval[datalen] = '\0';
> + val = strtol(zval, &e, 10);
> +
> + if ((val == LONG_MAX) || (val == LONG_MIN) ||
> + (e && (*e != '\0'))) {
> + oci_error(sth, imp_sth->errhp, OCI_ERROR,
> + "invalid number or over/under flow");
> + return Nullav;
> + }
> + sv_setiv(sv, val);
> + } else {
> + sv_setpvn(sv, p, (STRLEN)datalen);
> + if (CSFORM_IMPLIES_UTF8(fbh->csform) ){
> + SvUTF8_on(sv);
> + }
> + }

A simpler safer and more portable approach may be to just let the
existing code store the value in an sv and then add these lines:

if (fbh->req_type == 3)
sv_2iv(sv);

If the number is too large for an IV (or UV) you'll get an NV (float).
The original string of digits is preserved in all cases. That's all very
natural and predictable perlish behaviour.

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

Tim.

[Martin Evans]

Thanks Tim for the help on this.

Tim Bunce wrote:
> On Mon, Oct 26, 2009 at 05:29:21PM +0000, Martin Evans wrote:
>> What follows is a very rough patch (definitely not finished) which
>> proves you can do what I wanted to do. However, there on no checks on
>> the column being bound existing and I'm not sure how to save the TYPE
>> attribute when bind_col is called before execute (that is when the
>> result-set is not described yet). Basically, I think more is required in
>> dbd_st_bind_col but I've not sure as yet what that is and it is possible
>> returning 1 is a total hack. I'd appreciate any advice to complete this.
>>
>> Index: oci8.c
>> ===================================================================
>> --- oci8.c (revision 13427)
>> +++ oci8.c (working copy)
>> @@ -3279,10 +3279,31 @@
>> +
>> + if ((fbh->req_type == 3) &&
>> + ((fbh->dbtype == 2) || (fbh->dbtype == 3))){
>
> Best to avoid 'magic numbers'.

As I said = very rough. I'd already changed those to SQLT_NUM and
SQLT_INT as ORA types but I guessed they would need to be SQL_INTEGER,
SQL_NUMERIC, SQL_DOUBLE when finished i.e. you use the DBI types not the
oracle types here since the data is coming back into perl.

>> + char *e;
>> + char zval[32];
>> + long val;
>> +
>> + memcpy(zval, p, datalen);
>> + zval[datalen] = '\0';
>> + val = strtol(zval, &e, 10);
>> +
>> + if ((val == LONG_MAX) || (val == LONG_MIN) ||
>> + (e && (*e != '\0'))) {
>> + oci_error(sth, imp_sth->errhp, OCI_ERROR,
>> + "invalid number or over/under flow");
>> + return Nullav;
>> + }
>> + sv_setiv(sv, val);
>> + } else {
>> + sv_setpvn(sv, p, (STRLEN)datalen);
>> + if (CSFORM_IMPLIES_UTF8(fbh->csform) ){
>> + SvUTF8_on(sv);
>> + }
>> + }

Tried your suggestion of the grok_number but it does not work well for
negative numbers since it returns the abs then and puts the result in a
UV which may not fit signed into an IV. Anyway, you seem to have had
other ideas so I'll not worry about that too much.

> A simpler safer and more portable approach may be to just let the
> existing code store the value in an sv and then add these lines:
>
> if (fbh->req_type == 3)
> sv_2iv(sv);
>
> If the number is too large for an IV (or UV) you'll get an NV (float).
> The original string of digits is preserved in all cases. That's all very
> natural and predictable perlish behaviour.

Ok, I get that except you keep saying "(or UV)". Are you suggesting
there is some other logic to decide whether you create an IV or a UV?

I tried out various values and sv_2iv(sv) and what was returned looked
ok - I get a string when the number has decimal places or is too big and
an IV when it is an integer and fits.

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

BTW, did you look over the possible hackery I did in dbd_st_bind_col - I
wasn't sure if simply storing the requested type and returning 1 was
acceptable. My current dbd_st_bind_col is:

int dbd_st_bind_col(SV *sth, imp_sth_t *imp_sth, SV *col, SV *ref, IV
type, SV *attribs) {

dTHX;

int field = SvIV(col);

if (field <= DBIc_NUM_FIELDS(imp_sth)) {

imp_sth->fbh[field-1].req_type = type;
}

return 1;
}

This means if someone attempts to bind a non-existent column it falls
back into DBI's bind_col and signals the error but it also means
dbd_st_bind_col in DBD::Oracle is only there to capture the requested
bind type.

Thanks

[John Scoles]

Just to let you guys know I am following this very closely

Right now I as still pinned down with regular work but hopefully this
week I will get some DBD::Oracle time.

So far I have to agree with Tim about the 'magic numbers'

I think as well as we do a describe before the execute we can get the
size automatically and sizer them up there a little better than I do now.

we do get the

OCI_ATTR_DATA_TYPE and OCI_ATTR_DATA_SIZE

and I handle the number stuff like this

case ORA_NUMBER: /* NUMBER */
case 21: /* BINARY FLOAT os-endian */
case 22: /* BINARY DOUBLE os-endian */
case 100: /* BINARY FLOAT oracle-endian */
case 101: /* BINARY DOUBLE oracle-endian */
fbh->disize = 130+38+3; /* worst case */
avg_width = 4; /* NUMBER approx +/- 1_000_000 */
break;

maybe we can just diddle with this code and fix it for you long ints

Like you I am just taking a quick look at it

chees
John Scoles

[Tim Bunce]

On Tue, Oct 27, 2009 at 02:54:43PM +0000, Martin Evans wrote:
> >> +
> >> + memcpy(zval, p, datalen);
> >> + zval[datalen] = '\0';
> >> + val = strtol(zval, &e, 10);
> >> +
> >> + if ((val == LONG_MAX) || (val == LONG_MIN) ||
> >> + (e && (*e != '\0'))) {
> >> + oci_error(sth, imp_sth->errhp, OCI_ERROR,
> >> + "invalid number or over/under flow");
> >> + return Nullav;
> >> + }
> >> + sv_setiv(sv, val);
> >> + } else {
> >> + sv_setpvn(sv, p, (STRLEN)datalen);
> >> + if (CSFORM_IMPLIES_UTF8(fbh->csform) ){
> >> + SvUTF8_on(sv);
> >> + }
> >> + }
>
> Tried your suggestion of the grok_number but it does not work well for
> negative numbers since it returns the abs then and puts the result in a
> UV which may not fit signed into an IV. Anyway, you seem to have had
> other ideas so I'll not worry about that too much.

(See how grok_number is used in perl's toke.c for an example.)

> > A simpler safer and more portable approach may be to just let the
> > existing code store the value in an sv and then add these lines:
> >
> > if (fbh->req_type == 3)
> > sv_2iv(sv);
> >
> > If the number is too large for an IV (or UV) you'll get an NV (float).
> > The original string of digits is preserved in all cases. That's all very
> > natural and predictable perlish behaviour.
>
> Ok, I get that except you keep saying "(or UV)". Are you suggesting
> there is some other logic to decide whether you create an IV or a UV?

sv_2iv does that for you if the number is too big for an IN but will
fit in a UV.

> I tried out various values and sv_2iv(sv) and what was returned looked
> ok - I get a string when the number has decimal places or is too big and
> an IV when it is an integer and fits.

Take a look at the source code for sv_2iv (actually Perl_sv_2iv in sv.c)
It's scary!

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

Ok.

> BTW, did you look over the possible hackery I did in dbd_st_bind_col - I
> wasn't sure if simply storing the requested type and returning 1 was
> acceptable. My current dbd_st_bind_col is:
>
> int dbd_st_bind_col(SV *sth, imp_sth_t *imp_sth, SV *col, SV *ref, IV
> type, SV *attribs) {
> dTHX;
>
> int field = SvIV(col);
>
> if (field <= DBIc_NUM_FIELDS(imp_sth)) {
> imp_sth->fbh[field-1].req_type = type;
> }

Don't forget to catch field <= 0 :)

> return 1;
> }
>
> This means if someone attempts to bind a non-existent column it falls
> back into DBI's bind_col and signals the error but it also means
> dbd_st_bind_col in DBD::Oracle is only there to capture the requested
> bind type.

Yeap, that's as intended.

Tim.

[Martin J. Evans]

will do.

>>> A simpler safer and more portable approach may be to just let the
>>> existing code store the value in an sv and then add these lines:
>>>
>>> if (fbh->req_type == 3)
>>> sv_2iv(sv);
>>>
>>> If the number is too large for an IV (or UV) you'll get an NV (float).
>>> The original string of digits is preserved in all cases. That's all very
>>> natural and predictable perlish behaviour.
>> Ok, I get that except you keep saying "(or UV)". Are you suggesting
>> there is some other logic to decide whether you create an IV or a UV?
>
> sv_2iv does that for you if the number is too big for an IN but will
> fit in a UV.

argh - didn't see that - will try again with my test code and verify that.

>> I tried out various values and sv_2iv(sv) and what was returned looked
>> ok - I get a string when the number has decimal places or is too big and
>> an IV when it is an integer and fits.
>
> Take a look at the source code for sv_2iv (actually Perl_sv_2iv in sv.c)
> It's scary!

then perhaps I won't bother looking at it ;-)

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

>> BTW, did you look over the possible hackery I did in dbd_st_bind_col - I
>> wasn't sure if simply storing the requested type and returning 1 was
>> acceptable. My current dbd_st_bind_col is:
>>
>> int dbd_st_bind_col(SV *sth, imp_sth_t *imp_sth, SV *col, SV *ref, IV
>> type, SV *attribs) {
>> dTHX;
>>
>> int field = SvIV(col);
>>
>> if (field <= DBIc_NUM_FIELDS(imp_sth)) {
>> imp_sth->fbh[field-1].req_type = type;
>> }
>
> Don't forget to catch field <= 0 :)

yes, fair enough - changed.

>> return 1;
>> }
>>
>> This means if someone attempts to bind a non-existent column it falls
>> back into DBI's bind_col and signals the error but it also means
>> dbd_st_bind_col in DBD::Oracle is only there to capture the requested
>> bind type.
>
> Yeap, that's as intended.
>
> Tim.
>
>

ok, sounds like we have some progress and at least I'm nearly on the
right wavelength.

Thanks again. This really is useful to the project I am working on and
given the feedback I've had it sounds like a) it would be useful for a
DBD to implement it so other DBDs could have a reference for its use b)
useful for anyone wanting to override the returned type c) provides
further checks on the returned column which could fall out of range of
the data the perl app expects.

Martin

[Greg Sabino Mullane]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: RIPEMD160

> I cannot say why you changed DBD::Pg for JSON::XS but I can say why I
> want integers back from Oracle instead of strings when I ask for
> integers and the column is an integer. I don't think it is square pegs
> into round holes.

Well you'll never get a Perl integer (IV) to match Oracle's integer:
NUMBER(38). But sounds like y'all have found a good enough solution.

- --
Greg Sabino Mullane gr...@turnstep.com

PGP Key: 0x14964AC8 200910281253

http://biglumber.com/x/web?pk=2529DF6AB8F79407E94445B4BC9B906714964AC8
-----BEGIN PGP SIGNATURE-----

iEYEAREDAAYFAkrodzUACgkQvJuQZxSWSsjR7QCeKbkwnKKZK7zgC0laJ3sozeoh
JccAnA8puFN+uX7ieOSzf924LJuj72qw
=yYAY
-----END PGP SIGNATURE-----

[Tim Bunce]

On Wed, Oct 28, 2009 at 04:54:06PM -0000, Greg Sabino Mullane wrote:
>
> > I cannot say why you changed DBD::Pg for JSON::XS but I can say why I
> > want integers back from Oracle instead of strings when I ask for
> > integers and the column is an integer. I don't think it is square pegs
> > into round holes.
>
> Well you'll never get a Perl integer (IV) to match Oracle's integer:
> NUMBER(38). But sounds like y'all have found a good enough solution.

We could always make bind_col(..., SQL_BIGINT) enable creation of
Math::BigInt objects for values that are too large for IV.

Tim.

[Tim Bunce]

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;
+}
+

+
+

[Martin Evans]

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];
};

[Tim Bunce]

On Mon, Nov 09, 2009 at 02:43:28PM +0000, Martin Evans wrote:
> I'm presuming you missed a bit off your patch:

Yes, sorry, I was just "throwing it out there" for discussion of the
general approach/principles. None so far.

Tim.

[Martin Evans]

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?

[Tim Bunce]

> There was an omission in my addition to Tim's example as I forgot to
> change DBISTATE_VERSION.

Thanks. Though that's less important than it was now there's also
DBIXS_REVISION (in dbixs_rev.h) that automatically tracks the svn
revsion number.

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

Ok.

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

The return value of post_fetch_sv() is meant to allow drivers to
report an error.

I thought about making post_fetch_sv() itself call DBIh_SET_ERR_* to
report an error but opted to avoid that because, to generate a good
error more info would need to be passed, like the column number.

On the other hand, if post_fetch_sv() doesn't do it then there's a
greater risk of inconsistency between the drivers.

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

Perhaps we should have $sth->bind_col(..., { LooselyTyped => 1 });
to allow for those who don't want an error if the type doesn't fit.

That certainly feels better than overloading SQL_INTEGER vs SQL_NUMERIC
to achieve the same effect!

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

Does anyone know of any drivers that pay any attention to the type param
of bind_column?

We could make it default to issuing a warning on overflow, and have
attributes to specify either an error or ignore.

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

Via the flags argument.

> What was the intention of "void *v" argument at the end of post_fetch_sv?

Planning for an uncertain future.

After mulling it over some more, and looking at ODBC's SQLBindCol (which
takes a C type, not an SQL type) I've decided to err on the simple side.
I've appended a patch for review.

Tim.

Index: DBI.xs
===================================================================
--- DBI.xs (revision 13478)

+++ 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 sql_type_cast_svpv _((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));

@@ -434,11 +435,12 @@
DBIS->get_fbav = dbih_get_fbav;
DBIS->make_fdsv = dbih_make_fdsv;
DBIS->neat_svpv = neatsvpv;
- DBIS->bind_as_num = quote_type;
+ DBIS->bind_as_num = quote_type; /* XXX deprecated */
DBIS->hash = dbi_hash;

DBIS->set_err_sv = set_err_sv;
DBIS->set_err_char= set_err_char;
DBIS->bind_col = dbih_sth_bind_col;

+ DBIS->sql_type_cast_svpv = sql_type_cast_svpv;

/* Remember the last handle used. BEWARE! Sneaky stuff here! */

@@ -1696,6 +1698,8 @@
(void)s;
(void)t;
(void)v;
+ /* looks like it's never been used, and doesn't make much sense anyway */
+ warn("Use of DBI internal bind_as_num/quote_type function is deprecated");
switch(sql_type) {
case SQL_INTEGER:
case SQL_SMALLINT:
@@ -1714,6 +1718,95 @@

}


+/* 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, unchanged
+ * -2: sql_type isn't handled, value unchanged

+ * 0: sv couldn't be converted to requested (strict) type
+ * 1: sv was handled without a problem
+ */

+#define DBIstcf_DISCARD_PV 0x0001
+#define DBIstcf_STRICT 0x0002
+
+int
+sql_type_cast_svpv(pTHX_ SV *h, imp_xxh_t *imp_xxh, SV *sv, int sql_type, U32 flags, void *v)
+{
+

+ /* do nothing for undef (NULL) or non-string values */

+ if (!sv || !SvOK(sv))

+ return -1;
+
+ switch(sql_type) {
+

+ case SQL_INTEGER:
+ /* sv_2iv is liberal, may return SvIV, SvUV, or SvNV */
+ sv_2iv(sv);
+ /* if strict, complain if SvNOK set because value is out of range
+ * for IV/UV, or if SvIOK is not set because it's not numeric (in which
+ * case perl would have warn'd already if -w or warnings are in effect)
+ */
+ if (flags & DBIstcf_STRICT && (SvNOK(sv) || !SvIOK(sv))) {

+ return 0;
+ }
+ break;
+

+ case SQL_DOUBLE:
+ sv_2nv(sv);

+ /* if strict, complain if !SvNOK because value is not numeric
+ * (perl would have warn'd already if -w or warnings are in effect)
+ */
+ if (flags & DBIstcf_STRICT && !SvNOK(sv)) {

+ return 0;
+ }
+ break;
+

+ /* caller would like IV else UV else NV */
+ /* else no error and sv is untouched */
+ case SQL_NUMERIC: {

+ 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);
+ }

+ else if (flags & DBIstcf_STRICT)
+ return 0; /* not numeric */

+ break;
+
+#if 0 /* XXX future possibilities */
+ case SQL_BIGINT: /* use Math::BigInt if too large for IV/UV */
+#endif
+ default:

+ return -2; /* not a recognised SQL TYPE, value unchanged */
+ }
+
+ if (flags & DBIstcf_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;
+}
+
+
+

/* --- Generic Handle Attributes (for all handle types) --- */

static int

Index: DBIXS.h
===================================================================
--- DBIXS.h (revision 13478)
+++ DBIXS.h (working copy)

@@ -392,7 +392,7 @@

struct dbistate_st {

-#define DBISTATE_VERSION 94 /* Must change whenever dbistate_t does */
+#define DBISTATE_VERSION 95 /* Must change whenever dbistate_t does */

/* this must be the first member in structure */
void (*check_version) _((const char *name,
@@ -417,7 +417,7 @@
SV * (*get_attr_k) _((SV *h, SV *keysv, int dbikey));
AV * (*get_fbav) _((imp_sth_t *imp_sth));
SV * (*make_fdsv) _((SV *sth, const char *imp_class, STRLEN imp_size, const char *col_name));
- int (*bind_as_num) _((int sql_type, int p, int s, int *t, void *v));
+ int (*bind_as_num) _((int sql_type, int p, int s, int *t, void *v)); /* XXX deprecated */
I32 (*hash) _((const char *string, long i));
SV * (*preparse) _((SV *sth, char *statement, IV ps_return, IV ps_accept, void *foo));

@@ -432,9 +432,10 @@

IO *logfp_ref; /* DAA keep ptr to filehandle for refcounting */

+ int (*cast_svpv) _((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];
};

/* macros for backwards compatibility */
Index: dbixs_rev.h
===================================================================
--- dbixs_rev.h (revision 13478)
+++ dbixs_rev.h (working copy)
@@ -1,4 +1,4 @@
-/* Mon Nov 2 22:44:58 2009 */
-/* Mixed revision working copy (13455M:13465) */
+/* Fri Nov 6 23:01:13 2009 */
+/* Mixed revision working copy (13455M:13466) */
/* Code modified since last checkin */
#define DBIXS_REVISION 13455

[Martin Evans]

Thread is getting a bit long so I've snipped a lot of previous code.

Tim Bunce wrote:
> On Mon, Nov 09, 2009 at 05:05:11PM +0000, Martin Evans wrote:
>> Martin Evans wrote:
>>>>

<first patch snipped>

>
>> There was an omission in my addition to Tim's example as I forgot to
>> change DBISTATE_VERSION.
>
> Thanks. Though that's less important than it was now there's also
> DBIXS_REVISION (in dbixs_rev.h) that automatically tracks the svn
> revsion number.
>
>> 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).
>
> Ok.
>
>> 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.
>
> The return value of post_fetch_sv() is meant to allow drivers to
> report an error.
>
> I thought about making post_fetch_sv() itself call DBIh_SET_ERR_* to
> report an error but opted to avoid that because, to generate a good
> error more info would need to be passed, like the column number.

I agree and had already output an error containing the column number.

> On the other hand, if post_fetch_sv() doesn't do it then there's a
> greater risk of inconsistency between the drivers.

I think we already have a level of inconsistency as some drivers already
return IVs without being asked for them. Also, number handling in each
database tends to differ quite a bit so I suspect the default may want
to differ per DBD.

>> 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.
>
> Perhaps we should have $sth->bind_col(..., { LooselyTyped => 1 });
> to allow for those who don't want an error if the type doesn't fit.

I'm happy with that.

> That certainly feels better than overloading SQL_INTEGER vs SQL_NUMERIC
> to achieve the same effect!

agreed.

>> 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.
>
> Does anyone know of any drivers that pay any attention to the type param
> of bind_column?

I did not find one when I was looking a few months ago.

> We could make it default to issuing a warning on overflow, and have
> attributes to specify either an error or ignore.

We could but I think most people would be happy with error or specifying
LooselyTyped. You either know you need LooselyTyped or not and if not
you can leave it off and if it errors then your data was not as you
thought and have to decide if your data is wrong or you need
LooselyTyped. I'd be concerned a warning might just get in the way.

>> 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?
>
> Via the flags argument.

As it turns out I /need/ MinMemory or SvPOKp(sv) returns true and that
ends up being a string again in JSON::XS. i.e., I needed the equivalent
of adding 0 to the sv which does this:

perl -le 'use Devel::Peek;my $a = "5"; Dump($a); $a = $a + 0; Dump($a);'
SV = PV(0x8154b00) at 0x815469c
REFCNT = 1
FLAGS = (PADBUSY,PADMY,POK,pPOK)
PV = 0x816fb48 "5"\0
CUR = 1
LEN = 4
SV = PVIV(0x8155b10) at 0x815469c
REFCNT = 1
FLAGS = (PADBUSY,PADMY,IOK,pIOK)
IV = 5
PV = 0x816fb48 "5"\0
CUR = 1
LEN = 4

as JSON::XS does:

if (SvPOKp (sv))
{
.
}
else if (SvNOKp (sv))
{
.
}
else if (SvIOKp (sv))
{
I want this case.

Of course, I could argue with the JSON::XS maintainer for changing the
order but I don't hold out much hope of that changing.

>> What was the intention of "void *v" argument at the end of post_fetch_sv?
>
> Planning for an uncertain future.

ok.

> After mulling it over some more, and looking at ODBC's SQLBindCol (which
> takes a C type, not an SQL type) I've decided to err on the simple side.
> I've appended a patch for review.
>
> Tim.
>

<new patch snipped>

There were a few small issues with the latest patch but it was obvious
what you meant so I have rectified them and made changes to my test
DBD::Oracle. This works rather nicely now.

At this stage I have the following changes and comments:

o patch as you supplied with minor corrections in DBI

The only one I'm not 100% about is that:

if (flags & DBIstcf_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) = Nullch; /* changed from NULL to Nullch */
SvLEN(sv) = 0; /* <--- added this as it aborts without it */
SvPOK_off(sv);
}

Without that line adding it always aborts if MinMemory=1 and there is
more than one row.

o added dbd_st_bind_col to DDB::Oracle
stores the requested type in fbh
handles LooselyTyped and MinMemory and saves in fbh
defaults to DBIstcf_STRICT if LooselyTyped not specified

Comment: I thought DBIstcf_STRICT was a good default but others may
not.
Comment: DBIstcf_STRICT and DBIstcf_DISCARD_PV macros are not
available as symbols to a DBD at present so I'm using 1 and 2 in
DBD::Oracle. I wasn't sure where they'd go.

Comment: I was not so keen on the attribute being LooselyTyped and the
macro being DBIstcf_STRICT as they have opposite meanings but I'm not
really bothered as LooselyTyped is all that is really going to be visible.

o oci8.c and dbd_st_fetch modified to call DBI's sql_type_cast_svpv and
errors when 0 (no strict conversion, with "over/under flow converting
column N to type N") and -2 (sql type unhandled, with "unsupported bind
type N for column N"). I don't need to think about undef in
DBD::Oracle's case as it is handled separately.

o test code using all this which demonstrates binding with a type works
and LoosleyTyped and MinMemory do as they should. Some of this will be a
little awkward to turn into a test that works for everyone as it needs
Devel::Peek.

What I haven't done or would appreciate feedback on:

o My test DBD::Oracle currently assumes DBI has sql_type_cast_svpv. What
is the usual way of deciding this - checking DBI version?

o macros for DBIstcf_STRICT and DBIstcf_DISCARD_PV - see above

o any documentation for DBI::DBD although I am happy to do so when you
are happy we have reached a consensus.

o any documentation for DBI - as above.

o I wouldn't mind someone else looking over the changes as although I
currently maintain DBD::ODBC I do not consider myself a DBI internals of
XS expert.

o Although my primary impetus for doing this is I need it in DBD::Oracle
I would plan to do something similar in DBD::ODBC once this stuff is
committed.

o I do not have a commit bit on DBD::Oracle so will pass the DBD::Oracle
changes to John when ready.

Thanks.

Here are the changes so far:

Index: DBI.xs
===================================================================
--- DBI.xs (revision 13479)

+++ 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 sql_type_cast_svpv _((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));

@@ -434,13 +435,13 @@

DBIS->get_fbav = dbih_get_fbav;
DBIS->make_fdsv = dbih_make_fdsv;
DBIS->neat_svpv = neatsvpv;
- DBIS->bind_as_num = quote_type;
+ DBIS->bind_as_num = quote_type; /* XXX deprecated */
DBIS->hash = dbi_hash;
DBIS->set_err_sv = set_err_sv;
DBIS->set_err_char= set_err_char;
DBIS->bind_col = dbih_sth_bind_col;
+ DBIS->sql_type_cast_svpv = sql_type_cast_svpv;

-

/* Remember the last handle used. BEWARE! Sneaky stuff here! */

/* We want a handle reference but we don't want to increment */
/* the handle's reference count and we don't want perl to try */
@@ -1696,6 +1697,8 @@

(void)s;
(void)t;
(void)v;
+ /* looks like it's never been used, and doesn't make much sense
anyway */
+ warn("Use of DBI internal bind_as_num/quote_type function is
deprecated");
switch(sql_type) {
case SQL_INTEGER:
case SQL_SMALLINT:

@@ -1713,7 +1716,100 @@
return 1;
}

+/* 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, unchanged
+ * -2: sql_type isn't handled, value unchanged
+ * 0: sv couldn't be converted to requested (strict) type
+ * 1: sv was handled without a problem
+ */
+#define DBIstcf_DISCARD_PV 0x0001
+#define DBIstcf_STRICT 0x0002

+int

+ 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);
+ }
+ else if (flags & DBIstcf_STRICT)
+ return 0; /* not numeric */
+ break;
+ }
+
+#if 0 /* XXX future possibilities */
+ case SQL_BIGINT: /* use Math::BigInt if too large for IV/UV */
+#endif
+ default:
+ return -2; /* not a recognised SQL TYPE, value unchanged */
+ }
+
+ if (flags & DBIstcf_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) = Nullch;
+ SvLEN(sv) = 0;

+ SvPOK_off(sv);
+ }
+ return 1;
+}
+
+
+
+
/* --- Generic Handle Attributes (for all handle types) --- */

static int
Index: DBIXS.h
===================================================================

--- DBIXS.h (revision 13479)

+++ DBIXS.h (working copy)
@@ -392,7 +392,7 @@

struct dbistate_st {

-#define DBISTATE_VERSION 94 /* Must change whenever dbistate_t does */
+#define DBISTATE_VERSION 95 /* Must change whenever dbistate_t does */

/* this must be the first member in structure */
void (*check_version) _((const char *name,
@@ -417,7 +417,7 @@
SV * (*get_attr_k) _((SV *h, SV *keysv, int dbikey));
AV * (*get_fbav) _((imp_sth_t *imp_sth));
SV * (*make_fdsv) _((SV *sth, const char *imp_class,
STRLEN imp_size, const char *col_name));
- int (*bind_as_num) _((int sql_type, int p, int s, int *t,
void *v));
+ int (*bind_as_num) _((int sql_type, int p, int s, int *t,
void *v)); /* XXX deprecated */
I32 (*hash) _((const char *string, long i));
SV * (*preparse) _((SV *sth, char *statement, IV
ps_return, IV ps_accept, void *foo));

@@ -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 (*sql_type_cast_svpv) _((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];
};

/* macros for backwards compatibility */
Index: dbixs_rev.h
===================================================================

--- dbixs_rev.h (revision 13479)
+++ dbixs_rev.h (working copy)
@@ -1,4 +1,3 @@

-/* Mon Nov 2 22:44:58 2009 */
-/* Mixed revision working copy (13455M:13465) */

+/* Tue Nov 10 08:49:53 2009 */

/* Code modified since last checkin */

-#define DBIXS_REVISION 13455
+#define DBIXS_REVISION 13479

and DBD::Oracle (sorry about the tabbling, it seems oci.8.c has some
very perculiar tabbling):

Index: oci8.c
===================================================================
--- oci8.c (revision 13427)
+++ oci8.c (working copy)

@@ -3279,10 +3279,33 @@

while(datalen && p[datalen - 1]==' ')
--datalen;
}
- sv_setpvn(sv, p, (STRLEN)datalen);
- if (CSFORM_IMPLIES_UTF8(fbh->csform) ){
- SvUTF8_on(sv);
- }

+ sv_setpvn(sv, p, (STRLEN)datalen);
+ if (fbh->req_type != 0) {
+ int sts;
+ D_imp_xxh(sth);
+ char errstr[256];
+
+
+ sts = DBIc_DBISTATE(imp_sth)->sql_type_cast_svpv(
+ aTHX_ sth, imp_xxh, sv, fbh->req_type,
fbh->bind_flags, NULL);
+ if (sts == 0) {
+ sprintf(errstr,
+ "over/under flow converting column
%d to type %ld",
+ i+1, fbh->req_type);

+ oci_error(sth, imp_sth->errhp, OCI_ERROR,

errstr);
+ return Nullav;
+
+ } else if (sts == -2) {
+ sprintf(errstr,
+ "unsupported bind type %ld for
column %d",
+ fbh->req_type, i+1);
+ return Nullav;
+ }
+ } else {

+ if (CSFORM_IMPLIES_UTF8(fbh->csform) ){
+ SvUTF8_on(sv);
+ }
+ }
}
}

Index: dbdimp.c

===================================================================
--- dbdimp.c (revision 13427)
+++ dbdimp.c (working copy)

@@ -869,7 +869,50 @@
return 1;
}

+int dbd_st_bind_col(SV *sth, imp_sth_t *imp_sth, SV *col, SV *ref, IV
type, SV *attribs) {
+ dTHX;
+ int field;

+ if (!SvIOK(col)) {
+ croak ("Invalid column number") ;
+ }
+
+ field = SvIV(col);
+
+ if ((field < 1) || (field > DBIc_NUM_FIELDS(imp_sth))) {
+ croak("cannot bind to non-existent field %d", field);
+ }
+
+ imp_sth->fbh[field-1].req_type = type;
+ imp_sth->fbh[field-1].bind_flags = 2; /*DBIstcf_STRICT*/
+
+ if (attribs) {
+ HV *attr_hash;
+ SV **attr;
+
+ if (!SvROK(attribs)) {
+ croak ("attributes is not a reference");
+ } else if (SvTYPE(SvRV(attribs)) != SVt_PVHV) {
+ croak ("attributes not a hash reference");
+ }
+ attr_hash = (HV *)SvRV(attribs);
+
+ attr = hv_fetch(attr_hash, "LooselyTyped", (U32)12, 0);
+ if (attr && SvTRUE(*attr)) {
+ imp_sth->fbh[field-1].bind_flags &= ~(U32)2;
+ } else {
+ imp_sth->fbh[field-1].bind_flags |= 2;
+ }
+
+ attr = hv_fetch(attr_hash, "MinMemory", (U32)9, 0);
+ if (attr && SvTRUE(*attr)) {
+ imp_sth->fbh[field-1].bind_flags |= 1;
+ }
+ }
+

+ return 1;
+}
+
int
dbd_db_disconnect(SV *dbh, imp_dbh_t *imp_dbh)
{
Index: dbdimp.h
===================================================================
--- dbdimp.h (revision 13427)
+++ dbdimp.h (working copy)

@@ -191,6 +191,9 @@

int piece_lob; /*use piecewise fetch for lobs*/
/* Our storage space for the field data as it's fetched */
sword ftype; /* external datatype we wish to get */
+ IV req_type; /* type passed to
bind_col */

+ UV bind_flags; /* flags passed to

bind_col */
+
fb_ary_t *fb_ary ; /* field buffer array */
/* if this is an embedded object we use this */
fbh_obj_t *obj;

@@ -371,6 +374,7 @@

#define dbd_st_FETCH_attrib ora_st_FETCH_attrib
#define dbd_describe ora_describe
#define dbd_bind_ph ora_bind_ph
+#define dbd_st_bind_col ora_st_bind_col
#include "ocitrace.h"

/* end */
Index: Oracle.h
===================================================================
--- Oracle.h (revision 13427)
+++ Oracle.h (working copy)
@@ -67,6 +67,7 @@
int dbd_db_do _((SV *sv, char *statement));
int dbd_db_commit _((SV *dbh, imp_dbh_t *imp_dbh));
int dbd_db_rollback _((SV *dbh, imp_dbh_t *imp_dbh));
+int dbd_st_bind_col(SV *sth, imp_sth_t *imp_sth, SV *col, SV *ref, IV
type, SV *attribs);
int dbd_db_disconnect _((SV *dbh, imp_dbh_t *imp_dbh));
void dbd_db_destroy _((SV *dbh, imp_dbh_t *imp_dbh));
int dbd_db_STORE_attrib _((SV *dbh, imp_dbh_t *imp_dbh, SV *keysv, SV
*valuesv));

Martin

[Tim Bunce]

I'm sorry for the delay in replying Martin.

On Tue, Nov 10, 2009 at 03:24:33PM +0000, Martin Evans wrote:
>
> >> 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.
> >
> > Perhaps we should have $sth->bind_col(..., { LooselyTyped => 1 });
> > to allow for those who don't want an error if the type doesn't fit.
>
> I'm happy with that.
>
> > That certainly feels better than overloading SQL_INTEGER vs SQL_NUMERIC
> > to achieve the same effect!
>
> agreed.

> > We could make it default to issuing a warning on overflow, and have

> > attributes to specify either an error or ignore.
>
> We could but I think most people would be happy with error or specifying
> LooselyTyped. You either know you need LooselyTyped or not and if not
> you can leave it off and if it errors then your data was not as you
> thought and have to decide if your data is wrong or you need
> LooselyTyped. I'd be concerned a warning might just get in the way.
>
> >> 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?
> >
> > Via the flags argument.
>
> As it turns out I /need/ MinMemory or SvPOKp(sv) returns true and that
> ends up being a string again in JSON::XS.

> > After mulling it over some more, and looking at ODBC's SQLBindCol (which

> > takes a C type, not an SQL type) I've decided to err on the simple side.
> > I've appended a patch for review.
>

> <new patch snipped>
>
> There were a few small issues with the latest patch but it was obvious
> what you meant so I have rectified them and made changes to my test
> DBD::Oracle. This works rather nicely now.
>
> At this stage I have the following changes and comments:
>
> o patch as you supplied with minor corrections in DBI
>

> ) {
> Safefree(SvPVX(sv));
> SvPVX(sv) = Nullch; /* changed from NULL to Nullch */
> SvLEN(sv) = 0; /* <--- added this as it aborts without it */
> SvPOK_off(sv);
> }

Fixed. Thanks.

> o added dbd_st_bind_col to DDB::Oracle
> stores the requested type in fbh
> handles LooselyTyped and MinMemory and saves in fbh
> defaults to DBIstcf_STRICT if LooselyTyped not specified
>
> Comment: I thought DBIstcf_STRICT was a good default but others may not.

> Comment: I was not so keen on the attribute being LooselyTyped and the

> macro being DBIstcf_STRICT as they have opposite meanings but I'm not
> really bothered as LooselyTyped is all that is really going to be visible.

I'm mostly settled on DBIstcf_STRICT _not_ being the default.
So 'loosely typed' will be the default and people who want an error
if the type can't be cast safely will need to use StrictlyTyped => 1.

I'm (slightly) open to persuasion, but that seems to fit the general
philosophy of perl and the DBI better. It also makes transition easier
for anyone who happens to have code that calls bind_column with an SQL type.

> Comment: DBIstcf_STRICT and DBIstcf_DISCARD_PV macros are not
> available as symbols to a DBD at present so I'm using 1 and 2 in
> DBD::Oracle. I wasn't sure where they'd go.

I've added them to DBIXS.h now.

I'm undecided about the name of the attribute to indicate use of
DBIstcf_DISCARD_PV. MinMemory or DiscardString or ...?
I'm leaning more towards DiscardString => 1 as it directly describes
what it does rather than a side-effect.

> o oci8.c and dbd_st_fetch modified to call DBI's sql_type_cast_svpv and
> errors when 0 (no strict conversion, with "over/under flow converting
> column N to type N") and -2 (sql type unhandled, with "unsupported bind
> type N for column N"). I don't need to think about undef in
> DBD::Oracle's case as it is handled separately.

Ok.

> o test code using all this which demonstrates binding with a type works
> and LoosleyTyped and MinMemory do as they should. Some of this will be a
> little awkward to turn into a test that works for everyone as it needs
> Devel::Peek.

Or JSON::XS :) Umm, or perhaps DBI::neat?

> What I haven't done or would appreciate feedback on:
>
> o My test DBD::Oracle currently assumes DBI has sql_type_cast_svpv. What
> is the usual way of deciding this - checking DBI version?

I've bumped DBISTATE_VERSION from 94 to 95.
(Also DBIXS_REVISION > 13590 would work.)

> o any documentation for DBI::DBD although I am happy to do so when you
> are happy we have reached a consensus.

Thank you!

> o any documentation for DBI - as above.

Thank you!

> o I wouldn't mind someone else looking over the changes as although I
> currently maintain DBD::ODBC I do not consider myself a DBI internals of
> XS expert.

Post a diff and I'll review it for you. The code you appended previously
looks ok.

> o Although my primary impetus for doing this is I need it in DBD::Oracle
> I would plan to do something similar in DBD::ODBC once this stuff is
> committed.

Great.

> o I do not have a commit bit on DBD::Oracle so will pass the DBD::Oracle
> changes to John when ready.

I'd strongly recommend John to give you a commit bit! :)

> Here are the changes so far:

You've resent a diff against the original rather than a diff of your
extra changes. I can't do much with that easily. I assume the only
significant change was adding SvLEN(sv) = 0; If there's more then let me
know (as a diff of what's changed after applying my earlier diff :)

> and DBD::Oracle (sorry about the tabbling, it seems oci.8.c has some
> very perculiar tabbling):

[Probably tabstop=4. I used to use that before I finally realized that
tab _characters_ are at 8 column intervals and trying to pretend otherwise
caused minor but widely dispursed annoyance for myself and others.
Now my tab _key_ still indents by 4 but it uses spaces to do so.
META: This is just background info, not an invitation to anyone to open
a discussion on this topic :-]

I've made some further changes:

- Renamed DBIstcf_DISCARD_PV to DBIstcf_DISCARD_STRING
- DBIstcf_DISCARD_STRING only works if the cast worked ok
- No longer pass h and imp_xxh to sql_type_cast_svpv
- Added a perl API via DBI::sql_type_cast($foo, SQL_INTEGER, $flags)
- Minor return status change to indicate status of cast even if
DBIstcf_STRICT was not used.

and committed it:
http://svn.perl.org/viewvc/modules?view=revision&revision=13591

Tim.

[H.Merijn Brand]

On Tue, 24 Nov 2009 13:12:24 +0000, Tim Bunce <Tim....@pobox.com>
wrote:

> > o test code using all this which demonstrates binding with a type works
> > and LoosleyTyped and MinMemory do as they should. Some of this will be a
> > little awkward to turn into a test that works for everyone as it needs
> > Devel::Peek.

Shameless plug: Data::Peek

> Or JSON::XS :) Umm, or perhaps DBI::neat?

Note that in our company JSON::XS is forbidden due to the appalling
(read insulting) level of support (or better the lack of it) from the
author. No software from Marc can be installed. So for JSON, we use
JSON instead of JSON::XS.

:)

DBI-svn > git dpull
M lib/DBD/File.pm
M lib/DBI/Profile.pm
M Changes
r13581 = 025205cb02de9e16dea21c39598704d0feec68ab (refs/remotes/git-svn)
M DBI.xs
M DBIXS.h
M Changes
M dbixs_rev.h
M DBI.pm
r13591 = 60aac6bc5ebf48244d7eaef61d198e62d1ed59b3 (refs/remotes/git-svn)
M t/35thrclone.t
r13592 = a312cfa11d5816756150f7ce04b5d1b07d709b2e (refs/remotes/git-svn)
First, rewinding head to replay your work on top of it...
Fast-forwarded master to refs/remotes/git-svn.
DBI-svn > make test
:
All tests successful.
Files=130, Tests=5926, 59 wallclock secs ( 1.53 usr 0.29 sys + 47.18 cusr 5.15 csys = 54.15 CPU)
Result: PASS

--
H.Merijn Brand http://tux.nl Perl Monger http://amsterdam.pm.org/
using & porting perl 5.6.2, 5.8.x, 5.10.x, 5.11.x on HP-UX 10.20, 11.00,
11.11, 11.23, and 11.31, OpenSuSE 10.3, 11.0, and 11.1, AIX 5.2 and 5.3.
http://mirrors.develooper.com/hpux/ http://www.test-smoke.org/
http://qa.perl.org http://www.goldmark.org/jeff/stupid-disclaimers/

[Tim Bunce]

On Tue, Nov 24, 2009 at 02:32:51PM +0100, H.Merijn Brand wrote:
> On Tue, 24 Nov 2009 13:12:24 +0000, Tim Bunce <Tim....@pobox.com>
> wrote:
>
> > > o test code using all this which demonstrates binding with a type works
> > > and LoosleyTyped and MinMemory do as they should. Some of this will be a
> > > little awkward to turn into a test that works for everyone as it needs
> > > Devel::Peek.
>
> Shameless plug: Data::Peek
>
> > Or JSON::XS :) Umm, or perhaps DBI::neat?
>
> Note that in our company JSON::XS is forbidden due to the appalling
> (read insulting) level of support (or better the lack of it) from the
> author. No software from Marc can be installed. So for JSON, we use
> JSON instead of JSON::XS.

Umm. Ok.

> DBI-svn > git dpull

> All tests successful.

Probably because there aren't any new tests yet :)

One reason for adding DBI::sql_type_cast() is to make it easier to write
tests for the functionality.

Volunteers?

Tim.

[Greg Sabino Mullane]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: RIPEMD160

> I'm mostly settled on DBIstcf_STRICT _not_ being the default.
> So 'loosely typed' will be the default and people who want an error
> if the type can't be cast safely will need to use StrictlyTyped => 1.
>
> I'm (slightly) open to persuasion, but that seems to fit the general
> philosophy of perl and the DBI better. It also makes transition easier
> for anyone who happens to have code that calls bind_column with an SQL type.

I'd opine that since we're dealing with databases holding Important Things,
the default should be to throw an error and make people flip a switch
to get lax mode. A large warning in the upgreade notes should suffice for
anyone transitioning.

> I'm undecided about the name of the attribute to indicate use of
> DBIstcf_DISCARD_PV. MinMemory or DiscardString or ...?
> I'm leaning more towards DiscardString => 1 as it directly describes
> what it does rather than a side-effect.

+1 to DiscardString

> o I wouldn't mind someone else looking over the changes as although I
> currently maintain DBD::ODBC I do not consider myself a DBI internals of
> XS expert.

>> Post a diff and I'll review it for you. The code you appended previously
>> looks ok.

Ditto.

> o Although my primary impetus for doing this is I need it in DBD::Oracle
> I would plan to do something similar in DBD::ODBC once this stuff is
> committed.

Does anyone know if there are other databases that have mapping issues with
integers and Perlish integers other than Oracle? Just curious, as I only
personally know how a handful deal with ints internally.

- --
Greg Sabino Mullane gr...@turnstep.com

End Point Corporation
PGP Key: 0x14964AC8 200911271301

http://biglumber.com/x/web?pk=2529DF6AB8F79407E94445B4BC9B906714964AC8
-----BEGIN PGP SIGNATURE-----

iEYEAREDAAYFAksQFFkACgkQvJuQZxSWSshf5wCgn8mSxnHM4YaUgn9P2wXaxqSe
pvgAoIKvRQTZtxGKeJgs4dVzZAvCp1jg
=nQJI
-----END PGP SIGNATURE-----

[Martin J. Evans]

Greg, the issue I started by trying to address is rather specific to our
use of JSON::XS and roughly described at
http://rt.cpan.org/Public/Bug/Display.html?id=49818. The problem we
faced is not specific to DBD::Oracle and applies to many DBDs (and
certainly DBD::ODBC) although I believe it was addressed in Postgres a
while ago. I don't think there is anything fundamentally wrong with what
most DBDs do right now - Perl has always worked on the basis that you
shouldn't need to know the way the underlaying data is stored, Perl will
just do the right thing for the context you use the data. On the other
hand we have the TYPE attribute to bind_col and no DBDs seems to do
anything with it. While some may (and do) argue JSON::XS shouldn't do
what it does I think the DBI side of this has moved on a bit now with
these changes. Few, if any, DBDs pay any attention to the TYPE attribute
on the bind_col method but /could/ benefit from knowing the type before
the data is retrieved. For instance, DBD::ODBC (like may other DBDs)
binds all the data from the database as strings but in many cases if you
know the data is really an integer (or will be used as an integer) it is
more efficient to retrieve it as such from the database. The TYPE
argument to bind_col provides that information and these changes (if
implemented in a DBD) simply arrange for the retrieved data to be "cast"
to a Perl scalar with only the IV etc set and not the pv (assuming
DiscardString is used).

A big (pardon the pun) gain (other than the above) would be to support
SQL_BIGINT. The database we are using is Oracle and it supports very
large integers and as we know these will get (for primary keys etc) too
big to fit in an integer or long integer we have to treat them as
strings but they are not strings. Also, Oracle (and perhaps other
databases) don't always like accepting strings for columns which are
integers (they have to convert the strings and then sometimes decide
that because of that they won't use an index on that column). I'd like
to follow these changes up with some investigation for big integers and
I'll hopefully find some time to do just that but one thing at a time.

Martin

[Tim Bunce]

On Fri, Nov 27, 2009 at 06:03:29PM -0000, Greg Sabino Mullane wrote:
>
> > I'm mostly settled on DBIstcf_STRICT _not_ being the default.
> > So 'loosely typed' will be the default and people who want an error
> > if the type can't be cast safely will need to use StrictlyTyped => 1.
> >
> > I'm (slightly) open to persuasion, but that seems to fit the general
> > philosophy of perl and the DBI better. It also makes transition easier
> > for anyone who happens to have code that calls bind_column with an SQL type.
>
> I'd opine that since we're dealing with databases holding Important Things,
> the default should be to throw an error and make people flip a switch
> to get lax mode.

Umm, if the database holds Important Things one could opine that perl
should trust the Important Things that are already in the database.
We can argue it either way.

A compromise might be to default to strict (for SQL_INTEGER and
SQL_DOUBLE) but make SQL_NUMERIC immune from strict.

> > o Although my primary impetus for doing this is I need it in DBD::Oracle
> > I would plan to do something similar in DBD::ODBC once this stuff is
> > committed.
>
> Does anyone know if there are other databases that have mapping issues with
> integers and Perlish integers other than Oracle? Just curious, as I only
> personally know how a handful deal with ints internally.

DBD::SQLite returns only strings, along with most of the pure-perl
drivers, like DBD::CSV.

Which makes me wonder if $sth->_set_fbav(\@row), which is used by most
pure-perl drivers to store each row, should call sql_type_cast()
automatically.

Tim.

[H.Merijn Brand]

On Sat, 28 Nov 2009 00:43:48 +0000, Tim Bunce <Tim....@pobox.com>
wrote:

> On Fri, Nov 27, 2009 at 06:03:29PM -0000, Greg Sabino Mullane wrote:

The plan is still in my head to convert PV("1"\0) to IV(1) if the field
type is numeric on fetch. Low priority though. Probably needs some work
in DBD::File

> Which makes me wonder if $sth->_set_fbav(\@row), which is used by most
> pure-perl drivers to store each row, should call sql_type_cast()
> automatically.
>
> Tim.

[Tim Bunce]

On Sat, Nov 28, 2009 at 11:05:31AM +0100, H.Merijn Brand wrote:
> > >
> > > Does anyone know if there are other databases that have mapping issues with
> > > integers and Perlish integers other than Oracle? Just curious, as I only
> > > personally know how a handful deal with ints internally.
> >
> > DBD::SQLite returns only strings, along with most of the pure-perl
> > drivers, like DBD::CSV.
>
> The plan is still in my head to convert PV("1"\0) to IV(1) if the field
> type is numeric on fetch. Low priority though. Probably needs some work
> in DBD::File

Well, if this was done:

> > Which makes me wonder if $sth->_set_fbav(\@row), which is used by most
> > pure-perl drivers to store each row, should call sql_type_cast()
> > automatically.

then it would take no work in DBD::File at all. Applications that
actually want IV/UV/NV's for some columns could simply use bind_col() to
ask for it. Also, being done in C, it would also be much faster than a
pure-perl addition to DBD::File.

Tim.

p.s. Seems like there's a reasonable case for restoring the old initial
\%attr of bind_columns() method: $sth->bind_columns({ TYPE => SQL_NUMERIC });
The 'harmless' nature of the SQL_NUMERIC semantics we've adopted make
that quite useful.

[Martin J. Evans]

Apologies if it seems this change is stalled, I've been extremely busy
attempting to keep the wolf from the door. I will be back on it to
finish it in the next few days. I've so far:

o documented change in trunk for bind_col (edited by Tim)
o documented change in trunk for DBI::DBD
o fixed a problem in sql_type_cast
o written test cases for sql_type_cast in trunk

As far as I am aware there are four outstanding issues:

o 2 of the tests I submitted fail - sql_type_cast for double - I got
some input from perl monks and some guidance from Tim but I've not
properly looked at any of it yet.
o I've not posted a revised patch for DBD::Oracle using this stuff.
o I've not changed DBD::ODBC (which will wait until DBD::Oracle is
finalised)
o I've not got a commit bit on DBD::Oracle - not the end of the world as
I'll post a patch but it would be nice.

Sorry about lack of progress but I've been /really/ busy with paid work
and with bugs in other Perl modules that were more important to our
project right now.

Martin

[Rudy Lippan]

On 11/27/2009 01:03 PM, Greg Sabino Mullane wrote:
> I'd opine that since we're dealing with databases holding Important Things,
> the default should be to throw an error and make people flip a switch
> to get lax mode. A large warning in the upgreade notes should suffice for
> anyone transitioning.
>

FWIW, Many hosting companies do not send out upgrade notes to their
customers when upgrading some random Perl module. Many people do not
read the entire release notes when upgrading their OS. Many people do
not go looking for release notes before doing a 'sudo cpan install
Module' :) So I do not know if just an upgrade warning would suffice
for the majority of people.

I was about to say that I agree with the stricter being the default, and
that there should at least be a transition period. But then I got to
thinking.... What percentage of the databases out there are actually
holding "Important Things"? I suspect that the number of databases with
important data are far outweighed by those that contain nothing but
trivial data, alas.

-r

[Martin Evans]

Tim Bunce wrote:

>
> Post a diff and I'll review it for you. The code you appended previously
> looks ok.

Attached is a diff for DBD::Oracle based on subversion this morning (the
diffs for oci8.c may be a little difficult to read due to the large
indentation of the surrounding code and there are some additional
changes to fix comments). It adds support for DiscardString and
StrictlyTyped bind_col attributes and casting via sql_type_cast. Most
(all relevant) changes are conditional on a DBI with these features.

I have verified these changes work with Oracle 11 and DBD::Oracle from
subversion however, I note some lob tests were failing in DBD::Oracle
before my changes were applied.

There seem to be a large number of warnings compiling DBD::Oracle from
subversion but I've not touched any of them preferring to keep the patch
restricted to its purpose.

The issue with the SQL_DECIMAL return in sql_type_castsvpv still exists
(the NV_PRESERVES_UV issue) - I have not had time to look into this
properly as yet.

All comments appreciated.