array support patch phase 1 patch

This patch is starting to get large enough that it is a challenge to
keep in sync with cvs, and it is reasonably complete as a package, so I
was hoping it could be reviewed and committed as "array support phase
1". The most notable missing item is documentation, but if possible I'd
like to defer that until a few more items are complete (see
"Yet-to-come" below).

If a more complete package (e.g. want to wait for items 1-5 of
"Yet-to-come") is preferred, please let me know and I'll keep plugging
along.

Note that the gram.y changes are a bit ugly, but I struggled with
getting anything less ugly to work.

As is, it passes all regression tests (make installcheck), and covers
the following:

I can confirm that this patch applies cleanly, compiles and passes all
regression tests on FreeBSD/alpha.

Chris

Joe Conway <mail@joeconway.com> writes:

This patch is starting to get large enough that it is a challenge to
keep in sync with cvs, and it is reasonably complete as a package, so I
was hoping it could be reviewed and committed as "array support phase
1".

I looked over this patch a little bit, mostly at the anyarray/anyelement
changes; I didn't study the ArrayExpr stuff (except to notice that yeah,
the grammar change is very ugly; can't it be made independent of the
number of levels?).

I like the anyarray/anyelement stuff with the exception of the
actualRetType field added to FunctionCallInfoData. In the first place,
that's the wrong place for such data; FunctionCallInfoData should only
contain data that will change for each call, which resolved type won't.
This should go into FmgrInfo, perhaps, or maybe we could treat it as a
call-context item (though I think we might have conflicts with existing
uses of the context link). A bigger gripe is that passing only
actualRetType isn't future-proof. It doesn't help functions that take
anyelement but return a fixed type; they won't know what the argument type
is. And it won't scale if we end up adding anyarray2/anyelement2/etc to
allow multiple generic parameter types; at most one of the actual
parameter types could be made visible to the function. I see your concern
here, but I think the only adequate solution is to make sure the function
can learn the types of all its arguments, as well as the assigned return
type. (Of course it could deduce the latter from the former, but we may
as well save it the effort, especially since it would have to repeat
parse-time catalog lookups in many interesting cases.)

A notion that I've toyed with more than once is to allow a function
to get at the parse tree representation for its call (ie, the FuncExpr
or OpExpr node). If we did that, a function could learn all these
types by examining the parse tree, and it could learn other things
besides. A disadvantage is that functions would have to be ready to
cope with all the wooliness of parse trees. It'd probably be bright
to make some convenience functions "get my return type", "get type
of my n'th argument" to localize this logic as much as possible.

In short then, what about passing an Expr* link in FmgrInfo as a
substitute for actualRetType? (An additional advantage of this way
is that it's obvious whether or not the information has been provided,
which it would not be for, say, functions invoked via DirectFunctionCallN.
The patch as it stands fails silently if a polymorphic function is called
from a call site that doesn't provide the needed info.)

regards, tom lane

Tom Lane wrote:

I looked over this patch a little bit, mostly at the anyarray/anyelement
changes; I didn't study the ArrayExpr stuff (except to notice that yeah,
the grammar change is very ugly; can't it be made independent of the
number of levels?).

I struggled with it for quite a while, but then again, I can't claim
yacc grammar as a particularly strong point. I kept running into
reduce/reduce errors with anything completely recursive, or when it did
work, I found myself restricted to one level of nesting. I'll try again
-- any suggestions for something similar to study?

parameter types could be made visible to the function. I see your concern
here, but I think the only adequate solution is to make sure the function
can learn the types of all its arguments, as well as the assigned return
type. (Of course it could deduce the latter from the former, but we may
as well save it the effort, especially since it would have to repeat
parse-time catalog lookups in many interesting cases.)

I came to the same conclusion with ArrayExpr/ArrayExprState -- you can
always deduce the array type from the element type and vice-versa, but
it seemed costly to do in the executor when it can be done just once in
the parser.

A notion that I've toyed with more than once is to allow a function
to get at the parse tree representation for its call (ie, the FuncExpr
or OpExpr node). If we did that, a function could learn all these
types by examining the parse tree, and it could learn other things
besides. A disadvantage is that functions would have to be ready to
cope with all the wooliness of parse trees. It'd probably be bright
to make some convenience functions "get my return type", "get type
of my n'th argument" to localize this logic as much as possible.

