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

Attached is the updated v19 patches. Mostly applied changes suggested
by Chao.

>> 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.

Done.

>> 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?

I tried to change all "int ignore_nulls;" to "uint8 ignore_nulls;" but
gen_node_support.pl dislikes it and complains like:

  could not handle type "uint8" in struct "FuncCall" field "ignore_nulls"

>> 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.

Done.

>> 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, ...).

I added more tests for functions (rank(), dense_rank(),
percent_rank(), cume_dist() and ntile()) that do not support
RESPECT/IGNORE NULLS options to confirm that they throw errors if the
options are given. Previously there was only test cases for
row_number().

Also I have made small cosmetic changes to executor/nodeWindowAgg.c to
make too long lines shorter.

Best regards,
--
Tatsuo Ishii
SRA OSS K.K.
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp