Mailing List Archive

On Fri, 13 Mar 2015, Peter wrote:

> IRT this block of code in the create_sql function in DBI.pm:
>
> for (my $i = 0; $i < @$columns; $i++) {
> $cols[$i] = $$columns[$i];
> #::logDebug("checking column '$cols[$i]'");
> if(defined $key) {
> $keycol = $i if $cols[$i] eq $key;
> }
> if(defined $config->{COLUMN_DEF}->{$cols[$i]}) {
> $cols[$i] .= " " .
> $config->{COLUMN_DEF}->{$cols[$i]};
> }
> else {
> $cols[$i] .= " $def_type";
> }
> $$columns[$i] = $cols[$i];
> $$columns[$i] =~ s/\s+.*//;
> }
>
> The last two lines of the loop look to be redundant to me.
> $$columns[$i] already contains the column name, and $cols[$i] contain
> the column definition which is the column name, followed by a space and
> other stuff, so the last two names write $cols[$i] back to $$columns[$i]
> then strip the space + other stuff off the end leaving just the column
> name ... which is what was in $$columns[$i] to begin with?
>
> Can we scrap those two lines or is there something I'm missing?

I read it that way too.

However, to avoid getting caught by anything "tricky, tricky" I think you
would want to put some heavy debugging in there before & after and test
pretty extensively.

It would also be good to hear from Mike whether he remembers any obscure
reason for this.

Jon

--
Jon Jensen
End Point Corporation
https://www.endpoint.com/

_______________________________________________
interchange-users mailing list
interchange-users@icdevgroup.org
http://www.icdevgroup.org/mailman/listinfo/interchange-users

Re: Redundant code in DBI.pm? [ In reply to ]

Mar 12, 2015, 8:40 PM

Post #3 of 16 (3125 views)

On 03/13/2015 04:27 PM, Jon Jensen wrote:
> I read it that way too.
>
> However, to avoid getting caught by anything "tricky, tricky" I think
> you would want to put some heavy debugging in there before & after and
> test pretty extensively.

Well, I can only see it being of any use in some edge case that I can't
fathom, so I'm not sure I can test for an edge case when I don't even
know what that edge case is.

> It would also be good to hear from Mike whether he remembers any obscure
> reason for this.

Yeah, I am hoping to hear from Mike on this. That code goes back to the
initial CVS import of that file so there is a good chance even Mike
can't remember why it's there.

Peter

_______________________________________________
interchange-users mailing list
interchange-users@icdevgroup.org
http://www.icdevgroup.org/mailman/listinfo/interchange-users

Re: Redundant code in DBI.pm? [ In reply to ]

jon at endpoint

Mar 12, 2015, 8:46 PM

Post #4 of 16 (3119 views)

On Fri, 13 Mar 2015, Peter wrote:

>> However, to avoid getting caught by anything "tricky, tricky" I think
>> you would want to put some heavy debugging in there before & after and
>> test pretty extensively.
>
> Well, I can only see it being of any use in some edge case that I can't
> fathom, so I'm not sure I can test for an edge case when I don't even
> know what that edge case is.

I'm just saying that our code reading is nice, but you would gain valuable
real experience if you have evidence logged that the exact same values
come out of there with and without those 2 lines you delete, for all the
different database connections you can test.

Jon

--
Jon Jensen
End Point Corporation
https://www.endpoint.com/

_______________________________________________
interchange-users mailing list
interchange-users@icdevgroup.org
http://www.icdevgroup.org/mailman/listinfo/interchange-users

Re: Redundant code in DBI.pm? [ In reply to ]

Mar 12, 2015, 9:53 PM

Post #5 of 16 (3127 views)

On 03/13/2015 04:46 PM, Jon Jensen wrote:
> I'm just saying that our code reading is nice, but you would gain
> valuable real experience if you have evidence logged that the exact same
> values come out of there with and without those 2 lines you delete, for
> all the different database connections you can test.

Fair enough, I'll do some sort of test on that before I push out any new
code for this, then.

I still want to hear from Mike, though.

Peter

_______________________________________________
interchange-users mailing list
interchange-users@icdevgroup.org
http://www.icdevgroup.org/mailman/listinfo/interchange-users

Re: Redundant code in DBI.pm? [ In reply to ]

Mar 13, 2015, 2:27 AM

Post #6 of 16 (3126 views)

On 03/13/2015 05:53 PM, Peter wrote:
> On 03/13/2015 04:46 PM, Jon Jensen wrote:
>> I'm just saying that our code reading is nice, but you would gain
>> valuable real experience if you have evidence logged that the exact same
>> values come out of there with and without those 2 lines you delete, for
>> all the different database connections you can test.
>
> Fair enough, I'll do some sort of test on that before I push out any new
> code for this, then.

I did some debugging and the output looks exactly how we thought it
would when reading the code.

Peter

_______________________________________________
interchange-users mailing list
interchange-users@icdevgroup.org
http://www.icdevgroup.org/mailman/listinfo/interchange-users

Re: Redundant code in DBI.pm? [ In reply to ]

Mar 13, 2015, 5:07 AM

Post #7 of 16 (3120 views)

Quoting Jon Jensen (jon@endpoint.com):
> On Fri, 13 Mar 2015, Peter wrote:
>
> >IRT this block of code in the create_sql function in DBI.pm:
> >
> > for (my $i = 0; $i < @$columns; $i++) {
> > $cols[$i] = $$columns[$i];
> >#::logDebug("checking column '$cols[$i]'");
> > if(defined $key) {
> > $keycol = $i if $cols[$i] eq $key;
> > }
> > if(defined $config->{COLUMN_DEF}->{$cols[$i]}) {
> > $cols[$i] .= " " .
> >$config->{COLUMN_DEF}->{$cols[$i]};
> > }
> > else {
> > $cols[$i] .= " $def_type";
> > }
> > $$columns[$i] = $cols[$i];
> > $$columns[$i] =~ s/\s+.*//;
> > }
> >
> >The last two lines of the loop look to be redundant to me.
> >$$columns[$i] already contains the column name, and $cols[$i] contain
> >the column definition which is the column name, followed by a space and
> >other stuff, so the last two names write $cols[$i] back to $$columns[$i]
> >then strip the space + other stuff off the end leaving just the column
> >name ... which is what was in $$columns[$i] to begin with?
> >
> >Can we scrap those two lines or is there something I'm missing?
>
> I read it that way too.
>
> However, to avoid getting caught by anything "tricky, tricky" I
> think you would want to put some heavy debugging in there before &
> after and test pretty extensively.
>
> It would also be good to hear from Mike whether he remembers any
> obscure reason for this.

The reason is that at you can define the type in the column
label of the table, i.e.

code price decimal(12) description varchar(200) image

I don't think anyone has really ever used that, so it could
go away if we want it to.

--
Mike Heins
Perusion -- Expert Interchange Consulting http://www.perusion.com/
phone +1.765.253.4194 <mike@perusion.com>

Socialism -- ideas so good they have to be enforced at gunpoint.
-- unknown

_______________________________________________
interchange-users mailing list
interchange-users@icdevgroup.org
http://www.icdevgroup.org/mailman/listinfo/interchange-users

Re: Redundant code in DBI.pm? [ In reply to ]

Mar 13, 2015, 5:58 AM

Post #8 of 16 (3119 views)

Quoting Mike Heins (mikeh@perusion.com):
> Quoting Jon Jensen (jon@endpoint.com):
> > On Fri, 13 Mar 2015, Peter wrote:
> >
> > >IRT this block of code in the create_sql function in DBI.pm:
> > >
> > > for (my $i = 0; $i < @$columns; $i++) {
> > > $cols[$i] = $$columns[$i];
> > >#::logDebug("checking column '$cols[$i]'");
> > > if(defined $key) {
> > > $keycol = $i if $cols[$i] eq $key;
> > > }
> > > if(defined $config->{COLUMN_DEF}->{$cols[$i]}) {
> > > $cols[$i] .= " " .
> > >$config->{COLUMN_DEF}->{$cols[$i]};
> > > }
> > > else {
> > > $cols[$i] .= " $def_type";
> > > }
> > > $$columns[$i] = $cols[$i];
> > > $$columns[$i] =~ s/\s+.*//;
> > > }
> > >
> > >The last two lines of the loop look to be redundant to me.
> > >$$columns[$i] already contains the column name, and $cols[$i] contain
> > >the column definition which is the column name, followed by a space and
> > >other stuff, so the last two names write $cols[$i] back to $$columns[$i]
> > >then strip the space + other stuff off the end leaving just the column
> > >name ... which is what was in $$columns[$i] to begin with?
> > >
> > >Can we scrap those two lines or is there something I'm missing?
> >
> > I read it that way too.
> >
> > However, to avoid getting caught by anything "tricky, tricky" I
> > think you would want to put some heavy debugging in there before &
> > after and test pretty extensively.
> >
> > It would also be good to hear from Mike whether he remembers any
> > obscure reason for this.
>
> The reason is that at you can define the type in the column
> label of the table, i.e.
>
> code price decimal(12) description varchar(200) image
>
> I don't think anyone has really ever used that, so it could
> go away if we want it to.

Just tested that out of curiosity:

catalog.cfg:

Database foo foo.txt __SQLDSN__

In foo.txt:

code varchar(33) description varchar(233) price decimal(12)
1111 The inimitable product. 11.11

Yields:

mysql> desc foo;
+-------------+---------------+------+-----+---------+-------+
| Field | Type | Null | Key | Default | Extra |
+-------------+---------------+------+-----+---------+-------+
| code | varchar(33) | YES | MUL | NULL | |
| description | varchar(233) | YES | | NULL | |
| price | decimal(12,0) | YES | | NULL | |
+-------------+---------------+------+-----+---------+-------+
3 rows in set (0.00 sec)

I see no reason to disturb it at this point. While I don't expect
anyone uses it, I did for some old things, and I am constantly amazed
at the old catalogs that are still out there.

--
Mike Heins
Perusion -- Expert Interchange Consulting http://www.perusion.com/
phone +1.765.253.4194 <mike@perusion.com>

The sun, with all those planets revolving around it and
dependent on it, can still ripen a bunch of grapes as if
it had nothing else in the universe to do. -- Galileo

_______________________________________________
interchange-users mailing list
interchange-users@icdevgroup.org
http://www.icdevgroup.org/mailman/listinfo/interchange-users

Re: Redundant code in DBI.pm? [ In reply to ]

Mar 13, 2015, 6:33 AM

Post #9 of 16 (3119 views)

On 03/14/2015 01:58 AM, Mike Heins wrote:
>>>> for (my $i = 0; $i < @$columns; $i++) {
>>>> $cols[$i] = $$columns[$i];
>>>> #::logDebug("checking column '$cols[$i]'");
>>>> if(defined $key) {
>>>> $keycol = $i if $cols[$i] eq $key;
>>>> }
>>>> if(defined $config->{COLUMN_DEF}->{$cols[$i]}) {
>>>> $cols[$i] .= " " .
>>>> $config->{COLUMN_DEF}->{$cols[$i]};
>>>> }
>>>> else {
>>>> $cols[$i] .= " $def_type";
>>>> }
>>>> $$columns[$i] = $cols[$i];
>>>> $$columns[$i] =~ s/\s+.*//;
>>>> }
>>
>> The reason is that at you can define the type in the column
>> label of the table, i.e.
>>
>> code price decimal(12) description varchar(200) image

Oh, I see, it looks like the first of those two lines is still
redundant, though. I think if we were to remove the $$columns[$i] =
$cols[$i] line and leave the next one in then the code would still work,
the column type could be defined in the column and just the name would
be left in @$columns.

> Just tested that out of curiosity:

Just to satisfy my own curiosity, can you comment out the $$columns[$i]
= $cols[$i]; line and see if your test still works?

Peter

_______________________________________________
interchange-users mailing list
interchange-users@icdevgroup.org
http://www.icdevgroup.org/mailman/listinfo/interchange-users

Re: Redundant code in DBI.pm? [ In reply to ]

Mar 13, 2015, 6:57 AM

Post #10 of 16 (3115 views)

Quoting Peter (peter@pajamian.dhs.org):
> On 03/14/2015 01:58 AM, Mike Heins wrote:
> >>>> for (my $i = 0; $i < @$columns; $i++) {
> >>>> $cols[$i] = $$columns[$i];
> >>>> #::logDebug("checking column '$cols[$i]'");
> >>>> if(defined $key) {
> >>>> $keycol = $i if $cols[$i] eq $key;
> >>>> }
> >>>> if(defined $config->{COLUMN_DEF}->{$cols[$i]}) {
> >>>> $cols[$i] .= " " .
> >>>> $config->{COLUMN_DEF}->{$cols[$i]};
> >>>> }
> >>>> else {
> >>>> $cols[$i] .= " $def_type";
> >>>> }
> >>>> $$columns[$i] = $cols[$i];
> >>>> $$columns[$i] =~ s/\s+.*//;
> >>>> }
> >>
> >> The reason is that at you can define the type in the column
> >> label of the table, i.e.
> >>
> >> code price decimal(12) description varchar(200) image
>
> Oh, I see, it looks like the first of those two lines is still
> redundant, though.

No, I don't think so.

> I think if we were to remove the $$columns[$i] =
> $cols[$i] line and leave the next one in then the code would still work,
> the column type could be defined in the column and just the name would
> be left in @$columns.
>
> > Just tested that out of curiosity:
>
> Just to satisfy my own curiosity, can you comment out the $$columns[$i]
> = $cols[$i]; line and see if your test still works?
>

It seems to, and it seems safe and logical. But I would hesitate to
modify it at this point unless you are going to remove the bug/feature
completely.

I would be for removing it if we did some sanity checks on the final
product, i.e. check $$columns[i] for some field-name-like value.

--
Mike Heins
Perusion -- Expert Interchange Consulting http://www.perusion.com/
phone +1.765.253.4194 <mike@perusion.com>

Life isn't fair, but it's good. -- Regina Brett

_______________________________________________
interchange-users mailing list
interchange-users@icdevgroup.org
http://www.icdevgroup.org/mailman/listinfo/interchange-users

Re: Redundant code in DBI.pm? [ In reply to ]

Mar 13, 2015, 7:51 AM

Post #11 of 16 (3121 views)

On 03/14/2015 02:57 AM, Mike Heins wrote:
> It seems to, and it seems safe and logical. But I would hesitate to
> modify it at this point unless you are going to remove the bug/feature
> completely.

I'm adding a feature: Database foo QUOTE_IDENTIFIERS
...or more likely: DatabaseDefault QUOTE_IDENTIFIERS

See the ml thread "Quoting Identifiers" for more details.

> I would be for removing it if we did some sanity checks on the final
> product, i.e. check $$columns[i] for some field-name-like value.

Well I've managed to write my code so that those two lines aren't
affected by it whether QUOTE_IDENTIFIERS is set or not, so I can leave
it in. I just noticed those lines when I was digging around in that
section of code and I think the first can, at least, be removed without
consequence. But I can leave it in to be extra safe.

Peter

_______________________________________________
interchange-users mailing list
interchange-users@icdevgroup.org
http://www.icdevgroup.org/mailman/listinfo/interchange-users

Re: Redundant code in DBI.pm? [ In reply to ]

Mar 13, 2015, 7:59 AM

Post #12 of 16 (3124 views)

On 03/14/2015 03:51 AM, Peter wrote:
> Well I've managed to write my code so that those two lines aren't
> affected by it whether QUOTE_IDENTIFIERS is set or not, so I can leave
> it in. I just noticed those lines when I was digging around in that
> section of code and I think the first can, at least, be removed without
> consequence. But I can leave it in to be extra safe.

Oh, sorry, I take it back. I can't leave that line in. The reason is
as follows:

When QUOTE_IDENTIFIERS is enabled, $columns will contain the unquoted
column names as it did before, but @cols will contain the quoted column
names that will be passed to the SQL db. so if the column name is
copied from @cols to $columns it will break later code that needs the
unquoted name (not destined for an SQL query).

As I said earlier, and I believe you have confirmed in your tests,
removing the first of those two lines does not break the feature you
implemented with them, and the second regexp line still supports it.

Peter

_______________________________________________
interchange-users mailing list
interchange-users@icdevgroup.org
http://www.icdevgroup.org/mailman/listinfo/interchange-users

Re: Redundant code in DBI.pm? [ In reply to ]

Mar 13, 2015, 8:23 AM

Post #13 of 16 (3120 views)

Quoting Peter (peter@pajamian.dhs.org):
> On 03/14/2015 03:51 AM, Peter wrote:
> > Well I've managed to write my code so that those two lines aren't
> > affected by it whether QUOTE_IDENTIFIERS is set or not, so I can leave
> > it in. I just noticed those lines when I was digging around in that
> > section of code and I think the first can, at least, be removed without
> > consequence. But I can leave it in to be extra safe.
>
> Oh, sorry, I take it back. I can't leave that line in. The reason is
> as follows:
>
> When QUOTE_IDENTIFIERS is enabled, $columns will contain the unquoted
> column names as it did before, but @cols will contain the quoted column
> names that will be passed to the SQL db. so if the column name is
> copied from @cols to $columns it will break later code that needs the
> unquoted name (not destined for an SQL query).
>
> As I said earlier, and I believe you have confirmed in your tests,
> removing the first of those two lines does not break the feature you
> implemented with them, and the second regexp line still supports it.

OK.

If anyone wonders why I bothered with any of this, at one point I
was working on a database configuration system for Minivend 4. Spaces
in field names were not on my radar (even if they were supported in
SQL, which I don't think they were at that time), so it seemed like an
easy way to specify a field name and definition as in SQL. This never
was really used, as Akopia took over custody and we went in a different
direction.

--
Mike Heins
Perusion -- Expert Interchange Consulting http://www.perusion.com/
phone +1.765.253.4194 <mike@perusion.com>

There's nothing sweeter than life nor more precious than time.
-- Barney

_______________________________________________
interchange-users mailing list
interchange-users@icdevgroup.org
http://www.icdevgroup.org/mailman/listinfo/interchange-users

Re: Redundant code in DBI.pm? [ In reply to ]

Mar 13, 2015, 1:16 PM

Post #14 of 16 (3118 views)