Horton

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

Tatsuo Ishii <ishii@postgresql.org>

View thread
From: Tatsuo Ishii <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
Date: 2025-09-17T09:18:51Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Fix coding style with "else".

  2. Fix multi WinGetFuncArgInFrame/Partition calls with IGNORE NULLS.

  3. Fix Coverity issue reported in commit 2273fa32bce.

  4. Use ereport rather than elog in WinCheckAndInitializeNullTreatment.

  5. Avoid uninitialized-variable warnings from older compilers.

  6. Fix Coverity issues reported in commit 25a30bbd423.

  7. Improve EXPLAIN's display of window functions.

  8. Automatically generate node support functions

> 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