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

I revisited the thread:
https://www.postgresql.org/message-id/flat/CAGMVOdsbtRwE_4%2Bv8zjH1d9xfovDeQAGLkP_B6k69_VoFEgX-A%40mail.gmail.com

and came up with attached POC patch (I used some varibale names
appearing in the Krasiyan Andreev's patch). I really love to have
RESPECT/IGNORE NULLS because I believe they are convenient for
users. For FIRST/LAST I am not so excited since there are alternatives
as our document stats, so FIRST/LAST are not included in the patch.

Currently in the patch only nth_value is allowed to use RESPECT/IGNORE
NULLS. I think it's not hard to implement it for others (lead, lag,
first_value and last_value).  No document nor test patches are
included for now.

Note that RESPECT/IGNORE are not registered as reserved keywords in
this patch (but registered as unreserved keywords). I am not sure if
this is acceptable or not.

> The questions of how we interface to the individual window functions
> are really independent of how we handle the parsing problem.  My
> first inclination is to just pass the flags down to the window functions
> (store them in WindowObject and provide some additional inquiry functions
> in windowapi.h) and let them deal with it.

I agree with this.  Also I do not change the prototype of
nth_value. So I pass RESPECT/IGNORE NULLS information from the raw
parser to parse/analysis and finally to WindowObject.

> It's also worth wondering if we couldn't just implement the flags in
> some generic fashion and not need to involve the window functions at
> all.  FROM LAST, for example, could and perhaps should be implemented
> by inverting the sort order.  Possibly IGNORE NULLS could be implemented
> inside the WinGetFuncArgXXX functions?  These behaviors might or might
> not make much sense with other window functions, but that doesn't seem
> like it's our problem.

Yes, probably we could make WinGetFuncArgXXX a little bit smarter in
this direction (not implemented in the patch at this point).

Best reagards,
--
Tatsuo Ishii
SRA OSS LLC
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp
From 07f01f8859e159c908ada72e8f53daf51e0b8bdf Mon Sep 17 00:00:00 2001
From: Tatsuo Ishii <ishii@postgresql.org>
Date: Sat, 22 Apr 2023 16:52:50 +0900
Subject: [PATCH v1 1/3] Implement IGNORE or RESPECT NULLS parse/analysis part.

Implement SQL standard's IGNORE/RESPECT NULLS clause for window functions.
For now, only nth_value() can use this option.
---
 src/backend/parser/gram.y       | 22 ++++++++++++++++++----
 src/backend/parser/parse_func.c | 13 +++++++++++++
 src/include/nodes/parsenodes.h  |  8 ++++++++
 src/include/nodes/primnodes.h   |  2 ++
 src/include/parser/kwlist.h     |  2 ++
 5 files changed, 43 insertions(+), 4 deletions(-)

diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index acf6cf4866..2980ecd666 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
@@ -661,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>        opt_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
@@ -718,7 +721,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
 
     HANDLER HAVING HEADER_P HOLD HOUR_P
 
-    IDENTITY_P IF_P ILIKE IMMEDIATE IMMUTABLE IMPLICIT_P IMPORT_P IN_P INCLUDE
+    IDENTITY_P IF_P IGNORE_P ILIKE IMMEDIATE IMMUTABLE IMPLICIT_P IMPORT_P IN_P INCLUDE
     INCLUDING INCREMENT INDENT INDEX INDEXES INHERIT INHERITS INITIALLY INLINE_P
     INNER_P INOUT INPUT_P INSENSITIVE INSERT INSTEAD INT_P INTEGER
     INTERSECT INTERVAL INTO INVOKER IS ISNULL ISOLATION
@@ -752,7 +755,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
 
     RANGE READ REAL REASSIGN RECHECK RECURSIVE REF_P REFERENCES REFERENCING
     REFRESH REINDEX RELATIVE_P RELEASE RENAME REPEATABLE REPLACE REPLICA
-    RESET RESTART RESTRICT RETURN RETURNING RETURNS REVOKE RIGHT ROLE ROLLBACK ROLLUP
+    RESET RESPECT RESTART RESTRICT RETURN RETURNING RETURNS REVOKE RIGHT ROLE ROLLBACK ROLLUP
     ROUTINE ROUTINES ROW ROWS RULE
 
     SAVEPOINT SCALAR SCHEMA SCHEMAS SCROLL SEARCH SECOND_P SECURITY SELECT
@@ -15213,7 +15216,7 @@ func_application: func_name '(' ')'
  * (Note that many of the special SQL functions wouldn't actually make any
  * sense as functional index entries, but we ignore that consideration here.)
  */
-func_expr: func_application within_group_clause filter_clause over_clause
+func_expr: func_application within_group_clause filter_clause opt_null_treatment over_clause
                 {
                     FuncCall   *n = (FuncCall *) $1;
 
@@ -15246,7 +15249,8 @@ func_expr: func_application within_group_clause filter_clause over_clause
                         n->agg_within_group = true;
                     }
                     n->agg_filter = $3;
-                    n->over = $4;
+                    n->null_treatment = $4;
+                    n->over = $5;
                     $$ = (Node *) n;
                 }
             | json_aggregate_func filter_clause over_clause
@@ -15790,6 +15794,14 @@ filter_clause:
             | /*EMPTY*/                                { $$ = NULL; }
         ;
 
+/*
+ * Window function option clauses
+ */
+opt_null_treatment:
+            RESPECT NULLS_P                            { $$ = RESPECT_NULLS; }
+            | IGNORE_P NULLS_P                        { $$ = IGNORE_NULLS; }
+            | /*EMPTY*/                                { $$ = NULL_TREATMENT_NOT_SET; }
+        ;
 
 /*
  * Window Definitions
@@ -17111,6 +17123,7 @@ unreserved_keyword:
             | HOUR_P
             | IDENTITY_P
             | IF_P
+            | IGNORE_P
             | IMMEDIATE
             | IMMUTABLE
             | IMPLICIT_P
@@ -17223,6 +17236,7 @@ unreserved_keyword:
             | REPLACE
             | REPLICA
             | RESET
+            | RESPECT
             | RESTART
             | RESTRICT
             | RETURN
diff --git a/src/backend/parser/parse_func.c b/src/backend/parser/parse_func.c
index b3f0b6a137..92af0d10f1 100644
--- a/src/backend/parser/parse_func.c
+++ b/src/backend/parser/parse_func.c
@@ -31,6 +31,7 @@
 #include "parser/parse_target.h"
 #include "parser/parse_type.h"
 #include "utils/builtins.h"
+#include "utils/fmgroids.h"
 #include "utils/lsyscache.h"
 #include "utils/syscache.h"
 
@@ -99,6 +100,7 @@ ParseFuncOrColumn(ParseState *pstate, List *funcname, List *fargs,
     bool        agg_distinct = (fn ? fn->agg_distinct : false);
     bool        func_variadic = (fn ? fn->func_variadic : false);
     CoercionForm funcformat = (fn ? fn->funcformat : COERCE_EXPLICIT_CALL);
+    NullTreatment null_treatment = (fn ? fn->null_treatment : NULL_TREATMENT_NOT_SET);
     bool        could_be_projection;
     Oid            rettype;
     Oid            funcid;
@@ -534,6 +536,13 @@ ParseFuncOrColumn(ParseState *pstate, List *funcname, List *fargs,
                      errmsg("window function %s cannot have WITHIN GROUP",
                             NameListToString(funcname)),
                      parser_errposition(pstate, location)));
+        /* Check RESPECT NULLS or IGNORE NULLS is specified. They are only allowed with nth_value */
+        if (null_treatment != NULL_TREATMENT_NOT_SET && funcid != F_NTH_VALUE)
+            ereport(ERROR,
+                    (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+                     errmsg("window function %s cannot have RESPECT NULLS or IGNORE NULLS",
+                            NameListToString(funcname)),
+                     parser_errposition(pstate, location)));
     }
     else if (fdresult == FUNCDETAIL_COERCION)
     {
@@ -835,6 +844,10 @@ ParseFuncOrColumn(ParseState *pstate, List *funcname, List *fargs,
         wfunc->winagg = (fdresult == FUNCDETAIL_AGGREGATE);
         wfunc->aggfilter = agg_filter;
         wfunc->location = location;
+        if (null_treatment == IGNORE_NULLS)
+            wfunc->ignorenulls = true;
+        else
+            wfunc->ignorenulls = false;
 
         /*
          * agg_star is allowed for aggregate functions but distinct isn't
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index cc7b32b279..f13ae26a24 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -404,6 +404,13 @@ typedef struct RoleSpec
     int            location;        /* token location, or -1 if unknown */
 } RoleSpec;
 
+typedef enum NullTreatment
+{
+    NULL_TREATMENT_NOT_SET = 0,
+    RESPECT_NULLS,
+    IGNORE_NULLS
+} NullTreatment;
+
 /*
  * FuncCall - a function or aggregate invocation
  *
@@ -431,6 +438,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 be9c29f0bf..213297dbd3 100644
--- a/src/include/nodes/primnodes.h
+++ b/src/include/nodes/primnodes.h
@@ -559,6 +559,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);
+    /* true if IGNORE NULLS, false if RESPECT NULLS */
+    bool        ignorenulls pg_node_attr(query_jumble_ignore);
     /* token location, or -1 if unknown */
     int            location;
 } WindowFunc;
