Hi,
> Thanks for working on this!
>
> I have a couple of comments.
>
> When executing a non-existent function with IGNORE NULLS, for example,
>
> SELECT no_such_func() IGNORE NULLS;
>
> previously we got:
>
> ERROR: function no_such_func() does not exist
>
> but now we get:
>
> ERROR: only window functions accept RESPECT/IGNORE NULLS
>
> Isn't the previous error more helpful in this case? It makes me wonder
> whether the IGNORE NULLS check is being performed too early. Perhaps
> it would be better to move the check to other place.
Agreed. BTW, if the check is moved there, it might be better to
change the error message according to the surroundings. The pattern is
"... specified, but %s is not a ..."
errmsg("OVER specified, but %s is not a window function nor an aggregate
function",
So the message could be changed to something like:
errmsg("RESPECT/IGNORE NULLS is specified, but %s is not a window function"),
> I also noticed that when using an aggregate function as a window
> function, for example,
>
> SELECT sum(i) IGNORE NULLS OVER() FROM ...;
>
> we now get:
>
> ERROR: only window functions accept RESPECT/IGNORE NULLS
>
> whereas previously the error was:
>
> ERROR: aggregate functions do not accept RESPECT/IGNORE NULLS
>
> The new message seems confusing because `sum()` is being used as
> a window function in this case, so saying "only window functions accept ..."
> doesn't seem accurate.
I agree with your concern and am fine with the change.
> /*
> * NULL TREATEMENT is only allowed for window functions per spec.
> */
>
> Also, I noticed a typo: TREATEMENT should be TREATMENT.
Thanks for the correction.
>
> The attached patch implements these changes. Thoughts?
Please see comment above.
Regards,
--
Tatsuo Ishii
SRA OSS K.K.
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp
