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