> Overall LGTM. Just a few small comments:
> 1 - 0001
> ```
> --- a/src/backend/parser/parse_func.c
> +++ b/src/backend/parser/parse_func.c
> @@ -98,6 +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);
> + int ignore_nulls = (fn ? fn->ignore_nulls : 0);
> ```
>
> Should we use the constant NO_NULLTREATMENT here for 0?
Good suggestion. Will fix.
> 2 - 0001
> ```
> --- a/src/include/nodes/primnodes.h
> +++ b/src/include/nodes/primnodes.h
> @@ -579,6 +579,17 @@ typedef struct GroupingFunc
> * Collation information is irrelevant for the query jumbling, as is the
> * internal state information of the node like "winstar" and "winagg".
> */
> +
> +/*
> + * Null Treatment options. If specified, initially set to PARSER_IGNORE_NULLS
> + * which is then converted to IGNORE_NULLS if the window function allows the
> + * null treatment clause.
> + */
> +#define NO_NULLTREATMENT 0
> +#define PARSER_IGNORE_NULLS 1
> +#define PARSER_RESPECT_NULLS 2
> +#define IGNORE_NULLS 3
> +
> typedef struct WindowFunc
> {
> Expr xpr;
> @@ -602,6 +613,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. One of the Null Treatment options */
> + int ignore_nulls;
> ```
>
> Maybe we can use “uint8” type for “ignore_nulls”. Because the previous two
> are both of type “bool”, an uint8 will just fit to the padding bytes, so that
> new field won’t add extra memory to the structure.
If we change the data type for ignore_nulls in WindowFunc, we may also
want to change it elsewhere (FuncCall, WindowObjectData,
WindowStatePerFuncData) for consistency?
> 3 - 0004
> ```
> winobj->markpos = -1;
> winobj->seekpos = -1;
> +
> + /* reset null map */
> + if (perfuncstate->winobj->ignore_nulls == IGNORE_NULLS)
> + memset(perfuncstate->winobj->notnull_info, 0,
> +
> NN_POS_TO_BYTES(perfuncstate->winobj->num_notnull_info));
> }
> ```
> Where in “if” and “memset()”, we can just use “winobj”.
Good catch. Will fix.
> 4 - 0004
> ```
> + if (!HeapTupleIsValid(proctup))
> + elog(ERROR, "cache lookup failed for function %u",
> funcid);
> + procform = (Form_pg_proc) GETSTRUCT(proctup);
> + elog(ERROR, "function %s does not allow RESPECT/IGNORE NULLS",
> + NameStr(procform->proname));
> ```
>
> “Procform” is assigned but not used.
I think procform is used in the following elog(ERROR, ...).
Best regards,
--
Tatsuo Ishii
SRA OSS K.K.
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp
