Re: [COMMITTERS] pgsql: /contrib/cube improvements: Update

Log Message:
-----------
/contrib/cube improvements:

Update the calling convention for all external facing functions. By
external facing, I mean all functions that are directly referenced in
cube.sql. Prior to my update, all functions used the older V0 calling
convention. They now use V1.

New Functions:

cube(float[]), which makes a zero volume cube from a float array

cube(float[], float[]), which allows the user to create a cube from
two float arrays; one for the upper right and one for the lower left
coordinate.

cube_subset(cube, int4[]), to allow you to reorder or choose a subset of
dimensions from a cube, using index values specified in the array.

Joshua Reich

Modified Files:
--------------
pgsql/contrib/cube:
CHANGES (r1.2 -> r1.3)
(http://developer.postgresql.org/cvsweb.cgi/pgsql/contrib/cube/CHANGES.diff?r1=1.2&r2=1.3)
README.cube (r1.7 -> r1.8)
(http://developer.postgresql.org/cvsweb.cgi/pgsql/contrib/cube/README.cube.diff?r1=1.7&r2=1.8)
cube.c (r1.26 -> r1.27)
(http://developer.postgresql.org/cvsweb.cgi/pgsql/contrib/cube/cube.c.diff?r1=1.26&r2=1.27)
cube.sql.in (r1.17 -> r1.18)
(http://developer.postgresql.org/cvsweb.cgi/pgsql/contrib/cube/cube.sql.in.diff?r1=1.17&r2=1.18)
uninstall_cube.sql (r1.2 -> r1.3)
(http://developer.postgresql.org/cvsweb.cgi/pgsql/contrib/cube/uninstall_cube.sql.diff?r1=1.2&r2=1.3)
pgsql/contrib/cube/expected:
cube.out (r1.16 -> r1.17)
(http://developer.postgresql.org/cvsweb.cgi/pgsql/contrib/cube/expected/cube.out.diff?r1=1.16&r2=1.17)
pgsql/contrib/cube/sql:
cube.sql (r1.8 -> r1.9)
(http://developer.postgresql.org/cvsweb.cgi/pgsql/contrib/cube/sql/cube.sql.diff?r1=1.8&r2=1.9)

momjian(at)postgresql(dot)org (Bruce Momjian) writes:
> Log Message:
> -----------
> /contrib/cube improvements:

Should I point out that the buildfarm universally reports failure?

regards, tom lane

Can you point to me where I can get a regression diff for a failed
build. For the install-checks that I looked at on the buildfarm, I see:

============== running regression test queries ==============
test cube ... ok

Josh

Tom Lane wrote:
> momjian(at)postgresql(dot)org (Bruce Momjian) writes:
>
>> Log Message:
>> -----------
>> /contrib/cube improvements:
>>
>
> Should I point out that the buildfarm universally reports failure?
>
> regards, tom lane
>
> ---------------------------(end of broadcast)---------------------------
> TIP 1: if posting/reading through Usenet, please send an appropriate
> subscribe-nomail command to majordomo(at)postgresql(dot)org so that your
> message can get through to the mailing list cleanly
>

Joshua Reich <josh(at)root(dot)net> writes:
> Can you point to me where I can get a regression diff for a failed
> build.

It's all on the webpage, just scroll down ...

regards, tom lane

Ok. If I understand this correctly, earthdistance required cube. So,
I'll fix the test for earthdistance to work with the new cube code.

The problem is that there are new functions in cube.sql, so the output
is now different and breaks the diff (to state the obvious).

Josh Reich

Tom Lane wrote:
> Joshua Reich <josh(at)root(dot)net> writes:
>
>> Can you point to me where I can get a regression diff for a failed
>> build.
>>
>
> It's all on the webpage, just scroll down ...
>
> regards, tom lane
>
> ---------------------------(end of broadcast)---------------------------
> TIP 4: Have you searched our list archives?
>
> http://archives.postgresql.org
>

Stating the obvious, again - attached is my patch for earthdistance.

Apologies for breaking the build.

Josh

Joshua Reich wrote:
> Ok. If I understand this correctly, earthdistance required cube. So,
> I'll fix the test for earthdistance to work with the new cube code.
>
> The problem is that there are new functions in cube.sql, so the output
> is now different and breaks the diff (to state the obvious).
>
> Josh Reich
>
>
> Tom Lane wrote:
>> Joshua Reich <josh(at)root(dot)net> writes:
>>
>>> Can you point to me where I can get a regression diff for a failed
>>> build.
>>>
>>
>> It's all on the webpage, just scroll down ...
>>
>> regards, tom lane
>>
>> ---------------------------(end of broadcast)---------------------------
>> TIP 4: Have you searched our list archives?
>>
>> http://archives.postgresql.org
>>
>
>
> ---------------------------(end of broadcast)---------------------------
> TIP 1: if posting/reading through Usenet, please send an appropriate
> subscribe-nomail command to majordomo(at)postgresql(dot)org so that your
> message can get through to the mailing list cleanly

Joshua Reich <josh(at)root(dot)net> writes:
>> The problem is that there are new functions in cube.sql, so the output
>> is now different and breaks the diff (to state the obvious).

Actually, the new theory on this is that you should explicitly create
a shell type first:

CREATE TYPE cube;

and then create the functions, and then make the type "for real".

This will still involve a change in the earthdistance expected
output (down to zero expected NOTICEs) but if we're gonna fix it
then let's fix it right.

regards, tom lane

Tom Lane wrote:
> Joshua Reich <josh(at)root(dot)net> writes:
> >> The problem is that there are new functions in cube.sql, so the output
> >> is now different and breaks the diff (to state the obvious).
>
> Actually, the new theory on this is that you should explicitly create
> a shell type first:
>
> CREATE TYPE cube;
>
> and then create the functions, and then make the type "for real".
>
> This will still involve a change in the earthdistance expected
> output (down to zero expected NOTICEs) but if we're gonna fix it
> then let's fix it right.

OK, I will wait for the next patch. Funny I tested the cube regression
test, but not earthdistance.

--
Bruce Momjian bruce(at)momjian(dot)us
EnterpriseDB http://www.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian wrote:
> Tom Lane wrote:
>> Joshua Reich <josh(at)root(dot)net> writes:
>>>> The problem is that there are new functions in cube.sql, so the output
>>>> is now different and breaks the diff (to state the obvious).
>> Actually, the new theory on this is that you should explicitly create
>> a shell type first:
>>
>> CREATE TYPE cube;
>>
>> and then create the functions, and then make the type "for real".
>>
>> This will still involve a change in the earthdistance expected
>> output (down to zero expected NOTICEs) but if we're gonna fix it
>> then let's fix it right.
>
> OK, I will wait for the next patch. Funny I tested the cube regression
> test, but not earthdistance.

FYI: the buildfarm-script has a "no reporting" mode and can use
preexisting sourcetrees - that's what I regulary use to test patches and
modifications to the source.

Stefan

I'm not sure exactly what to do here. I tried simply doing

CREATE TYPE cube;

But I still get NOTICE's to the effect of "return type cube is only a
shell" or "argument type cube is only a shell".

I'm not sure how to do this to get zero NOTICES.

Advice appreciated.

Josh Reich

Tom Lane wrote:
> Joshua Reich <josh(at)root(dot)net> writes:
>>> The problem is that there are new functions in cube.sql, so the output
>>> is now different and breaks the diff (to state the obvious).
>
> Actually, the new theory on this is that you should explicitly create
> a shell type first:
>
> CREATE TYPE cube;
>
> and then create the functions, and then make the type "for real".
>
> This will still involve a change in the earthdistance expected
> output (down to zero expected NOTICEs) but if we're gonna fix it
> then let's fix it right.
>
> regards, tom lane
>
> ---------------------------(end of broadcast)---------------------------
> TIP 6: explain analyze is your friend

--
Joshua Reich
Finance and Corporate Development
ROOT Exchange, A Division of ROOT Markets
601 W. 26th St. / Suite 1500
New York, NY 10001
W - (212) 645 6320 x 7101
M / T - (646) 427 7959
E - josh(at)rootexchange(dot)com

Joshua Reich <josh(at)root(dot)net> writes:
> I'm not sure exactly what to do here. I tried simply doing
> CREATE TYPE cube;

> But I still get NOTICE's to the effect of "return type cube is only a
> shell" or "argument type cube is only a shell".

Hmm ... I remembered that we'd discussed suppressing those NOTICEs,
but I guess we didn't actually do it.

> I'm not sure how to do this to get zero NOTICES.

I think what I'd suggest is making earthdistance do

SET client_min_messages = warning;
include the file
RESET client_min_messages;

This will avoid any future flap in the test output if we do remove
those notices.

regards, tom lane

Ok. Here is a diff taken from the top of the contrib tree. I have
suppressed the notices in both the cube and earthdistance packages. All
tests pass.

Josh

Tom Lane wrote:
> Joshua Reich <josh(at)root(dot)net> writes:
>> I'm not sure exactly what to do here. I tried simply doing
>> CREATE TYPE cube;
>
>> But I still get NOTICE's to the effect of "return type cube is only a
>> shell" or "argument type cube is only a shell".
>
> Hmm ... I remembered that we'd discussed suppressing those NOTICEs,
> but I guess we didn't actually do it.
>
>> I'm not sure how to do this to get zero NOTICES.
>
> I think what I'd suggest is making earthdistance do
>
> SET client_min_messages = warning;
> include the file
> RESET client_min_messages;
>
> This will avoid any future flap in the test output if we do remove
> those notices.
>
> regards, tom lane
>
> ---------------------------(end of broadcast)---------------------------
> TIP 1: if posting/reading through Usenet, please send an appropriate
> subscribe-nomail command to majordomo(at)postgresql(dot)org so that your
> message can get through to the mailing list cleanly

Joshua Reich wrote:

> Ok. Here is a diff taken from the top of the contrib tree. I have
> suppressed the notices in both the cube and earthdistance packages.
> All tests pass.
>

Doesn't this contain parts that have been already applied? It looks to
me like you need to do a cvs update (and then a subsequent clean and
test) before generating the diff.

cheers

andrew

You are correct.

Patch against the latest HEAD attached.

Josh

Andrew Dunstan wrote:
> Joshua Reich wrote:
>
>> Ok. Here is a diff taken from the top of the contrib tree. I have
>> suppressed the notices in both the cube and earthdistance packages.
>> All tests pass.
>>
>
> Doesn't this contain parts that have been already applied? It looks to
> me like you need to do a cvs update (and then a subsequent clean and
> test) before generating the diff.
>
> cheers
>
> andrew
>
> ---------------------------(end of broadcast)---------------------------
> TIP 4: Have you searched our list archives?
>
> http://archives.postgresql.org

Joshua Reich wrote:
> You are correct.
>
> Patch against the latest HEAD attached.
>

I have applied this and made an attempt to fix cube/expected/cube_1.out
- we will still need to keep an eye on the various cube tests.

cheers

andrew

I wrote:

>
> I have applied this and made an attempt to fix
> cube/expected/cube_1.out - we will still need to keep an eye on the
> various cube tests.
>

It looks like with the current tests we will need at least one more
possible results file (gazelle and asp on buildfarm agree about negative
zero but disagree about ordering, so we would need to handle both
variants which were previously handed in just cube_1.out). That would
make four for this little module. Is it just me or does that seem
excessive? Can we possibly simplify things a bit?

cheers

andrew

I am uncomfortable with the fact that the ordering is different on
different architectures. AFAIK, all I did was port the ordering function
from V0 to V1.

Short of me continually submitting patches until I fix it, is there an
easier way, or some pointers as to what I should be looking for in the code?

The function in question, I believe, is cube_cmp_v0().

Josh

Andrew Dunstan wrote:
> I wrote:
>
>>
>> I have applied this and made an attempt to fix
>> cube/expected/cube_1.out - we will still need to keep an eye on the
>> various cube tests.
>>
>
> It looks like with the current tests we will need at least one more
> possible results file (gazelle and asp on buildfarm agree about negative
> zero but disagree about ordering, so we would need to handle both
> variants which were previously handed in just cube_1.out). That would
> make four for this little module. Is it just me or does that seem
> excessive? Can we possibly simplify things a bit?
>
> cheers
>
> andrew

--
Joshua Reich
Finance and Corporate Development
ROOT Exchange, A Division of ROOT Markets
601 W. 26th St. / Suite 1500
New York, NY 10001
W - (212) 645 6320 x 7101
M / T - (646) 427 7959
E - josh(at)rootexchange(dot)com

Is this being worked on?

---------------------------------------------------------------------------

Tom Lane wrote:
> Joshua Reich <josh(at)root(dot)net> writes:
> > I'm not sure exactly what to do here. I tried simply doing
> > CREATE TYPE cube;
>
> > But I still get NOTICE's to the effect of "return type cube is only a
> > shell" or "argument type cube is only a shell".
>
> Hmm ... I remembered that we'd discussed suppressing those NOTICEs,
> but I guess we didn't actually do it.
>
> > I'm not sure how to do this to get zero NOTICES.
>
> I think what I'd suggest is making earthdistance do
>
> SET client_min_messages = warning;
> include the file
> RESET client_min_messages;
>
> This will avoid any future flap in the test output if we do remove
> those notices.
>
> regards, tom lane
>
> ---------------------------(end of broadcast)---------------------------
> TIP 1: if posting/reading through Usenet, please send an appropriate
> subscribe-nomail command to majordomo(at)postgresql(dot)org so that your
> message can get through to the mailing list cleanly

--
Bruce Momjian bruce(at)momjian(dot)us
EnterpriseDB http://www.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian <bruce(at)momjian(dot)us> writes:
> Is this being worked on?

I think we dealt with that already.

regards, tom lane