Re: Add RESPECT/IGNORE NULLS and FROM FIRST/LAST options

>> The attached test patch is mostly the same as in the previous patch
>> set, but it doesn't fail on row_number anymore as the main patch
>> only rejects aggregate functions. The test patch also adds a test for
> 
>> +SELECT sum(orbit) RESPECT NULLS OVER () FROM planets; -- succeeds
> 
> I think the standard does not allow to specify RESPECT NULLS other
> than lead, lag, first_value, last_value and nth_value. Unless we agree
> that PostgreSQL violates the standard in this regard, you should not
> allow to use RESPECT NULLS for the window functions, expect lead etc.
> and aggregates.
> 
> See my patch.
> 
>> +/*
>> + * Window function option clauses
>> + */
>> +opt_null_treatment:
>> +            RESPECT NULLS_P                            { $$ = RESPECT_NULLS; }
>> +            | IGNORE_P NULLS_P                        { $$ = IGNORE_NULLS; }
>> +            | /*EMPTY*/                                { $$ = NULL_TREATMENT_NOT_SET; }
>> +        ;
> 
> With this, you can check if null treatment clause is used or not in
> each window function.
> 
> In my previous patch I did the check in parse/analysis but I think
> it's better to be checked in each window function. This way,
> 
> - need not to add a column to pg_proc.
> 
> - allow user defined window functions to decide by themselves whether
>   they can accept null treatment option.

Attached is the patch to implement this (on top of your patch).

test=# SELECT row_number() RESPECT NULLS OVER () FROM (SELECT 1) AS s;
ERROR:  window function row_number cannot have RESPECT NULLS or IGNORE NULLS

Best reagards,
--
Tatsuo Ishii
SRA OSS LLC
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp
diff --git a/src/backend/executor/nodeWindowAgg.c b/src/backend/executor/nodeWindowAgg.c
index fac0e05dee..b8519d9890 100644
--- a/src/backend/executor/nodeWindowAgg.c
+++ b/src/backend/executor/nodeWindowAgg.c
@@ -74,6 +74,7 @@ typedef struct WindowObjectData
     int64        *win_nonnulls;    /* tracks non-nulls in ignore nulls mode */
     int            nonnulls_size;    /* track size of the win_nonnulls array */
     int            nonnulls_len;    /* track length of the win_nonnulls array */
+    WindowFunc    *wfunc;            /* WindowFunc of this function */
 } WindowObjectData;
 
 /*
@@ -2634,6 +2635,8 @@ ExecInitWindowAgg(WindowAgg *node, EState *estate, int eflags)
                 winobj->nonnulls_len = 0;
             }
 
+            winobj->wfunc = wfunc;
+
             /* It's a real window function, so set up to call it. */
             fmgr_info_cxt(wfunc->winfnoid, &perfuncstate->flinfo,
                           econtext->ecxt_per_query_memory);
@@ -3881,3 +3884,24 @@ WinGetFuncArgCurrent(WindowObject winobj, int argno, bool *isnull)
     return ExecEvalExpr((ExprState *) list_nth(winobj->argstates, argno),
                         econtext, isnull);
 }
+
+/*
+ * Error out that this window function cannot have null treatement.
+ */
+void
+ErrorOutNullTreatment(WindowObject winobj)
+{
+    char            *fname;
+
+    Assert(WindowObjectIsValid(winobj));
+
+    if (winobj->wfunc->null_treatment == NULL_TREATMENT_NOT_SET)
+        return;
+
+    fname = get_func_name(winobj->wfunc->winfnoid);
+
+    ereport(ERROR,
+            (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+             errmsg("window function %s cannot have RESPECT NULLS or IGNORE NULLS",
+                    fname)));
+}
diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c
index 01fd16acf9..05e64c4569 100644
--- a/src/backend/optimizer/util/clauses.c
+++ b/src/backend/optimizer/util/clauses.c
@@ -2475,6 +2475,7 @@ eval_const_expressions_mutator(Node *node,
                 newexpr->winstar = expr->winstar;
                 newexpr->winagg = expr->winagg;
                 newexpr->ignore_nulls = expr->ignore_nulls;
+                newexpr->null_treatment = expr->null_treatment;
                 newexpr->location = expr->location;
 
                 return (Node *) newexpr;
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 58c00bfd4f..e131428e85 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -276,6 +276,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
     MergeWhenClause *mergewhen;
     struct KeyActions *keyactions;
     struct KeyAction *keyaction;
+    NullTreatment    nulltreatment;
 }
 
 %type <node>    stmt toplevel_stmt schema_stmt routine_body_stmt
@@ -633,7 +634,6 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
                 opt_frame_clause frame_extent frame_bound
 %type <ival>    opt_window_exclusion_clause
 %type <str>        opt_existing_window_name
-%type <boolean> null_treatment
 %type <boolean> opt_if_not_exists
 %type <boolean> opt_unique_null_treatment
 %type <ival>    generated_when override_kind
@@ -662,6 +662,8 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
                     json_object_constructor_null_clause_opt
                     json_array_constructor_null_clause_opt
 
+%type <nulltreatment>        null_treatment
+
 /*
  * Non-keyword token types.  These are hard-wired into the "flex" lexer.
  * They must be listed first so that their numeric codes do not depend on
@@ -15247,7 +15249,7 @@ func_expr: func_application within_group_clause filter_clause null_treatment ove
                         n->agg_within_group = true;
                     }
                     n->agg_filter = $3;
-                    n->ignore_nulls = $4;
+                    n->null_treatment = $4;
                     n->over = $5;
                     $$ = (Node *) n;
                 }
@@ -15797,9 +15799,9 @@ filter_clause:
  * Window Definitions
  */
 null_treatment:
-            IGNORE_P NULLS_P                        { $$ = true; }
-            | RESPECT_P NULLS_P                        { $$ = false; }
-            | /*EMPTY*/                                { $$ = false; }
+            RESPECT_P NULLS_P                        { $$ = RESPECT_NULLS; }
+            | IGNORE_P NULLS_P                        { $$ = IGNORE_NULLS; }
+            | /*EMPTY*/                                { $$ = NULL_TREATMENT_NOT_SET; }
         ;
 
 window_clause:
diff --git a/src/backend/parser/parse_func.c b/src/backend/parser/parse_func.c
index afa4bcc8d1..63af8ca6aa 100644
--- a/src/backend/parser/parse_func.c
+++ b/src/backend/parser/parse_func.c
@@ -98,7 +98,7 @@ ParseFuncOrColumn(ParseState *pstate, List *funcname, List *fargs,
     bool        agg_star = (fn ? fn->agg_star : false);
     bool        agg_distinct = (fn ? fn->agg_distinct : false);
     bool        func_variadic = (fn ? fn->func_variadic : false);
-    bool        ignore_nulls = (fn ? fn->ignore_nulls : false);
+    NullTreatment null_treatment = (fn ? fn->null_treatment : NULL_TREATMENT_NOT_SET);
     CoercionForm funcformat = (fn ? fn->funcformat : COERCE_EXPLICIT_CALL);
     bool        could_be_projection;
     Oid            rettype;
@@ -516,11 +516,12 @@ ParseFuncOrColumn(ParseState *pstate, List *funcname, List *fargs,
                                 NameListToString(funcname)),
                          parser_errposition(pstate, location)));
 
-            /* It also can't treat nulls as a window function */
-            if (ignore_nulls)
+            /* Aggregate functions cannot have null treatment clause */
+            if (null_treatment != NULL_TREATMENT_NOT_SET)
                 ereport(ERROR,
                         (errcode(ERRCODE_WRONG_OBJECT_TYPE),
-                         errmsg("aggregate functions do not accept RESPECT/IGNORE NULLS"),
+                         errmsg("aggregate function %s cannot have RESPECT NULLS or IGNORE NULLS",
+                                NameListToString(funcname)),
                          parser_errposition(pstate, location)));
         }
     }
@@ -842,7 +843,8 @@ ParseFuncOrColumn(ParseState *pstate, List *funcname, List *fargs,
         wfunc->winstar = agg_star;
         wfunc->winagg = (fdresult == FUNCDETAIL_AGGREGATE);
         wfunc->aggfilter = agg_filter;
-        wfunc->ignore_nulls = ignore_nulls;
+        wfunc->null_treatment = null_treatment;
+        wfunc->ignore_nulls = (null_treatment == IGNORE_NULLS);
         wfunc->location = location;
 
         /*
diff --git a/src/backend/utils/adt/windowfuncs.c b/src/backend/utils/adt/windowfuncs.c
index b87a624fb2..297e787927 100644
--- a/src/backend/utils/adt/windowfuncs.c
+++ b/src/backend/utils/adt/windowfuncs.c
@@ -85,6 +85,9 @@ window_row_number(PG_FUNCTION_ARGS)
     WindowObject winobj = PG_WINDOW_OBJECT();
     int64        curpos = WinGetCurrentPosition(winobj);
 
+    /* row_number() does not support null treatment */
+    ErrorOutNullTreatment(winobj);
+
     WinSetMarkPosition(winobj, curpos);
     PG_RETURN_INT64(curpos + 1);
 }