diff --git a/src/include/parser/kwlist.h b/src/include/parser/kwlist.h
index f5b2e61ca5..c7e61a8f0e 100644
--- a/src/include/parser/kwlist.h
+++ b/src/include/parser/kwlist.h
@@ -198,6 +198,7 @@ PG_KEYWORD("hold", HOLD, UNRESERVED_KEYWORD, BARE_LABEL)
 PG_KEYWORD("hour", HOUR_P, UNRESERVED_KEYWORD, AS_LABEL)
 PG_KEYWORD("identity", IDENTITY_P, UNRESERVED_KEYWORD, BARE_LABEL)
 PG_KEYWORD("if", IF_P, UNRESERVED_KEYWORD, BARE_LABEL)
+PG_KEYWORD("ignore", IGNORE_P, UNRESERVED_KEYWORD, AS_LABEL)
 PG_KEYWORD("ilike", ILIKE, TYPE_FUNC_NAME_KEYWORD, BARE_LABEL)
 PG_KEYWORD("immediate", IMMEDIATE, UNRESERVED_KEYWORD, BARE_LABEL)
 PG_KEYWORD("immutable", IMMUTABLE, UNRESERVED_KEYWORD, BARE_LABEL)
@@ -360,6 +361,7 @@ PG_KEYWORD("repeatable", REPEATABLE, UNRESERVED_KEYWORD, BARE_LABEL)
 PG_KEYWORD("replace", REPLACE, UNRESERVED_KEYWORD, BARE_LABEL)
 PG_KEYWORD("replica", REPLICA, UNRESERVED_KEYWORD, BARE_LABEL)
 PG_KEYWORD("reset", RESET, UNRESERVED_KEYWORD, BARE_LABEL)
+PG_KEYWORD("respect", RESPECT, UNRESERVED_KEYWORD, AS_LABEL)
 PG_KEYWORD("restart", RESTART, UNRESERVED_KEYWORD, BARE_LABEL)
 PG_KEYWORD("restrict", RESTRICT, UNRESERVED_KEYWORD, BARE_LABEL)
 PG_KEYWORD("return", RETURN, UNRESERVED_KEYWORD, BARE_LABEL)
-- 
2.25.1

From 3dc6f4bb897f76247589db018716bf5680d5331c Mon Sep 17 00:00:00 2001
From: Tatsuo Ishii <ishii@postgresql.org>
Date: Sat, 22 Apr 2023 16:58:48 +0900
Subject: [PATCH v1 2/3] Implement IGNORE or RESPECT NULLS planner part.

---
 src/backend/optimizer/util/clauses.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c