OK -- I'll take a look at that option.

In short then, what about passing an Expr* link in FmgrInfo as a
substitute for actualRetType? (An additional advantage of this way
is that it's obvious whether or not the information has been provided,
which it would not be for, say, functions invoked via DirectFunctionCallN.
The patch as it stands fails silently if a polymorphic function is called
from a call site that doesn't provide the needed info.)

Sounds good to me -- thanks for the quick feedback!

A question -- I was thinking that we will need to allow one array type
to be coerced into another in this brave new world (e.g.
ARRAY[1,2,3]::float8[] results in "ERROR: Cannot cast type integer[] to
double precision[]"). My plan was to check the source and target
datatypes in can_coerce_type() and coerce_type() to see if both are
array types. If so, use the element types to determine whether we can
coerce, and loop over the array to coerce, etc.

There are a few issues that I can think of with this (and undoubtedly
you will have some others ;-)):
- this would add at least one cache lookup to every call to
can_coerce_type
- if we pass by can_coerce_type with an array, we'd need to do the same
cache lookup in coerce_type unless we modify the api of the related
functions to pass along the "isarray" status of the datatype
- currently the only reliable(?) method to determine if a
datatype is a varlena array is to check for '_' as the first
character of the type name -- it seems wrong to depend on that
forever.

Any suggestions? I was toying with the idea that an isarray attribute
should be added to pg_type -- thoughts on that?

Joe

Joe Conway <mail@joeconway.com> writes:

Any suggestions? I was toying with the idea that an isarray attribute
should be added to pg_type -- thoughts on that?

Don't think you need it to go in that direction: the property of having
nonzero typelem indicates that something is an array.

The real issue is whether we should add a link to let you find the array
type given the base type (without relying on the '_' naming convention).
I'm undecided about that...

regards, tom lane

Tom Lane wrote:

Joe Conway <mail@joeconway.com> writes:

Any suggestions? I was toying with the idea that an isarray attribute
should be added to pg_type -- thoughts on that?

Don't think you need it to go in that direction: the property of having
nonzero typelem indicates that something is an array.

But I don't think of the following as arrays, at least not in the same
sense of _text, etc.:

regression=# select typname from pg_type where typelem != 0 and typname
not like '\\_%';
typname
------------
name
int2vector
oidvector
point
lseg
box
line
(7 rows)

Joe

Joe Conway <mail@joeconway.com> writes:

Tom Lane wrote:

Don't think you need it to go in that direction: the property of having
nonzero typelem indicates that something is an array.

But I don't think of the following as arrays, at least not in the same
sense of _text, etc.:

If you want to reject fixed-length array types for this purpose, you can
also insist on typlen < 0.

regards, tom lane

Tom Lane wrote:

I looked over this patch a little bit, mostly at the anyarray/anyelement
changes; I didn't study the ArrayExpr stuff (except to notice that yeah,
the grammar change is very ugly; can't it be made independent of the
number of levels?).

[...snip...]

In short then, what about passing an Expr* link in FmgrInfo as a
substitute for actualRetType? (An additional advantage of this way
is that it's obvious whether or not the information has been provided,
which it would not be for, say, functions invoked via DirectFunctionCallN.
The patch as it stands fails silently if a polymorphic function is called
from a call site that doesn't provide the needed info.)

Here's a new patch that fixes the grammar ugliness (amazing how easily I
got that to work after a week of not looking at it!) and adds an
(Expr *) link in FmgrInfo. It passes all regression tests. If there are
no objections, please apply.

(as phase 1 -- see:
http://archives.postgresql.org/pgsql-patches/2003-03/msg00191.php
for what the patch covers and the planned "yet-to-come").

Thanks,

Joe

Joe Conway wrote:

Here's a new patch that fixes the grammar ugliness (amazing how easily I
got that to work after a week of not looking at it!) and adds an
(Expr *) link in FmgrInfo. It passes all regression tests. If there are
no objections, please apply.

