Thread

1. review: More frame options in window functions

   Pavel Stehule <pavel.stehule@gmail.com> — 2010-01-14T10:06:26Z

   Hello
   
   I looked on Hitoshi's patch - and my result is:
   
   1. Patch is in correct format, it is cleanly applied and compilation
   is without warnings,
   2. Patch contains adequate changes of doc, and rich set of regress tests,
   3. If I could to understand to implemented feature - it is in
   conformity with SQL standard,
   4. This feature increase feature set in analytic area:
        a)  we can do simply some operation over time series like moving average,
        b) it decrease difference between Oracle and DB2 in this area (so
   port of some application should be simpler)
   
   5. Last version of this patch work without known crashes
   6. make check doesn't signalise any problem
   
   7. Source code respects our coding guidelines - it is well commented
   8. Contrib is compiled without warnings
   9. All contrib regress tests passed.
   
   Bonus:
   It simplify and unifies access to agg or window context via func:
   AggGetMemoryContext. It protect us against a future changes.
   
   My  recommendation
   * add some examples to documentation - mainly some use case of RANGE frame
   
   Possible issue
   * It could to break compatibility for some custom aggregates in C.
   This topic was discussed and this way is preferred. I thing, so this
   issue  have to be documented somewhere:
   
   "if you use aggcontext in your custom aggregates, fix your code"
   
   if (fcinfo->context && IsA(fcinfo->context, AggState))
            aggcontext = ((AggState *) fcinfo->context)->aggcontext;
   else if (fcinfo->context && IsA(fcinfo->context, WindowAggState))
            aggcontext = ((WindowAggState *) fcinfo->context)->wincontext;
   else
   
   have to be replaced with line:
         aggcontext = AggGetMemoryContext((Node *) fcinfo->context, &iswindowagg);
   
   Regards
   Pavel Stehule
   
   

2. Re: review: More frame options in window functions

   Hitoshi Harada <umi.tanuki@gmail.com> — 2010-01-14T18:32:32Z

   2010/1/14 Pavel Stehule <pavel.stehule@gmail.com>:
   > Hello
   >
   > I looked on Hitoshi's patch - and my result is:
   
   Thanks for the review. I've found another crash today and attached is
   fixed version. The case is:
   
   SELECT four, sum(ten) over (PARTITION BY four ORDER BY four RANGE 1
   PRECEDING) FROM tenk1 WHERE unique1 < 10;
   
   The planner recognizes windowagg->ordNumCol as 0 for optimization, but
   RANGE offset cases should really have interest on that value as the
   syntax says.
   
   Regards,
   
   
   -- 
   Hitoshi Harada