Re: multiset patch review

* Itagaki Takahiro (itagaki(dot)takahiro(at)gmail(dot)com) wrote:
> I will remove parser changes from the patch; it will add only a few array
> functions. Then, please let me know functions you don't want to include
> in the core, if any. I'll remove them at the same time.

Seems like this should be 'waiting on author', but since it was marked
'needs review', I started taking a look at it. Without the grammar
changes, it's just adding a couple of functions. In general, I'm all
for that, but I don't like this:

Input arrays are always flattened into one-dimensional arrays.

That just strikes me as completely broken when it comes to PG Arrays.
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..?

I would think that we would have a way to define what dimension or piece
of the array that would be sorted or returned as a set, etc. I could
see having a 'flatten' function which could be called first, if that's
really what you want.. Or maybe we just need a slice function whose
result could then be passed in to these functions, perhaps..

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.

From above, that makes me not thrilled with the 'flatten' boolean for
array_cat_internal(), nor with how it was implemented, or how those
changes apparently didn't justfiy *any* comment updates..

I'm not thrilled with called ArrayGetNItems array_cardinality(). Why
not just provide a function with a name like "array_getnitems()"
instead?

trim_array() suffers from two problems: lack of comments, and no spell
checking done on those that are there.

Should get_type_cache() really live in array_userfuncs.c ?

There's more, primairly lack of comments and what I consider poor
function naming ("sort_or_unique()" ? really?), but in the end my
feeling is that this could survive just fine, for now, as a contrib
module, and that it really isn't close enough to being committable to
make it into 9.1 in any case.

I'll mark it waiting on author, since I think the formal 'returned with
feedback' needs to come from someone else, but that's where I think this
is headed.

Thanks,

Stephen