* Itagaki Takahiro (itagaki.takah...@gmail.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
signature.asc
Description: Digital signature