Date: Wed, 17 Sep 2025 18:18:51 +0900 (JST)
Message-Id: <20250917.181851.461494433278003071.ishii@postgresql.org>
To: li.evan.chao@gmail.com
Cc: ojford@gmail.com, krasiyan@gmail.com, tgl@sss.pgh.pa.us,
 vik@postgresfriends.org, andrew@tao11.riddles.org.uk, david@fetter.org,
 pgsql-hackers@lists.postgresql.org
Subject: Re: Add RESPECT/IGNORE NULLS and FROM FIRST/LAST options
From: Tatsuo Ishii <ishii@postgresql.org>
In-Reply-To: <76F5962C-01AA-4D81-B9C2-70590746EC5C@gmail.com>
References: 
 <CAGMVOduZ+7=NR7zW5cLfSRghZAf0yzb3tRceKAXkfg+f-xAZog@mail.gmail.com>
	<20250912.185316.492168971773927670.ishii@postgresql.org>
	<76F5962C-01AA-4D81-B9C2-70590746EC5C@gmail.com>
Mime-Version: 1.0
Content-Type: Text/Plain; charset=iso-2022-jp
Content-Transfer-Encoding: 7bit
Archived-At: 
 <https://www.postgresql.org/message-id/20250917.181851.461494433278003071.ishii%40postgresql.org>
Precedence: bulk

> 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