From 920c8261d58c10de7e68d99c8dd21a9650928d59 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Tue, 5 Nov 2013 21:58:08 -0500 Subject: [PATCH] Improve the error message given for modifying a window with frame clause. For rather inscrutable reasons, SQL:2008 disallows copying-and-modifying a window definition that has any explicit framing clause. The error message we gave for this only made sense if the referencing window definition itself contains an explicit framing clause, which it might well not. Moreover, in the context of an OVER clause it's not exactly obvious that "OVER (windowname)" implies copy-and-modify while "OVER windowname" does not. This has led to multiple complaints, eg bug #5199 from Iliya Krapchatov. Change to a hopefully more intelligible error message, and in the case where we have just "OVER (windowname)", add a HINT suggesting that omitting the parentheses will fix it. Also improve the related documentation. Back-patch to all supported branches. --- doc/src/sgml/syntax.sgml | 32 ++++++++++++++++++------------- src/backend/parser/parse_clause.c | 31 +++++++++++++++++++++++++----- 2 files changed, 45 insertions(+), 18 deletions(-) diff --git a/doc/src/sgml/syntax.sgml b/doc/src/sgml/syntax.sgml index e3dbc4b5ea..4fe872290a 100644 --- a/doc/src/sgml/syntax.sgml +++ b/doc/src/sgml/syntax.sgml @@ -1730,10 +1730,10 @@ SELECT string_agg(a ORDER BY a, ',') FROM table; -- incorrect The syntax of a window function call is one of the following: -function_name (expression , expression ... ) [ FILTER ( WHERE filter_clause ) ] OVER ( window_definition ) function_name (expression , expression ... ) [ FILTER ( WHERE filter_clause ) ] OVER window_name -function_name ( * ) [ FILTER ( WHERE filter_clause ) ] OVER ( window_definition ) +function_name (expression , expression ... ) [ FILTER ( WHERE filter_clause ) ] OVER ( window_definition ) function_name ( * ) [ FILTER ( WHERE filter_clause ) ] OVER window_name +function_name ( * ) [ FILTER ( WHERE filter_clause ) ] OVER ( window_definition ) where window_definition has the syntax @@ -1768,15 +1768,14 @@ UNBOUNDED FOLLOWING window_name is a reference to a named window specification defined in the query's WINDOW clause. - Named window specifications are usually referenced with just - OVER window_name, but it is - also possible to write a window name inside the parentheses and then - optionally supply an ordering clause and/or frame clause (the referenced - window must lack these clauses, if they are supplied here). - This latter syntax follows the same rules as modifying an existing - window name within the WINDOW clause; see the - reference - page for details. + Alternatively, a full window_definition can + be given within parentheses, using the same syntax as for defining a + named window in the WINDOW clause; see the + reference page for details. It's worth + pointing out that OVER wname is not exactly equivalent to + OVER (wname); the latter implies copying and modifying the + window definition, and will be rejected if the referenced window + specification includes a frame clause. @@ -1853,12 +1852,19 @@ UNBOUNDED FOLLOWING PRECEDING is not allowed. + + If FILTER is specified, then only the input + rows for which the filter_clause + evaluates to true are fed to the window function; other rows + are discarded. Only aggregate window functions accept + a FILTER clause. + + The built-in window functions are described in . Other window functions can be added by the user. Also, any built-in or user-defined aggregate function can be - used as a window function. Only aggregate window functions accept - a FILTER clause. + used as a window function. diff --git a/src/backend/parser/parse_clause.c b/src/backend/parser/parse_clause.c index ea90e58f71..1f6306a7d3 100644 --- a/src/backend/parser/parse_clause.c +++ b/src/backend/parser/parse_clause.c @@ -1735,11 +1735,16 @@ transformWindowDefinitions(ParseState *pstate, /* * Per spec, a windowdef that references a previous one copies the * previous partition clause (and mustn't specify its own). It can - * specify its own ordering clause. but only if the previous one had + * specify its own ordering clause, but only if the previous one had * none. It always specifies its own frame clause, and the previous - * one must not have a frame clause. (Yeah, it's bizarre that each of + * one must not have a frame clause. Yeah, it's bizarre that each of * these cases works differently, but SQL:2008 says so; see 7.11 - * syntax rule 10 and general rule 1.) + * syntax rule 10 and general rule 1. The frame + * clause rule is especially bizarre because it makes "OVER foo" + * different from "OVER (foo)", and requires the latter to throw an + * error if foo has a nondefault frame clause. Well, ours not to + * reason why, but we do go out of our way to throw a useful error + * message for such cases. */ if (refwc) { @@ -1778,11 +1783,27 @@ transformWindowDefinitions(ParseState *pstate, wc->copiedOrder = false; } if (refwc && refwc->frameOptions != FRAMEOPTION_DEFAULTS) + { + /* + * Use this message if this is a WINDOW clause, or if it's an OVER + * clause that includes ORDER BY or framing clauses. (We already + * rejected PARTITION BY above, so no need to check that.) + */ + if (windef->name || + orderClause || windef->frameOptions != FRAMEOPTION_DEFAULTS) + ereport(ERROR, + (errcode(ERRCODE_WINDOWING_ERROR), + errmsg("cannot copy window \"%s\" because it has a frame clause", + windef->refname), + parser_errposition(pstate, windef->location))); + /* Else this clause is just OVER (foo), so say this: */ ereport(ERROR, (errcode(ERRCODE_WINDOWING_ERROR), - errmsg("cannot override frame clause of window \"%s\"", - windef->refname), + errmsg("cannot copy window \"%s\" because it has a frame clause", + windef->refname), + errhint("Omit the parentheses in this OVER clause."), parser_errposition(pstate, windef->location))); + } wc->frameOptions = windef->frameOptions; /* Process frame offset expressions */ wc->startOffset = transformFrameOffset(pstate, wc->frameOptions,