Skip Menu |
 

This queue is for tickets about the Tangram CPAN distribution.

Report information
The Basics
Id: 24324
Status: open
Priority: 0/
Queue: Tangram

People
Owner: Nobody in particular
Requestors: pt [...] tchorbadjiev.com
Cc:
AdminCc:

Bug Information
Severity: (no value)
Broken in:
  • 2.04
  • 2.05
  • 2.06
  • 2.07
  • 2.07_03
  • 2.07_04
  • 2.07_05
  • 2.07_06
  • 2.07_07
  • 2.08
Fixed in:
  • 2.09
  • 2.10



Subject: Tangram::Storage::count (2.07_07) patch
Date: Thu, 11 Jan 2007 19:18:26 +0200
To: bug-Tangram [...] rt.cpan.org
From: Assen Tchorbadjiev <pt [...] tchorbadjiev.com>
Download (untitled) / with headers
text/plain 742b
Hello, attached is a [minor] patch for the Tangram::Storage module (from 2.07_07). Inside the count() sub is a call like: Show quoted text
> $objects->insert($expr->objects);
Since $expr usually end up being Tangram::QueryObject(s), they don't have the objectS() method, but only object(). I've looked against the stable version, but the count() sub is implemented in a different way, w/o using such a call on the $expr argument. The patch effectively calls object(), instead of objectS(). It works for me at least (and it gives the correct counts). The code I'm using is basically like: Show quoted text
> $r1 = $remote-data-type-as-per-schema->new(); > $r2 = $remote-data-type-as-per-schema->{field}->eq(xxx); > Tangram::Storage->count( $r1, $r2 );
Regards, Assen
Download Storage.pm.patch
text/x-diff 260b
--- Storage.pm.orig Thu Jan 11 18:49:41 2007 +++ Storage.pm Thu Jan 11 18:49:44 2007 @@ -1110,7 +1110,7 @@ { my $expr = shift; $target = $expr->{expr}; - $objects->insert($expr->objects); + $objects->insert($expr->object); $filter = shift; }
From: SAMV [...] cpan.org
On Thu Jan 11 12:06:34 2007, pt@tchorbadjiev.com wrote: Show quoted text
> > The patch effectively calls object(), instead of objectS(). > > It works for me at least (and it gives the correct counts). > The code I'm using is basically like: >
> > $r1 = $remote-data-type-as-per-schema->new(); > > $r2 = $remote-data-type-as-per-schema->{field}->eq(xxx); > > Tangram::Storage->count( $r1, $r2 );
That doesn't look quite right, $r1 should be a remote object you got with Tangram::Storage->remote("ClassName"). While the change might be the right thing to do (I'm not positive right now, but quite possibly), proving what we're doing using the regression test suite would be better. Can you please work your use case into a test script which fails against 2.07_07 and succeeds with your patch - or feel free to modify an existing test script and send a patch on that. Currently, ->count() is only tested in t/springfield/82-aggregate.t and t/musicstore/01-simple.t. As things stand, I can't apply your patch against the head Tangram release. Sam.
Subject: Re: [rt.cpan.org #24324] Tangram::Storage::count (2.07_07) patch
Date: Fri, 12 Jan 2007 00:03:41 +0200
To: bug-Tangram [...] rt.cpan.org
From: Assen Tchorbadjiev <pt [...] tchorbadjiev.com>
Download (untitled) / with headers
text/plain 1.9k
Show quoted text
>> That doesn't look quite right, $r1 should be a remote object you got..
Language error on my part, sorry. Yes, its a remote object retrieved via Storage->remote. Attached is a patch against: t/musicstore/01-simple, I've added an explicit $expr object // $storage->remote("CD") // as the first argument to the count() call , and the output is: Show quoted text
> # make test > [ .. ] > t/musicstore/01-simple........ok 2/24 > Can't locate object method "objects" via package "Tangram::QueryObject" > at /usr/home/mason/html/anet-cp/lib3/build/Tangram-2.07_07/blib/lib/Tangram/Storage.pm line 1113, <CONFIG> line 1 > [ .. ]
btw, the pod docs say that count() should be invoked as: Show quoted text
> count( $expr, [$filter] )
The $filter is marked as optional, but the test case above uses only the $filter when calling the method. Regards, Assen via RT wrote: Show quoted text
> <URL: http://rt.cpan.org/Ticket/Display.html?id=24324 > > > On Thu Jan 11 12:06:34 2007, pt@tchorbadjiev.com wrote:
>> The patch effectively calls object(), instead of objectS(). >> >> It works for me at least (and it gives the correct counts). >> The code I'm using is basically like: >>
>>> $r1 = $remote-data-type-as-per-schema->new(); >>> $r2 = $remote-data-type-as-per-schema->{field}->eq(xxx); >>> Tangram::Storage->count( $r1, $r2 );
> > That doesn't look quite right, $r1 should be a remote object you got > with Tangram::Storage->remote("ClassName"). > > While the change might be the right thing to do (I'm not positive right > now, but quite possibly), proving what we're doing using the regression > test suite would be better. > > Can you please work your use case into a test script which fails against > 2.07_07 and succeeds with your patch - or feel free to modify an > existing test script and send a patch on that. Currently, ->count() is > only tested in t/springfield/82-aggregate.t and > t/musicstore/01-simple.t. As things stand, I can't apply your patch > against the head Tangram release. > > Sam. > > >
Download 01-simple.t.patch
text/x-diff 468b
--- t/musicstore/01-simple.t.orig Thu Jan 11 23:53:10 2007 +++ t/musicstore/01-simple.t Thu Jan 11 23:54:32 2007 @@ -134,7 +134,7 @@ is(@cds, 3, "Found three CDs by artists matching %beat%"); # if we just wanted the count: - my ($count) = $storage->count($filter); + my ($count) = $storage->count( $r_cd, $filter); # count with $expr as 1st arg is($count, 3, "Can do simple COUNT() queries"); # maybe some other aggregation type queries:
From: SAMV [...] cpan.org
Download (untitled) / with headers
text/plain 714b
On Thu Jan 11 17:05:32 2007, pt@tchorbadjiev.com wrote: Show quoted text
> > Attached is a patch against: t/musicstore/01-simple, I've added an > explicit $expr object // $storage->remote("CD") // as the first > argument > to the count() call , and the output is:
[...] Show quoted text
> btw, the pod docs say that count() should be invoked as:
> > count( $expr, [$filter] )
Well spotted. I have updated the test case (see http://utsl.gen.nz/gitweb/?p=Tangram;a=commitdiff;h=c06d12d1226c2a3c881194b88f3d5c32e5e6244f), but it seems that it passes on master already. Checking other versions, it seems that 2.09 and above pass this test. Are you able to try a newer version? I have updated the documentation to reflect this flexibility. Sam.
Subject: Re: [rt.cpan.org #24324] Tangram::Storage::count (2.07_07) patch
Date: Fri, 12 Jan 2007 12:38:53 +0200
To: bug-Tangram [...] rt.cpan.org
From: Assen Tchorbadjiev <pt [...] tchorbadjiev.com>
Download (untitled) / with headers
text/plain 1007b
Show quoted text
>> [ .. ] >> it seems that 2.09 and above pass this test. Are you able to try a >> newer version?
Yes, 2.10 passes for sure (with your patch on the test case). Regards, Assen via RT wrote: Show quoted text
> <URL: http://rt.cpan.org/Ticket/Display.html?id=24324 > > > On Thu Jan 11 17:05:32 2007, pt@tchorbadjiev.com wrote:
>> Attached is a patch against: t/musicstore/01-simple, I've added an >> explicit $expr object // $storage->remote("CD") // as the first >> argument >> to the count() call , and the output is:
> [...]
>> btw, the pod docs say that count() should be invoked as:
>>> count( $expr, [$filter] )
> > Well spotted. I have updated the test case (see > http://utsl.gen.nz/gitweb/?p=Tangram;a=commitdiff;h=c06d12d1226c2a3c881194b88f3d5c32e5e6244f), > but it seems that it passes on master already. Checking other versions, > it seems that 2.09 and above pass this test. Are you able to try a > newer version? > > I have updated the documentation to reflect this flexibility. > > Sam. > > >


This service is sponsored and maintained by Best Practical Solutions and runs on Perl.org infrastructure.

Please report any issues with rt.cpan.org to rt-cpan-admin@bestpractical.com.