I just found a huge bogosity in this patch (ExecEvalArray)-- I must have
been tired, and my test cases were obviously too simple ;(

I'm working on a fix right now, but I guess I ought to add some good
test cases to the regression suite before submitting again. Please
retract this patch for the moment, and I'll send in the fix with
regression test adjustments in the next few days.

Sorry if I wasted anyone's time!

Joe

Joe Conway wrote:

I just found a huge bogosity in this patch (ExecEvalArray)-- I must have
been tired, and my test cases were obviously too simple ;(

I'm working on a fix right now, but I guess I ought to add some good
test cases to the regression suite before submitting again. Please
retract this patch for the moment, and I'll send in the fix with
regression test adjustments in the next few days.

Here's an updated patch that fixes the above mentioned issues. I also
added significantly more functionality.

It needs a good review, but everything seems to work, it passes all
regression tests**, and I think it is ready to apply (I needed to change
some of the regression tests, so someone please take a close look at
those too).

**except horology due to DST, and for some reason that I don't yet
understand, /expected/misc.out loses my changes every time I update from
CVS -- is misc.out autogenerated?

It covers the following:
----------------------------------------------------------------------
1. Support for polymorphic functions, accepting and returning ANYARRAY
and ANYELEMENT datatypes that are "tied" to each other and resolved
to an actual type at runtime. This also includes the ability to
define aggregates using the polymorphic functions.

2. Array handling functions:
- singleton_array(ANYELEMENT) returns ANYARRAY
- array_append(ANYARRAY, ANYELEMENT) returns ANYARRAY
- array_prepend(ANYELEMENT, ANYARRAY) returns ANYARRAY
- array_accum(ANYARRAY, ANYELEMENT) returns ANYARRAY
- array_assign(ANYARRAY, int, ANYELEMENT) returns ANYARRAY
- array_subscript(ANYARRAY, int) returns ANYELEMENT
- array_cat(ANYARRAY, ANYARRAY) returns ANYARRAY

3. Grammar and underlying support for the following (examples):

create table foo(f1 integer ARRAY);
create table foo(f1 integer ARRAY[]);
create table foo(f1 integer ARRAY[x]);
create table foo(f1 integer ARRAY[][]); (and more [] etc)
create table foo(f1 integer ARRAY[x][y]); (and more [] etc)

select ARRAY[1,2,3];
select ARRAY[[1,2,3],[4,5,6]];
select ARRAY[ARRAY[1,2,3],ARRAY[4,5,6]];
...etc up to 6 dimensions

select ARRAY[1,2] || 3;
select 0 || ARRAY[1,2];
select ARRAY[1,2] || ARRAY[3,4];
select ARRAY[1,2] || ARRAY[[3,4],[5,6]];
select ARRAY[[1,2],[3,4]] || ARRAY[5,6];
select ARRAY(select oid from pg_class order by relname);

select ARRAY[[1,2,3],[4,5,6]]::float8[];
select ARRAY[[1,2,3],[4,5,6]]::text[];
select CAST(ARRAY[[1,2,3],[4,5,6]] AS float8[]);

4. Duplicated contrib/array functionality (and then some) in the
backend using polymorphic functions and operators. Also cleared
out the datatype specific "=" operators. Now there is just one
polymorphic one. If the patch is applied, I think contrib/array can
be safely retired. Examples:

SELECT ARRAY[1,2,3] *= 2 AS "TRUE";
SELECT ARRAY[1,2,3] *<> 4 AS "TRUE";
SELECT ARRAY[1,2,3] *< 2 AS "TRUE";
SELECT ARRAY[1,2,3] *> 2 AS "TRUE";
SELECT ARRAY[1,2,3] *<= 1 AS "TRUE";
SELECT ARRAY[1,2,3] *>= 3 AS "TRUE";
SELECT ARRAY['abc','def','qwerty'] *~ 'wer' AS "TRUE";
SELECT ARRAY['abc','def','qwerty'] *!~ 'xyz' AS "TRUE";
SELECT ARRAY['abc','def','qwerty'] *~~ '_wer%' AS "TRUE";
SELECT 'hello' *~~ ARRAY['abc','def','_ell%'] AS "TRUE";
SELECT ARRAY['abc','def','qwerty'] *!~~ '%xyz%' AS "TRUE";

5. Side note: I added ANYARRAY1 and ANYELEMENT1 in this version. I
needed ANYARRAY1 for polymorphic array coercion, but did not add
support for it to be used in any other way. I added ANYELEMENT1
only for symmetry, and did not use it at all.

Still needs to be done (in roughly this order):
----------------------------------------------------------------------
1. Functions:
- str_to_array(str TEXT, delim TEXT) returns TEXT[]
- array_to_str(array ANYARRAY, delim TEXT) returns TEXT

2. Documentation update:
Update "User's Guide"->"Data Types"->"Arrays" documentation
create a new section: "User's Guide"->
"Functions and Operators"->
"Array Functions and Operators"

4. PL/pgSQL support for polymorphic types

5. SQL function support for polymorphic types.

6. Move as much of contrib/intarray into backend as makes sense,
including migration to use polymorphic semantics (therefore make
work on other than int where possible). Note: this may not happen
for 7.4 as it looks to be fairly involved, at least at first glance.

7. Additional documentation and regression test updates

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

Hopefully I haven't forgotten anything. If so, be gentle ;-)

Regards,

Joe

Joe Conway <mail@joeconway.com> writes:

**except horology due to DST, and for some reason that I don't yet
understand, /expected/misc.out loses my changes every time I update from
CVS -- is misc.out autogenerated?

Yeah. You need to back-patch src/test/regress/output/misc.source ...

regards, tom lane

Joe Conway writes:

select ARRAY[1,2,3];
select ARRAY[[1,2,3],[4,5,6]];
select ARRAY[ARRAY[1,2,3],ARRAY[4,5,6]];
...etc up to 6 dimensions

Why only 6?

4. Duplicated contrib/array functionality (and then some) in the
backend using polymorphic functions and operators.

SELECT ARRAY[1,2,3] *= 2 AS "TRUE";
SELECT ARRAY[1,2,3] *<> 4 AS "TRUE";

Couldn't this kind of operation be handled more cleanly (at least
semantically speaking), if we provide a function that converts an array to
a set and then use standard set searching operations? For example,

SELECT 2 IN TABLE(ARRAY[1,2,3]);

5. Side note: I added ANYARRAY1 and ANYELEMENT1 in this version.

Doing what?

--
Peter Eisentraut peter_e@gmx.net

Joe Conway kirjutas E, 07.04.2003 kell 05:12:

Joe Conway wrote:
It covers the following:
----------------------------------------------------------------------
1. Support for polymorphic functions, accepting and returning ANYARRAY
and ANYELEMENT datatypes that are "tied" to each other and resolved
to an actual type at runtime. This also includes the ability to
define aggregates using the polymorphic functions.

2. Array handling functions:
- singleton_array(ANYELEMENT) returns ANYARRAY
- array_append(ANYARRAY, ANYELEMENT) returns ANYARRAY
- array_prepend(ANYELEMENT, ANYARRAY) returns ANYARRAY
- array_accum(ANYARRAY, ANYELEMENT) returns ANYARRAY
- array_assign(ANYARRAY, int, ANYELEMENT) returns ANYARRAY
- array_subscript(ANYARRAY, int) returns ANYELEMENT
- array_cat(ANYARRAY, ANYARRAY) returns ANYARRAY

How hard would it be to add

array_eq, array_ne, array_gt, array_le and corresponding operators

SELECT ARRAY[1,2,3] = ARRAY[1,2,3]; # --> TRUE
SELECT ARRAY[1,2,3] < ARRAY[1,2,3]; # --> FALSE
SELECT ARRAY[1,2,3] <= ARRAY[1,2,3]; # --> TRUE
SELECT ARRAY[1,2,3] > ARRAY[1,2,3]; # --> FALSE
SELECT ARRAY[1,2,3] >= ARRAY[1,2,3]; # --> TRUE

I'd assume them to behave like string comparisons, i.e shorter subarray
is smaller:

SELECT ARRAY[1,2] < ARRAY[1,2,3]; # --> FALSE

Support for sorting and b-tree indexing could be nice too.

Still needs to be done (in roughly this order):
----------------------------------------------------------------------
1. Functions:
- str_to_array(str TEXT, delim TEXT) returns TEXT[]
- array_to_str(array ANYARRAY, delim TEXT) returns TEXT

2. Documentation update:
Update "User's Guide"->"Data Types"->"Arrays" documentation
create a new section: "User's Guide"->
"Functions and Operators"->
"Array Functions and Operators"

4. PL/pgSQL support for polymorphic types

Where should one start to add PL/Python support for polymorphic types ?

6. Move as much of contrib/intarray into backend as makes sense,
including migration to use polymorphic semantics (therefore make
work on other than int where possible). Note: this may not happen
for 7.4 as it looks to be fairly involved, at least at first glance.

What about moving contrib/intagg into backend ?

(And converting it into ANYagg on the way ;)

--------------------
Hannu

Peter Eisentraut <peter_e@gmx.net> writes:

Joe Conway writes:

...etc up to 6 dimensions

Why only 6?

See uses of MAXDIM. If you feel that 6 isn't enough, I wouldn't have
a problem with raising MAXDIM to 10 or so. I don't think it's worth
trying to eliminate the limit completely; that would add palloc overhead
to every array operation, for a feature people are unlikely to use.

4. Duplicated contrib/array functionality (and then some) in the
backend using polymorphic functions and operators.

Couldn't this kind of operation be handled more cleanly (at least
semantically speaking), if we provide a function that converts an array to
a set and then use standard set searching operations? For example,
SELECT 2 IN TABLE(ARRAY[1,2,3]);

