Re: multiset patch review

Itagaki,

* Itagaki Takahiro (itagaki(dot)takahiro(at)gmail(dot)com) wrote:
> On Sat, Feb 12, 2011 at 05:01, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> > What does the spec say about this, if anything?  Is that required by
> > spec, or is the spec not relevant to this because MULTISETs are only one
> > dimensional..?
>
> Yes. The SQL standard has only one-dimensional ARRAYs and MULTISETs ,
> though it supports "collections of collections", that we don't have.

Yeah, I was afraid of that.. :(

> > In my view, we should be throwing an error if we get called on a
> > multi-dim array and we can't perform the operation on that in an
> > obviously correct way, not forcing the input to match something we can
> > make work.
>
> Agreed, I'll do so. We could also keep lower-bounds of arrays,
> at least on sort.

Sounds good. I'm also fine with providing a 'flatten' function, I just
don't agree w/ doing it automatically.

> > I'm not thrilled with called ArrayGetNItems array_cardinality().  Why
> > not just provide a function with a name like "array_getnitems()"
> > instead?
>
> We must use the name, because it is in the SQL standard.

If we use the name, then we have to throw an error when it's not a
single dimension array, since that's what's in the SQL standard. In
that case, we might as well have another function which gives us
ArrayGetNItems anyway.

> > trim_array() suffers from two problems: lack of comments, and no spell
> > checking done on those that are there.
>
> What comments do you want?

Uhm, how about ones that explain what's going on in each paragraph of
code..? And in other places, commenting the functions, what they do,
what they're used for, and documenting each of the arguments that are
passed in..

> > Should get_type_cache() really live in array_userfuncs.c ?
>
> Do you find codes using the same operation in other files?

Not yet, but logically it's about gathering information about types and
could be needed beyond just arrays..

> > There's more, primairly lack of comments and what I consider poor
> > function naming ("sort_or_unique()" ?  really?),
>
> Could you suggest better names?

How about 'array_sort()' or similar? With appropriate arguments that
can be used to request unique'ing or not? Or is there a "just unique
it, but don't sort it" option for this function?

Thanks,

Stephen