index a9c7bc342e..40fc62c447 100644
--- a/src/backend/optimizer/util/clauses.c
+++ b/src/backend/optimizer/util/clauses.c
@@ -2474,6 +2474,7 @@ eval_const_expressions_mutator(Node *node,
                 newexpr->winref = expr->winref;
                 newexpr->winstar = expr->winstar;
                 newexpr->winagg = expr->winagg;
+                newexpr->ignorenulls = expr->ignorenulls;
                 newexpr->location = expr->location;
 
                 return (Node *) newexpr;
-- 
2.25.1

From a78feec9bf7b08c644c2b3089b2de9237d4fcd9e Mon Sep 17 00:00:00 2001
From: Tatsuo Ishii <ishii@postgresql.org>
Date: Sat, 22 Apr 2023 17:00:06 +0900
Subject: [PATCH v1 3/3] Implement IGNORE or RESPECT NULLS executor and window
 functions part.

---
 src/backend/executor/nodeWindowAgg.c | 11 ++++++++++
 src/backend/utils/adt/windowfuncs.c  | 30 +++++++++++++++++++++++++---
 src/include/windowapi.h              |  2 ++
 3 files changed, 40 insertions(+), 3 deletions(-)

diff --git a/src/backend/executor/nodeWindowAgg.c b/src/backend/executor/nodeWindowAgg.c
index 3ac581a711..7e2affb12c 100644
--- a/src/backend/executor/nodeWindowAgg.c
+++ b/src/backend/executor/nodeWindowAgg.c
@@ -69,6 +69,7 @@ typedef struct WindowObjectData
     int            readptr;        /* tuplestore read pointer for this fn */
     int64        markpos;        /* row that markptr is positioned on */
     int64        seekpos;        /* row that readptr is positioned on */
+    WindowFunc    *wfunc;            /* WindowFunc of this function */
 } WindowObjectData;
 
 /*
@@ -2617,6 +2618,7 @@ ExecInitWindowAgg(WindowAgg *node, EState *estate, int eflags)
             winobj->winstate = winstate;
             winobj->argstates = wfuncstate->args;
             winobj->localmem = NULL;
+            winobj->wfunc = wfunc;
             perfuncstate->winobj = winobj;
 
             /* It's a real window function, so set up to call it. */
@@ -3620,3 +3622,12 @@ WinGetFuncArgCurrent(WindowObject winobj, int argno, bool *isnull)
     return ExecEvalExpr((ExprState *) list_nth(winobj->argstates, argno),
                         econtext, isnull);
 }
+
+/*
+ * Return current WindowFunc
+ */
+WindowFunc    *
+WinGetWindowFunc(WindowObject winobj)
+{
+    return winobj->wfunc;
+}
diff --git a/src/backend/utils/adt/windowfuncs.c b/src/backend/utils/adt/windowfuncs.c
index b87a624fb2..919295ba13 100644
--- a/src/backend/utils/adt/windowfuncs.c
+++ b/src/backend/utils/adt/windowfuncs.c
@@ -693,6 +693,7 @@ window_nth_value(PG_FUNCTION_ARGS)
     bool        const_offset;
     Datum        result;
     bool        isnull;
+    bool        isout;
     int32        nth;
 
     nth = DatumGetInt32(WinGetFuncArgCurrent(winobj, 1, &isnull));
@@ -705,9 +706,32 @@ window_nth_value(PG_FUNCTION_ARGS)
                 (errcode(ERRCODE_INVALID_ARGUMENT_FOR_NTH_VALUE),
                  errmsg("argument of nth_value must be greater than zero")));
 
-    result = WinGetFuncArgInFrame(winobj, 0,
-                                  nth - 1, WINDOW_SEEK_HEAD, const_offset,
-                                  &isnull, NULL);
+    if (WinGetWindowFunc(winobj)->ignorenulls)
+    {
+        int        i, n;
+
+        i = n = 0;
+
+        for (;;)
+        {
+            result = WinGetFuncArgInFrame(winobj, 0,
+                                          i++, WINDOW_SEEK_HEAD, false,
+                                          &isnull, &isout);
+            if (isout)
+                break;
+
+            if (!isnull)
+            {
+                if (n == nth - 1)
+                    break;
+                n++;
+            }
+        }
+    }
+    else
+        result = WinGetFuncArgInFrame(winobj, 0,
+                                      nth - 1, WINDOW_SEEK_HEAD, const_offset,
+                                      &isnull, NULL);
     if (isnull)
         PG_RETURN_NULL();
 
diff --git a/src/include/windowapi.h b/src/include/windowapi.h
index b8c2c565d1..64f7d4c84d 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 WindowFunc    *WinGetWindowFunc(WindowObject winobj);
+
 #endif                            /* WINDOWAPI_H */
-- 
2.25.1