Not sure about that. Is there any guidance in the SQL 200x spec about
what they expect people to actually *do* with the ARRAY[] syntax?

I'm currently working over the patch, but am not going to commit this
part (yet), since I have some problems with the implementation anyway.

5. Side note: I added ANYARRAY1 and ANYELEMENT1 in this version.

Doing what?

I'm hoping to avoid committing those, as they don't seem to be
completely implemented in this patch. They may be required to
represent the behavior of array_coerce() though. Not sure yet.

regards, tom lane

Peter Eisentraut wrote:

Joe Conway writes:

...etc up to 6 dimensions

Why only 6?

As Tom mentioned MAXDIM is currently set to 6. I can't imagine many
people using anything over 3 or maybe 4 dimensions.

4. Duplicated contrib/array functionality (and then some) in the
backend using polymorphic functions and operators.

SELECT ARRAY[1,2,3] *= 2 AS "TRUE";
SELECT ARRAY[1,2,3] *<> 4 AS "TRUE";

Couldn't this kind of operation be handled more cleanly (at least
semantically speaking), if we provide a function that converts an array to
a set and then use standard set searching operations? For example,

SELECT 2 IN TABLE(ARRAY[1,2,3]);

I thought about that too. It wouldn't be general enough to handle other
operators though, so I decided to stick with the already somewhat
established contrib/array operators. It sounds like Tom has some
concerns with those anyway, so in the meantime I'll take another look at
SQL200x to see if this is covered somewhere.

