Re: Window functions review

Thank you for your reviewing my code.

2008/11/12 Heikki Linnakangas <heikki.linnakangas@enterprisedb.com>:
> I've been slicing and dicing this patch for the last few days. There's a lot
> of code in there, but here's some initial comments:
>
> The code to initialize, advance, and finalize an aggregate should be shared
> between Agg and Window nodes.
>
> I'm a bit disappointed that we need so much code to support the window
> aggregates. I figured we'd just have a special window function that calls
> the initialize/advance/finalize functions for the right aggregate, working
> with the WindowObject object like other window functions. But we have in
> fact very separate code path for aggregates. I feel we should implement the
> aggregates as just a thin wrapper window function that I just described. Or,
> perhaps the aggregate functionality should be moved to Agg node, although if
> we do that, we'd probably have to change that again when we implement the
> window frames. Or perhaps it should be completely new node type, though
> you'd really want to share the tuplestore with the Window node if there's
> any window functions in the same query, using the same window specification.

I am disappointed at that, too. I was thinking as you do, that special
thin wrapper would do for pure aggregates. It seems, however,
impossible to me. Aggregates have two separate functions then
initialize value, store intermediate results, pass if each function is
strict, and so on. My rough sketch of wrapper for pure aggregates was
eval_windowaggregate() but actually they need to initialize, advance,
and finalize. As long as we have the same structure for aggregates,
they're always needed.
So the next choice is to share those three with Agg. This sounds sane.
I've not touched any code in nodeAgg.c. If we really need it I will do
it. Then we can remove initialize_windowaggregate(),
advance_windowaggregate(), finalize_windowaggregate(), and
initialize_peragg() from nodeWindow.c. They are almost same
correspoinding functions in Agg, except advance requires window
function API in Window. And if we do that, PerAgg and PerGroup should
be shared also.
The reason I didn't touch nodeAgg.c and didn't share them is that
there are only two nodes that use the share code. I wasn't sure if it
is general enough. Or I thought it'd be better to seperate code than
to share it so we keep them simple. What do you think?

> I don't like that the window functions are passed Var and Const nodes as
> arguments. A user-defined function shouldn't see those internal data
> structures, even though they're just passed as opaque arguments to
> WinRowGetArg. You could pass WinRowGetArg just argument numbers, and have it
> fetch the Expr nodes.

I understand what you point. The current signature of WinRowGetArg is

Datum WinRowGetArg(WindowObject winobj, ExprState *argstate, bool *isnull);

And if we use a number as arguments instead of argstate, we need
fcinfo. How do you think we should pass it? WindowObject may hold it
but it is data related with the window logically, not with each
function. The alternative is to add one more argument to
WinRowGetArg(). I don't like long argument list. But if we really need
it, no way to deny it.

> The incomplete support for window frames should be removed. It was good that
> you did it, so that we have a sketch of how it'd work and got some
> confidence that we won't have to rip out and rewrite all of this in the
> future to support the window frames. But if it's not going to go into this
> release, we should take it out.

Agreed. I'll remove it. Until recently I was wondering if I could try
to implement the FRAME clause. But actually it's too late for 8.4. Or
it would have to be after the current patch is commited.

> The buffering strategies are very coarse. For aggregates, as long as we
> don't support window frames, row buffering is enough. Even when
> partition-buffering is needed, we shouldn't need to fetch the whole
> partition into the tuplestore before we start processing. lead/lag for
> example can start returning values earlier. That's just an optimization,
> though, so maybe we can live with that for now.

Aggregates() RANGE BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW doesn't
require frame buffering but RANGE BETEWEEN UNBOUNDED PRECEDING AND
UNBOUNDED FOLLOWING (that is OVER() ) needs it. You're telling me to
switch the strategy depending on cases? Hmmm, let me see.



-- 
Hitoshi Harada