Re: review: More frame options in window functions

On Mon, Feb 8, 2010 at 10:37 PM, Hitoshi Harada <umi(dot)tanuki(at)gmail(dot)com> wrote:
> 2010/2/9 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
>> Hitoshi Harada <umi(dot)tanuki(at)gmail(dot)com> writes:
>>> 2010/1/23 Robert Haas <robertmhaas(at)gmail(dot)com>:
>>>> Would it make sense to pull some of the infrastructure bits out of
>>>> this patch and commit those bits separately, so as to reduce the size
>>>> of the main patch?  In particular, the AggGetMemoryContext() stuff
>>>> looks like a good candidate for that treatment.
>>
>>> Fair enough. Attached contains that part only.
>>
>> I started looking at this patch.  I believe that we should commit the
>> AggGetMemoryContext API function --- *not* the window context
>> management changes that you included here, but only the API abstraction
>> --- to be sure that that gets into 9.0 so that third-party aggregate
>> functions can start relying on it instead of looking directly at the
>> AggState or WindowAggState node.  The rest of the patch might or might
>> not make it, but we can at least help people future-proof their code.
>>
>> I'm fairly desperately unhappy with the RANGE PRECEDING/FOLLOWING parts
>> of the patch.  We have expended a great deal of sweat over the years
>> to avoid hard-wiring assumptions about particular operator names into
>> the code, but this patch is blithely ignoring that history and assuming
>> that "+" and "-" will do the right thing.  Also LookupOperName is
>> probably not the right thing, since it insists on exact or
>> binary-compatible match.  To the extent that there is any justification
>> at all for assuming that "+"/"-" are the right operators, it is that the
>> spec speaks in terms of the range bounds being VSK+V2F etc --- but if
>> someone were to actually write out such an expression, the parser would
>> allow for implicit casts to happen, so this code is not implementing
>> what that expression would produce.  Plus the results are dependent on
>> the current search path, which for example means it'll fail if the
>> window sort column is a user-defined type whose operators happen to be
>> out of path at the moment --- even though we would have found its
>> default sort opclass despite that.  And lastly, I'm totally unconvinced
>> that it's a good idea to accept an operator that returns a type other
>> than the type of the window sort column.  That seems to eliminate
>> whatever little protection we had against picking up an unsuitable
>> operator; and it complicates the code as well.
>
> I know "+"/"-" part is an issue. But as far as I know there's been no
> infrastructure to handle such situation. My ideal plan is to introduce
> some mechanism to make "+"/"-" operation abstract enough such like
> sort opclass/opfamily, although I wasn't sure if that is to be
> introduced for this (ie RANGE frame) purpose only.
>
> Now that specialized hard-coding "+"/"-" in source seems unacceptable,
> I would like to hear how to handle this. Is there any better than
> introducing new mechanism such like opclass?
>
>> Given the lack of time remaining in this CF, I'm tempted to propose
>> ripping out the RANGE support and just trying to get ROWS committed.
>> That should be substantially less controversial from a semantic
>> standpoint, and it still seems like a considerable improvement in
>> functionality.
>>
>> Thoughts?
>
> As expected. I don't mind splitting patch to be committable if users
> who expected this feature don't mind.

Well, they'll likely be happier with a partial feature than no feature
at all... I agree with Tom that there's no time time now to resolve
the issue of how + and - should be handled.

...Robert