5. Side note: I added ANYARRAY1 and ANYELEMENT1 in this version.

Doing what?

As I said:
" I needed ANYARRAY1 for polymorphic array coercion, but did not add
support for it to be used in any other way. I added ANYELEMENT1
only for symmetry, and did not use it at all."

The problem lies in that fact that more than one reference to ANYARRAY
in the argument list of a function implies that the said arrays are all
the same data type. This doesn't work for a coercion function where you
need two array arguments, of arbitrary types, but that are not the same
as each other. I could not see any other clean way to achieve this.

I specifically did not add any other support for these data types
because there is not yet a strong case that user defined functions need
this capability. Of course if there is a strong feeling that they should
have full fledged support similar to ANYARRAY and ANYELEMENT, I'll
gladly add it.

Joe

Joe Conway <mail@joeconway.com> writes:

5. Side note: I added ANYARRAY1 and ANYELEMENT1 in this version.

Doing what?

The problem lies in that fact that more than one reference to ANYARRAY
in the argument list of a function implies that the said arrays are all
the same data type. This doesn't work for a coercion function where you
need two array arguments, of arbitrary types, but that are not the same
as each other. I could not see any other clean way to achieve this.

What I'm currently thinking of is declaring array_coerce to take and
return ANYARRAY (which will make it a no-op if someone tries to invoke
it by hand). The coercion code will have to force the output type to
the desired thing after building the FuncExpr node. But the other way
needs special-case code in parse_coerce too, and it doesn't do something
reasonable if array_coerce is invoked by hand.

regards, tom lane

Hannu Krosing wrote:

How hard would it be to add

array_eq, array_ne, array_gt, array_le and corresponding operators

SELECT ARRAY[1,2,3] = ARRAY[1,2,3]; # --> TRUE
SELECT ARRAY[1,2,3] < ARRAY[1,2,3]; # --> FALSE
SELECT ARRAY[1,2,3] <= ARRAY[1,2,3]; # --> TRUE
SELECT ARRAY[1,2,3] > ARRAY[1,2,3]; # --> FALSE
SELECT ARRAY[1,2,3] >= ARRAY[1,2,3]; # --> TRUE

I'd assume them to behave like string comparisons, i.e shorter subarray
is smaller:

SELECT ARRAY[1,2] < ARRAY[1,2,3]; # --> FALSE

Support for sorting and b-tree indexing could be nice too.

I thought briefly about this, but it wasn't immediately clear what the
semantics ought to be in all cases. I've also spent literally all my
available "hacking" time for the last several weeks just to get to the
patch submitted. I'd like to see at least some of it committed before I
take on anything new ;-)

If you want to propose in detail how these would behave - including:
- different length arrays
- different dimension arrays
- is comparison by ordinal position, or is each element compared to
all elements of the other side, e.g. is
(ARRAY[1,2,3] < ARRAY[2,3,4]) TRUE or FALSE?
If you compare by ordinal position TRUE, but in the latter case
FALSE

Where should one start to add PL/Python support for polymorphic types ?

Not entirely sure. In plperl, pltcl, and plr there is a section of code
in the compile function that looks something like:

/* Disallow pseudotype result, except VOID */
if (typeStruct->typtype == 'p')
{
if (procStruct->prorettype == VOIDOID)
/* okay */ ;
else if (procStruct->prorettype == TRIGGEROID)
{
free(prodesc->proname);
free(prodesc);
elog(ERROR, "plperl functions cannot return type %s"
"\n\texcept when used as triggers",
format_type_be(procStruct->prorettype));

And something similar for arguments. But I don't at quick glance see
something like this in plpython.

This needs to be changed to allow types ANYARRAYOID and ANYELEMENTOID.
Then you need to do the right thing with the arguments/return type. For
example, from plr:

#define GET_PROARGS(pronargs_, proargtypes_) \
do { \
int i; \
pronargs_ = procStruct->pronargs; \
for (i = 0; i < pronargs_; i++) \
{ \
if (procStruct->proargtypes[i] == ANYARRAYOID || \
procStruct->proargtypes[i] == ANYELEMENTOID) \
{ \
proargtypes_[i] = get_expr_argtype(fcinfo, i); \
if (proargtypes_[i] == InvalidOid) \
proargtypes_[i] = procStruct->proargtypes[i]; \
} \
else \
proargtypes_[i] = procStruct->proargtypes[i]; \
} \
} while (0)

This grabs the parser determined runtime datatype from the expression
node that has been added to FmgrInfo.

Last thing I can think of is an important safety tip. If plpython caches
the compiled function, you need to take care to invalidate it if the
return or argument types change from call to call.

What about moving contrib/intagg into backend ?
(And converting it into ANYagg on the way ;)

As I said earlier, all in good time ;-) One question about that though
-- how is intagg different from array_accum? (which is already
polymorphic and in the submitted patch)

Joe

Joe Conway kirjutas T, 08.04.2003 kell 19:55:

Hannu Krosing wrote:

SELECT ARRAY[1,2] < ARRAY[1,2,3]; # --> FALSE

Support for sorting and b-tree indexing could be nice too.

I thought briefly about this, but it wasn't immediately clear what the
semantics ought to be in all cases. I've also spent literally all my
available "hacking" time for the last several weeks just to get to the
patch submitted. I'd like to see at least some of it committed before I
take on anything new ;-)

If you want to propose in detail how these would behave - including:

I like the "compare like strings" approach

- different length arrays

shorter is smaller if all elements are same

- different dimension arrays

not comparable, just as array elemant is not comparable with array

- is comparison by ordinal position, or is each element compared to
all elements of the other side,

neither, pairs of elements at same positions are compared until first !=
or one array ends, then the shorter array the array with smaller element
value is considered smaller.

e.g. is
(ARRAY[1,2,3] < ARRAY[2,3,4]) TRUE or FALSE?
If you compare by ordinal position TRUE, but in the latter case
FALSE

all the following should also be TRUE

(ARRAY[1,2,3] < ARRAY[2,1,1])
(ARRAY[1,2,3] < ARRAY[1,2,3,4])
(ARRAY[] < ARRAY[1,2,3,4])
(ARRAY[1,2,3,4] < ARRAY[1,2,4,3])

Where should one start to add PL/Python support for polymorphic types ?

Not entirely sure. In plperl, pltcl, and plr there is a section of code
in the compile function that looks something like:

....

Thanks, I'll take a look.

What about moving contrib/intagg into backend ?
(And converting it into ANYagg on the way ;)

As I said earlier, all in good time ;-) One question about that though
-- how is intagg different from array_accum? (which is already
polymorphic and in the submitted patch)

Sorry, didn't notice array_accum.

intagg has functions for doing it both ways (array->table and table ->
array).

-----------------
Hannu

Joe Conway <mail@joeconway.com> writes:

Here's an updated patch that fixes the above mentioned issues. I also
added significantly more functionality.

I've applied much but not all of this. Some comments:

I didn't apply any of the aggregate changes, because I'm unsure of how
we actually want to do that. AFAICT you've set it up so that the
polymorphism of the transition function is hidden within the aggregate,
and one must still create a separate aggregate for each input datatype.
Couldn't we fix things so that a polymorphic transition function leads
to a polymorphic aggregate? I'm not totally sure that makes sense, but
it seems worth thinking about.

Also I didn't put in the bool_op stuff. That seemed pretty messy; in
particular I didn't care for looking at the operator names to decide
what to do. Another problem is that the actual lookup of the scalar
operators would be schema search path dependent. I'd feel more
comfortable with something that created a tighter binding of the array
operators to the underlying scalar operators. Not sure how to do it,
though.

There are a number of remaining annoyances in array type assignment and
coercion. Here's one:

create table t1 (p point[]);
insert into t1 values(array['(3,4)','(4,5)','(6,7)']);
ERROR: column "p" is of type point[] but expression is of type text[]
You will need to rewrite or cast the expression
Instead you have to write
insert into t1 values(array['(3,4)'::point,'(4,5)','(6,7)']);

I'm not sure if there's any way around this, but it certainly reduces the
usefulness of untyped literals. Right offhand it seems like maybe we could
invent an UNKNOWNARRAY type so as to postpone the decision about what the
array's type should be --- but I'm worried about possible side-effects.
I think there are places where the existence of "unknown[]" might allow
the type resolution logic to follow paths we don't want it to follow.

I think there are a lot of places where binary-compatible element types
and domain element types will not be treated reasonably. We need to think
about where and how to handle that.

Another problem is:

regression=# select distinct array(select * from text_tbl) from foo;
ERROR: Unable to identify an equality operator for type text[]

This DISTINCT query would fail anyway of course, for lack of a '<'
operator for arrays, but it seems like we ought to be able to find the
polymorphic '=' operator.

We really ought to reimplement array_eq to be less bogus: instead of bit
equality it ought to be applying the element type's equality operator.

I rearranged the way that parse_coerce and function/operator overloading
resolution handle arrays. It seems cleaner to me, but take a look and
see what you think.

One thing I didn't like about the implementation of many of these functions
is that they were willing to repeat catalog lookups on every call. I fixed
array_type_coerce to cache lookup results, please see if you can apply the
technique elsewhere.

The selectivity estimation functions you'd assigned to the bool_ops operators
seemed like pretty bogus choices. I am not sure we can use scalar estimators
for these at all --- perhaps a new set of estimators needs to be written.

create table foo(f1 integer ARRAY);
create table foo(f1 integer ARRAY[]);
create table foo(f1 integer ARRAY[x]);
create table foo(f1 integer ARRAY[][]); (and more [] etc)
create table foo(f1 integer ARRAY[x][y]); (and more [] etc)

AFAICT, SQL99 specifies the syntax "typename ARRAY [ intconst ]" and
nothing else. I do not see a reason to accept ARRAY without [],
ARRAY with empty [], ARRAY with multiple [], etc if the spec doesn't
make us do so. The existing Postgres syntax without the word ARRAY
gets the job done just fine.

regards, tom lane

Tom Lane wrote:

I've applied much but not all of this. Some comments:

Thanks for the review and cleanup!

I didn't apply any of the aggregate changes, because I'm unsure of how
we actually want to do that. AFAICT you've set it up so that the
polymorphism of the transition function is hidden within the aggregate,
and one must still create a separate aggregate for each input datatype.
Couldn't we fix things so that a polymorphic transition function leads
to a polymorphic aggregate? I'm not totally sure that makes sense, but
it seems worth thinking about.

I went back and forth on that issue myself more than once while I was
working on this. I'll take another look at making aggregate polymorphic
as well.

Also I didn't put in the bool_op stuff. That seemed pretty messy; in
particular I didn't care for looking at the operator names to decide
what to do. Another problem is that the actual lookup of the scalar
operators would be schema search path dependent. I'd feel more
comfortable with something that created a tighter binding of the array
operators to the underlying scalar operators. Not sure how to do it,
though.

But the lookup would be schema search path dependent if we were given
two scalars, so I don't see this as any different. Would it be better to
use the same operators as the scalars ("=", "<>", ...etc)? It makes
sense to me that "array = element" should apply the "=" operator for the
element data type, across all of the array elements. Maybe this takes us
back to Peter's suggestion:
expression IN (array)
expression NOT IN (array)
expression operator ANY (array)
expression operator SOME (array)
(expression) operator (array)
(expression) operator ALL (array)

In this case operator is taken as the appropriate operator for the
expression type as both left and right arguments, and array is treated
the same as a one column subquery.

There are a number of remaining annoyances in array type assignment and
coercion. Here's one:

create table t1 (p point[]);
insert into t1 values(array['(3,4)','(4,5)','(6,7)']);
ERROR: column "p" is of type point[] but expression is of type text[]
You will need to rewrite or cast the expression
Instead you have to write
insert into t1 values(array['(3,4)'::point,'(4,5)','(6,7)']);

I'm not sure if there's any way around this, but it certainly reduces the
usefulness of untyped literals. Right offhand it seems like maybe we could
invent an UNKNOWNARRAY type so as to postpone the decision about what the
array's type should be --- but I'm worried about possible side-effects.
I think there are places where the existence of "unknown[]" might allow
the type resolution logic to follow paths we don't want it to follow.

I think there are a lot of places where binary-compatible element types
and domain element types will not be treated reasonably. We need to think
about where and how to handle that.

Yeah, I think this is what led me to the hack in parse_coerce that you
didn't like ;-), but I'm not sure how to better handle it. I'll think a
bit more on it.

Another problem is:

regression=# select distinct array(select * from text_tbl) from foo;
ERROR: Unable to identify an equality operator for type text[]

This DISTINCT query would fail anyway of course, for lack of a '<'
operator for arrays, but it seems like we ought to be able to find the
polymorphic '=' operator.

We really ought to reimplement array_eq to be less bogus: instead of bit
equality it ought to be applying the element type's equality operator.

OK. I'll look at these issues again. Should I also look to implement:
array <> array
array > array
array < array
array >= array
array <= array

as Hannu suggested?

I rearranged the way that parse_coerce and function/operator overloading
resolution handle arrays. It seems cleaner to me, but take a look and
see what you think.

OK.

One thing I didn't like about the implementation of many of these functions
is that they were willing to repeat catalog lookups on every call. I fixed
array_type_coerce to cache lookup results, please see if you can apply the
technique elsewhere.

OK.

The selectivity estimation functions you'd assigned to the bool_ops operators
seemed like pretty bogus choices. I am not sure we can use scalar estimators
for these at all --- perhaps a new set of estimators needs to be written.

I figured you wouldn't like that, but I was at a loss as to how to
approach something better. I'll take a shot at writing new estimators
based on the scalar ones.

AFAICT, SQL99 specifies the syntax "typename ARRAY [ intconst ]" and
nothing else. I do not see a reason to accept ARRAY without [],
ARRAY with empty [], ARRAY with multiple [], etc if the spec doesn't
make us do so. The existing Postgres syntax without the word ARRAY
gets the job done just fine.

I had in my notes this (I think from SQL200x):

6.1 <data type>
<collection type> ::= <array type> | <multiset type>
<array type> ::=
<data type> ARRAY [ <left bracket or trigraph>
<unsigned integer>
<right bracket or trigraph> ]

So I thought
typename ARRAY [ intconst ]
typename ARRAY
were both per spec. As far as "typename ARRAY [ ]", ARRAY with multiple
[], etc goes, I only included them for consistency, but I don't care if
we leave them out either.

As always, a thorough review and a lot to think about! Thanks for the
help. It will be at least a couple days before I can pick this up again
(my development system lost a cpu fan yesterday, so I'm taking this as
an opportunity to upgrade ;-)), but I'll get on it as soon as I can.

Joe