@@ -140,6 +143,9 @@ window_rank(PG_FUNCTION_ARGS)
     rank_context *context;
     bool        up;
 
+    /* rank() does not support null treatment */
+    ErrorOutNullTreatment(winobj);
+
     up = rank_up(winobj);
     context = (rank_context *)
         WinGetPartitionLocalMemory(winobj, sizeof(rank_context));
@@ -202,6 +208,9 @@ window_dense_rank(PG_FUNCTION_ARGS)
     rank_context *context;
     bool        up;
 
+    /* dense_rank() does not support null treatment */
+    ErrorOutNullTreatment(winobj);
+
     up = rank_up(winobj);
     context = (rank_context *)
         WinGetPartitionLocalMemory(winobj, sizeof(rank_context));
@@ -266,6 +275,9 @@ window_percent_rank(PG_FUNCTION_ARGS)
 
     Assert(totalrows > 0);
 
+    /* percent_rank() does not support null treatment */
+    ErrorOutNullTreatment(winobj);
+
     up = rank_up(winobj);
     context = (rank_context *)
         WinGetPartitionLocalMemory(winobj, sizeof(rank_context));
@@ -335,6 +347,9 @@ window_cume_dist(PG_FUNCTION_ARGS)
 
     Assert(totalrows > 0);
 
+    /* cume_dist() does not support null treatment */
+    ErrorOutNullTreatment(winobj);
+
     up = rank_up(winobj);
     context = (rank_context *)
         WinGetPartitionLocalMemory(winobj, sizeof(rank_context));
@@ -412,6 +427,9 @@ window_ntile(PG_FUNCTION_ARGS)
     WindowObject winobj = PG_WINDOW_OBJECT();
     ntile_context *context;
 
+    /* ntile() does not support null treatment */
+    ErrorOutNullTreatment(winobj);
+
     context = (ntile_context *)
         WinGetPartitionLocalMemory(winobj, sizeof(ntile_context));
 
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index 073e2469ba..32fbab46a0 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -432,6 +432,7 @@ typedef struct FuncCall
     bool        agg_distinct;    /* arguments were labeled DISTINCT */
     bool        func_variadic;    /* last argument was labeled VARIADIC */
     CoercionForm funcformat;    /* how to display this node */
+    NullTreatment null_treatment;    /* RESPECT_NULLS or IGNORE NULLS */
     int            location;        /* token location, or -1 if unknown */
 } FuncCall;
 
diff --git a/src/include/nodes/primnodes.h b/src/include/nodes/primnodes.h
index 221b5e6218..545b5e5ac8 100644
--- a/src/include/nodes/primnodes.h
+++ b/src/include/nodes/primnodes.h
@@ -532,6 +532,13 @@ typedef struct GroupingFunc
     int            location;
 } GroupingFunc;
 
+typedef enum NullTreatment
+{
+    NULL_TREATMENT_NOT_SET = 0,
+    RESPECT_NULLS,
+    IGNORE_NULLS
+} NullTreatment;
+
 /*
  * WindowFunc
  *
@@ -559,7 +566,8 @@ typedef struct WindowFunc
     bool        winstar pg_node_attr(query_jumble_ignore);
     /* is function a simple aggregate? */
     bool        winagg pg_node_attr(query_jumble_ignore);
-    /* ignore nulls */
+    /* null treatement */
+    NullTreatment null_treatment pg_node_attr(query_jumble_ignore);
     bool        ignore_nulls;
     /* token location, or -1 if unknown */
     int            location;
diff --git a/src/include/windowapi.h b/src/include/windowapi.h
index b8c2c565d1..8a50478ee9 100644
--- a/src/include/windowapi.h
+++ b/src/include/windowapi.h
@@ -61,4 +61,6 @@ extern Datum WinGetFuncArgInFrame(WindowObject winobj, int argno,
 extern Datum WinGetFuncArgCurrent(WindowObject winobj, int argno,
                                   bool *isnull);
 
+extern void    ErrorOutNullTreatment(WindowObject winobj);
+
 #endif                            /* WINDOWAPI_H */