Department of Redundancy Department: makeNode(FuncCall) division

David Fetter <david@fetter.org> writes:

Per suggestions and lots of help from Andrew Gierth, please find
attached a patch to clean up the call sites for FuncCall nodes, which
I'd like to expand centrally rather than in each of the 37 (or 38, but
I only redid 37) places where it's called. The remaining one is in
src/backend/nodes/copyfuncs.c, which has to be modified for any
changes in the that struct anyhow.

TBH, I don't think this is an improvement.

The problem with adding a new field to any struct is that you have to
run around and examine (and, usually, modify) every place that
manufactures that type of struct. With a makeFuncCall defined like
this, you'd still have to do that; it would just become a lot easier
to forget to do so.

If the subroutine were defined like most other makeXXX subroutines,
ie you have to supply *all* the fields, that argument would go away,
but the notational advantage is then dubious.

The bigger-picture point is that you're proposing to make the coding
conventions for building FuncCalls different from what they are for
any other grammar node. I don't think that's a great idea; it will
mostly foster confusion.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Sun, Feb 10, 2013 at 10:09:19AM -0500, Tom Lane wrote:

David Fetter <david@fetter.org> writes:

Per suggestions and lots of help from Andrew Gierth, please find
attached a patch to clean up the call sites for FuncCall nodes, which
I'd like to expand centrally rather than in each of the 37 (or 38, but
I only redid 37) places where it's called. The remaining one is in
src/backend/nodes/copyfuncs.c, which has to be modified for any
changes in the that struct anyhow.

TBH, I don't think this is an improvement.

The problem with adding a new field to any struct is that you have to
run around and examine (and, usually, modify) every place that
manufactures that type of struct. With a makeFuncCall defined like
this, you'd still have to do that; it would just become a lot easier
to forget to do so.

So you're saying that I've insufficiently put the choke point there?

It seems to me that needing to go back to each and every jot and
tittle of the code to add a new default parameter in each place is a
recipe for mishaps, where in this one, the new correct defaults will
just appear in all the places they need to.

If the subroutine were defined like most other makeXXX subroutines,
ie you have to supply *all* the fields, that argument would go away,
but the notational advantage is then dubious.

The bigger-picture point is that you're proposing to make the coding
conventions for building FuncCalls different from what they are for
any other grammar node. I don't think that's a great idea; it will
mostly foster confusion.

I find this a good bit less confusing than the litany of repeated
parameter settings, the vast majority of which in most cases are along
the lines of NULL/NIL/etc.

This way, anything set that's not the default stands out.

Cheers,
David.
--
David Fetter <david@fetter.org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david.fetter@gmail.com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Sun, Feb 10, 2013 at 10:09:19AM -0500, Tom Lane wrote:

David Fetter <david@fetter.org> writes:

Per suggestions and lots of help from Andrew Gierth, please find
attached a patch to clean up the call sites for FuncCall nodes, which
I'd like to expand centrally rather than in each of the 37 (or 38, but
I only redid 37) places where it's called. The remaining one is in
src/backend/nodes/copyfuncs.c, which has to be modified for any
changes in the that struct anyhow.

TBH, I don't think this is an improvement.

The problem with adding a new field to any struct is that you have to
run around and examine (and, usually, modify) every place that
manufactures that type of struct. With a makeFuncCall defined like
this, you'd still have to do that; it would just become a lot easier
to forget to do so.

If the subroutine were defined like most other makeXXX subroutines,
ie you have to supply *all* the fields, that argument would go away,
but the notational advantage is then dubious.

The bigger-picture point is that you're proposing to make the coding
conventions for building FuncCalls different from what they are for
any other grammar node. I don't think that's a great idea; it will
mostly foster confusion.

The major difference between FuncCalls and others is that `while most
raw-parsetree nodes are constructed only in their own syntax
productions, FuncCall is constructed in many places unrelated to
actual function call syntax.

This really will make things a good bit easier on our successors.

Cheers,
David.
--
David Fetter <david@fetter.org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david.fetter@gmail.com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Sat, Feb 09, 2013 at 11:59:22PM -0800, David Fetter wrote:

Folks,

Per suggestions and lots of help from Andrew Gierth, please find
attached a patch to clean up the call sites for FuncCall nodes, which
I'd like to expand centrally rather than in each of the 37 (or 38, but
I only redid 37) places where it's called. The remaining one is in
src/backend/nodes/copyfuncs.c, which has to be modified for any
changes in the that struct anyhow.

The immediate purpose is two-fold: to reduce some redundancies, which
I believe is worth doing in and of itself, and to prepare for adding
FILTER on aggregates from the spec, and possibly other things in
the <aggregate function> part of the spec.

Cheers,
David.

Folks,

Please find attached two versions of a patch which provides optional
FILTER clause for aggregates (T612, "Advanced OLAP operations").

The first is intended to be applied on top of the previous patch, the
second without it. The first is, I believe, clearer in what it's
doing. Rather than simply mechanically visiting every place a
function call might be constructed, it visits a central one to change
the default, then goes only to the places where it's relevant.

The patches are both early WIP as they contain no docs or regression
tests yet.

Cheers,
David.
--
David Fetter <david@fetter.org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david.fetter@gmail.com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

On Wed, Feb 13, 2013 at 06:45:31AM -0800, David Fetter wrote:

On Sat, Feb 09, 2013 at 11:59:22PM -0800, David Fetter wrote:

Folks,

Per suggestions and lots of help from Andrew Gierth, please find
attached a patch to clean up the call sites for FuncCall nodes, which
I'd like to expand centrally rather than in each of the 37 (or 38, but
I only redid 37) places where it's called. The remaining one is in
src/backend/nodes/copyfuncs.c, which has to be modified for any
changes in the that struct anyhow.

The immediate purpose is two-fold: to reduce some redundancies, which
I believe is worth doing in and of itself, and to prepare for adding
FILTER on aggregates from the spec, and possibly other things in
the <aggregate function> part of the spec.

Cheers,
David.

Folks,

Please find attached two versions of a patch which provides optional
FILTER clause for aggregates (T612, "Advanced OLAP operations").

The first is intended to be applied on top of the previous patch, the
second without it.

I'll find a brown paper back to wear over my head at some point, but
meanwhile, here's a cleaned-up version of the patch that doesn't use
makeFuncArgs, now without merge artifacts and with the ability to
actually compile. It's still WIP in the sense previously mentioned.

Cheers,
David.
--
David Fetter <david@fetter.org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david.fetter@gmail.com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

On Wed, Feb 13, 2013 at 06:45:31AM -0800, David Fetter wrote:

On Sat, Feb 09, 2013 at 11:59:22PM -0800, David Fetter wrote:

Folks,

Per suggestions and lots of help from Andrew Gierth, please find
attached a patch to clean up the call sites for FuncCall nodes, which
I'd like to expand centrally rather than in each of the 37 (or 38, but
I only redid 37) places where it's called. The remaining one is in
src/backend/nodes/copyfuncs.c, which has to be modified for any
changes in the that struct anyhow.

The immediate purpose is two-fold: to reduce some redundancies, which
I believe is worth doing in and of itself, and to prepare for adding
FILTER on aggregates from the spec, and possibly other things in
the <aggregate function> part of the spec.

Cheers,
David.

Folks,

Please find attached two versions of a patch which provides optional
FILTER clause for aggregates (T612, "Advanced OLAP operations").

The first is intended to be applied on top of the previous patch, the
second without it. The first is, I believe, clearer in what it's
doing. Rather than simply mechanically visiting every place a
function call might be constructed, it visits a central one to change
the default, then goes only to the places where it's relevant.

The patches are both early WIP as they contain no docs or regression
tests yet.

Docs and regression tests added, makeFuncArgs approached dropped for
now, will re-visit later.

Cheers,
David.
--
David Fetter <david@fetter.org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david.fetter@gmail.com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

On Tue, Feb 26, 2013 at 01:09:30PM -0800, David Fetter wrote:

On Wed, Feb 13, 2013 at 06:45:31AM -0800, David Fetter wrote:

On Sat, Feb 09, 2013 at 11:59:22PM -0800, David Fetter wrote:

Folks,

Per suggestions and lots of help from Andrew Gierth, please find
attached a patch to clean up the call sites for FuncCall nodes, which
I'd like to expand centrally rather than in each of the 37 (or 38, but
I only redid 37) places where it's called. The remaining one is in
src/backend/nodes/copyfuncs.c, which has to be modified for any
changes in the that struct anyhow.

The immediate purpose is two-fold: to reduce some redundancies, which
I believe is worth doing in and of itself, and to prepare for adding
FILTER on aggregates from the spec, and possibly other things in
the <aggregate function> part of the spec.

Cheers,
David.

Folks,

Please find attached two versions of a patch which provides optional
FILTER clause for aggregates (T612, "Advanced OLAP operations").

The first is intended to be applied on top of the previous patch, the
second without it. The first is, I believe, clearer in what it's
doing. Rather than simply mechanically visiting every place a
function call might be constructed, it visits a central one to change
the default, then goes only to the places where it's relevant.

The patches are both early WIP as they contain no docs or regression
tests yet.

Docs and regression tests added, makeFuncArgs approached dropped for
now, will re-visit later.

Regression tests added to reflect bug fixes in COLLATE.

Cheers,
David.
--
David Fetter <david@fetter.org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david.fetter@gmail.com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

On Sun, Apr 28, 2013 at 01:29:41PM -0700, David Fetter wrote:

On Tue, Feb 26, 2013 at 01:09:30PM -0800, David Fetter wrote:

On Wed, Feb 13, 2013 at 06:45:31AM -0800, David Fetter wrote:

On Sat, Feb 09, 2013 at 11:59:22PM -0800, David Fetter wrote:

Folks,

Per suggestions and lots of help from Andrew Gierth, please find
attached a patch to clean up the call sites for FuncCall nodes, which
I'd like to expand centrally rather than in each of the 37 (or 38, but
I only redid 37) places where it's called. The remaining one is in
src/backend/nodes/copyfuncs.c, which has to be modified for any
changes in the that struct anyhow.

The immediate purpose is two-fold: to reduce some redundancies, which
I believe is worth doing in and of itself, and to prepare for adding
FILTER on aggregates from the spec, and possibly other things in
the <aggregate function> part of the spec.

Cheers,
David.

Folks,

Please find attached two versions of a patch which provides optional
FILTER clause for aggregates (T612, "Advanced OLAP operations").

The first is intended to be applied on top of the previous patch, the
second without it. The first is, I believe, clearer in what it's
doing. Rather than simply mechanically visiting every place a
function call might be constructed, it visits a central one to change
the default, then goes only to the places where it's relevant.

The patches are both early WIP as they contain no docs or regression
tests yet.

Docs and regression tests added, makeFuncArgs approached dropped for
now, will re-visit later.

Regression tests added to reflect bug fixes in COLLATE.

Rebased vs. master.

Cheers,
David.
--
David Fetter <david@fetter.org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david.fetter@gmail.com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

On Mon, Feb 11, 2013 at 10:48:38AM -0800, David Fetter wrote:

On Sun, Feb 10, 2013 at 10:09:19AM -0500, Tom Lane wrote:

David Fetter <david@fetter.org> writes:

Per suggestions and lots of help from Andrew Gierth, please find
attached a patch to clean up the call sites for FuncCall nodes, which
I'd like to expand centrally rather than in each of the 37 (or 38, but
I only redid 37) places where it's called. The remaining one is in
src/backend/nodes/copyfuncs.c, which has to be modified for any
changes in the that struct anyhow.

TBH, I don't think this is an improvement.

The problem with adding a new field to any struct is that you have to
run around and examine (and, usually, modify) every place that
manufactures that type of struct. With a makeFuncCall defined like
this, you'd still have to do that; it would just become a lot easier
to forget to do so.

If the subroutine were defined like most other makeXXX subroutines,
ie you have to supply *all* the fields, that argument would go away,
but the notational advantage is then dubious.

The bigger-picture point is that you're proposing to make the coding
conventions for building FuncCalls different from what they are for
any other grammar node. I don't think that's a great idea; it will
mostly foster confusion.

The major difference between FuncCalls and others is that `while most
raw-parsetree nodes are constructed only in their own syntax
productions, FuncCall is constructed in many places unrelated to
actual function call syntax.

This really will make things a good bit easier on our successors.

Here's a rebased patch with comments illustrating how best to employ.

In my previous message, I characterized the difference between
FuncCalls and other raw-parsetree nodes. Is there some flaw in that
logic? If there isn't, is there some reason not to treat them in a
less redundant, more uniform manner as this patch does?

Cheers,
David.
--
David Fetter <david@fetter.org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david.fetter@gmail.com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

* David Fetter (david@fetter.org) wrote:

On Mon, Feb 11, 2013 at 10:48:38AM -0800, David Fetter wrote:

On Sun, Feb 10, 2013 at 10:09:19AM -0500, Tom Lane wrote:

David Fetter <david@fetter.org> writes:

Per suggestions and lots of help from Andrew Gierth, please find
attached a patch to clean up the call sites for FuncCall nodes, which
I'd like to expand centrally rather than in each of the 37 (or 38, but
I only redid 37) places where it's called. The remaining one is in
src/backend/nodes/copyfuncs.c, which has to be modified for any
changes in the that struct anyhow.

TBH, I don't think this is an improvement.

The problem with adding a new field to any struct is that you have to
run around and examine (and, usually, modify) every place that
manufactures that type of struct. With a makeFuncCall defined like
this, you'd still have to do that; it would just become a lot easier
to forget to do so.

I don't really see how finding all callers of makeFuncCall is
particularly harder than finding the callers of makeNode(Func). If
there were cases where we still wanted to use makeNode(Func), perhaps
that would be annoying since you'd have to look for both- but, iiuc,
this patch changes all of the callers to use makeFuncCall and it seems
reasonable for all callers to do so in the future as well.

It looks to me like the advantage of this patch is exactly that you
*don't* have to run around and modify things which are completely
unrelated to the new feature being added (eg: FILTER). Add the new
field, set up the default/no-op case in makeFuncCall() and then only
change those places that actually need to worry about your new field.

If the subroutine were defined like most other makeXXX subroutines,
ie you have to supply *all* the fields, that argument would go away,
but the notational advantage is then dubious.

Having to supply all the fields certainly wouldn't make things any
better. Providing the base set of fields which are required to be set
for any FuncCall node does make sense though, which is what the patch
does. The rest of the fields are all special cases for which a default
value works perfectly fine (when the field isn't involved in the
specific case being handled).

The bigger-picture point is that you're proposing to make the coding
conventions for building FuncCalls different from what they are for
any other grammar node. I don't think that's a great idea; it will
mostly foster confusion.

The major difference between FuncCalls and others is that `while most
raw-parsetree nodes are constructed only in their own syntax
productions, FuncCall is constructed in many places unrelated to
actual function call syntax.

Yeah, FuncCall's are already rather special and they're built all over
the place.

That's my 2c on it anyhow. I don't see it as some kind of major
milestone but it looks like improvement to me and likely to make things
a bit easier on patch authors and reviewers who otherwise have to ponder
a bunch of repeated 'x->q = false;' statements in areas which are
completely unrelated to the new feature itself.

Thanks,

Stephen

On 17 June 2013 06:36, David Fetter <david@fetter.org> wrote:

Please find attached two versions of a patch which provides optional
FILTER clause for aggregates (T612, "Advanced OLAP operations").

The first is intended to be applied on top of the previous patch, the
second without it. The first is, I believe, clearer in what it's
doing. Rather than simply mechanically visiting every place a
function call might be constructed, it visits a central one to change
the default, then goes only to the places where it's relevant.

The patches are both early WIP as they contain no docs or regression
tests yet.

Docs and regression tests added, makeFuncArgs approached dropped for
now, will re-visit later.

Regression tests added to reflect bug fixes in COLLATE.

Rebased vs. master.

Hi,

I've been looking at this patch, which adds support for the SQL
standard feature of applying a filter to rows used in an aggregate.
The syntax matches the spec:

agg_fn ( <args> [ ORDER BY <sort_clause> ] ) [ FILTER ( WHERE <qual> ) ]

and this patch makes FILTER a new reserved keyword, usable as a
function or type, but not in other contexts, e.g., as a table name or
alias.

I'm not sure if that might be a problem for some people, but I can't
see any way round it, since otherwise there would be no way for the
parser to distinguish a filter clause from an alias expression.

The feature appears to work per-spec, and the code and doc changes
look reasonable. However, it looks a little light on regression tests,
and so I think some necessary changes have been overlooked.

In my testing of sub-queries in the FILTER clause (an extension to the
spec), I was able to produce the following error:

CREATE TABLE t1(a1 int);
CREATE TABLE t2(a2 int);
INSERT INTO t1 SELECT * FROM generate_series(1,10);
INSERT INTO t2 SELECT * FROM generate_series(3,6);

SELECT array_agg(a1) FILTER (WHERE a1 IN (SELECT a2 FROM t2)) FROM t1;

ERROR: plan should not reference subplan's variable

which looks to be related to subselect.c's handling of sub-queries in
aggregates.

Regards,
Dean

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Thu, Jun 20, 2013 at 08:59:27PM +0100, Dean Rasheed wrote:

On 17 June 2013 06:36, David Fetter <david@fetter.org> wrote:

Please find attached two versions of a patch which provides optional
FILTER clause for aggregates (T612, "Advanced OLAP operations").

The first is intended to be applied on top of the previous patch, the
second without it. The first is, I believe, clearer in what it's
doing. Rather than simply mechanically visiting every place a
function call might be constructed, it visits a central one to change
the default, then goes only to the places where it's relevant.

The patches are both early WIP as they contain no docs or regression
tests yet.

Docs and regression tests added, makeFuncArgs approached dropped for
now, will re-visit later.

Regression tests added to reflect bug fixes in COLLATE.

Rebased vs. master.

Hi,

I've been looking at this patch, which adds support for the SQL
standard feature of applying a filter to rows used in an aggregate.
The syntax matches the spec:

agg_fn ( <args> [ ORDER BY <sort_clause> ] ) [ FILTER ( WHERE <qual> ) ]

and this patch makes FILTER a new reserved keyword, usable as a
function or type, but not in other contexts, e.g., as a table name or
alias.

I'm not sure if that might be a problem for some people, but I can't
see any way round it, since otherwise there would be no way for the
parser to distinguish a filter clause from an alias expression.

The feature appears to work per-spec, and the code and doc changes
look reasonable. However, it looks a little light on regression tests,

What tests do you think should be there that aren't?

and so I think some necessary changes have been overlooked.

In my testing of sub-queries in the FILTER clause (an extension to the
spec), I was able to produce the following error:

Per the spec,

B) A <filter clause> shall not contain a <query expression>, a <window function>, or an outer reference.

I'd be happy to see about adding one despite this, though. That they
didn't figure out how doesn't mean we shouldn't eventually, ideally in
time for 9.4.

Cheers,
David.
--
David Fetter <david@fetter.org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david.fetter@gmail.com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

David Fetter escribi�:

On Thu, Jun 20, 2013 at 08:59:27PM +0100, Dean Rasheed wrote:

In my testing of sub-queries in the FILTER clause (an extension to the
spec), I was able to produce the following error:

Per the spec,

B) A <filter clause> shall not contain a <query expression>, a <window function>, or an outer reference.

If this is not allowed, I think there should be a clearer error message.
What Dean showed is an internal (not user-facing) error message.

--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Fri, Jun 21, 2013 at 12:10:25AM -0400, Alvaro Herrera wrote:

David Fetter escribi�:

On Thu, Jun 20, 2013 at 08:59:27PM +0100, Dean Rasheed wrote:

In my testing of sub-queries in the FILTER clause (an extension
to the spec), I was able to produce the following error:

Per the spec,

B) A <filter clause> shall not contain a <query expression>, a
<window function>, or an outer reference.

If this is not allowed, I think there should be a clearer error
message. What Dean showed is an internal (not user-facing) error
message.

Folding to lower isn't allowed by the spec either, and then there's
the matter of arrays...

Before going to lots of trouble to figure out how to throw an error
that says, "only the spec and no further," I'd like to investigate how
to make this work.

Cheers,
David.
--
David Fetter <david@fetter.org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david.fetter@gmail.com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Fri, Jun 21, 2013 at 12:10:25AM -0400, Alvaro Herrera wrote:

David Fetter escribi�:

On Thu, Jun 20, 2013 at 08:59:27PM +0100, Dean Rasheed wrote:

In my testing of sub-queries in the FILTER clause (an extension to the
spec), I was able to produce the following error:

Per the spec,

B) A <filter clause> shall not contain a <query expression>, a <window function>, or an outer reference.

If this is not allowed, I think there should be a clearer error message.
What Dean showed is an internal (not user-facing) error message.

Please find attached a patch which allows subqueries in the FILTER
clause and adds regression testing for same.

Cheers,
David.
--
David Fetter <david@fetter.org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david.fetter@gmail.com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

On 21 June 2013 05:01, David Fetter <david@fetter.org> wrote:

What tests do you think should be there that aren't?

I think I expected to see more tests related to some of the specific
code changes, such as

CREATE TABLE t AS SELECT * FROM generate_series(1,10) t(x);

-- Should fail (filter can't be used for non-aggregates)
SELECT abs(x) FILTER (WHERE x > 5) FROM t;

-- Should fail (filter clause can't contain aggregates)
SELECT array_agg(x) FILTER (WHERE x > AVG(x)) t;

-- OK (aggregate in filter sub-query)
SELECT array_agg(x) FILTER (WHERE x > (SELECT AVG(x) FROM t)) FROM t;

-- OK (trivial sub-query)
SELECT array_agg(x) FILTER (WHERE (SELECT x > 5)) FROM t;

-- OK
SELECT array_agg(x) FILTER (WHERE (SELECT x > (SELECT AVG(x) FROM t))) FROM t;

-- OK
SELECT array_agg(x) FILTER (WHERE (SELECT (SELECT t.x > AVG(t2.x) FROM
t as t2))) FROM t;

-- Should fail
SELECT array_agg(x) FILTER (WHERE (SELECT (SELECT 5 > AVG(t.x) FROM t
as t2))) FROM t;

-- OK (filtered aggregate in window context)
SELECT x, AVG(x) OVER(ORDER BY x), AVG(x) FILTER (WHERE x > 5)
OVER(ORDER BY x) FROM t;

-- Should fail (non-aggregate window function with filter)
SELECT x, rank() OVER(ORDER BY x), rank() FILTER (WHERE x > 5)
OVER(ORDER BY x) FROM t;

-- View definition test
CREATE VIEW v AS SELECT array_agg(x) FILTER (WHERE (SELECT (SELECT t.x

AVG(t2.x) FROM t as t2))) FROM t;

\d+ v

etc...

You could probably dream up better examples --- I just felt that the
current tests don't give much coverage of the code changes.

Regards,
Dean

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 21 June 2013 06:16, David Fetter <david@fetter.org> wrote:

On Fri, Jun 21, 2013 at 12:10:25AM -0400, Alvaro Herrera wrote:

David Fetter escribió:

On Thu, Jun 20, 2013 at 08:59:27PM +0100, Dean Rasheed wrote:

In my testing of sub-queries in the FILTER clause (an extension to the
spec), I was able to produce the following error:

Per the spec,

B) A <filter clause> shall not contain a <query expression>, a <window function>, or an outer reference.

If this is not allowed, I think there should be a clearer error message.
What Dean showed is an internal (not user-facing) error message.

Please find attached a patch which allows subqueries in the FILTER
clause and adds regression testing for same.

Nice!

I should have pointed out that it was already working for some
sub-queries, just not all. It's good to see that it was basically just
a one-line fix, because I think it would have been a shame to not
support sub-queries.

I hope to take another look at it over the weekend.

Regards,
Dean

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 21 June 2013 10:02, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:

On 21 June 2013 06:16, David Fetter <david@fetter.org> wrote:

On Fri, Jun 21, 2013 at 12:10:25AM -0400, Alvaro Herrera wrote:

David Fetter escribió:

On Thu, Jun 20, 2013 at 08:59:27PM +0100, Dean Rasheed wrote:

In my testing of sub-queries in the FILTER clause (an extension to the
spec), I was able to produce the following error:

Per the spec,

B) A <filter clause> shall not contain a <query expression>, a <window function>, or an outer reference.

If this is not allowed, I think there should be a clearer error message.
What Dean showed is an internal (not user-facing) error message.

Please find attached a patch which allows subqueries in the FILTER
clause and adds regression testing for same.

Nice!

I should have pointed out that it was already working for some
sub-queries, just not all. It's good to see that it was basically just
a one-line fix, because I think it would have been a shame to not
support sub-queries.

I hope to take another look at it over the weekend.

I'm still not happy that this patch is making FILTER a new reserved
keyword, because I think it is a common enough English word (and an
obscure enough SQL keyword) that people may well have used it for
table names or aliases, and so their code will break.

There is another thread discussing a similar issue with lead/lag ignore nulls:
/messages/by-id/CAOdE5WQnfR737OkxNXuLWnwpL7=OUssC-63ijP2=2RnRTw8emQ@mail.gmail.com

Playing around with the idea proposed there, it seems that it is
possible to make FILTER (and also OVER) unreserved keywords, by
splitting out the production of aggregate/window functions from normal
functions in gram.y. Aggregate/window functions are not allowed in
index_elem or func_table constructs, but they may appear in c_expr's.
That resolves the shift/reduce conflicts that would otherwise occur
from subsequent alias clauses, allowing FILTER and OVER to be
unreserved.

There is a drawback --- certain error messages become less specific,
for example: "SELECT * FROM rank() OVER(ORDER BY random());" is now a
syntax error, rather than the more useful error saying that window
functions aren't allowed in the FROM clause. That doesn't seem like
such a common case though.

What do you think?

Regards,
Dean

Dean Rasheed <dean.a.rasheed@gmail.com> wrote:

I'm still not happy that this patch is making FILTER a new reserved
keyword, because I think it is a common enough English word (and an
obscure enough SQL keyword) that people may well have used it for
table names or aliases, and so their code will break.

Well, if it is a useful capability with a standard syntax, I think
we should go with the standard syntax even if it might break
application code that uses, as unquoted identifiers, words reserved
by the SQL standard.  Of course, non-reserved is better than
reserved if we can manage it.

Playing around with the idea proposed there, it seems that it is
possible to make FILTER (and also OVER) unreserved keywords, by
splitting out the production of aggregate/window functions from normal
functions in gram.y. Aggregate/window functions are not allowed in
index_elem or func_table constructs, but they may appear in c_expr's.
That resolves the shift/reduce conflicts that would otherwise occur
from subsequent alias clauses, allowing FILTER and OVER to be
unreserved.

There is a drawback --- certain error messages become less specific,
for example: "SELECT * FROM rank() OVER(ORDER BY random());" is now a
syntax error, rather than the more useful error saying that window
functions aren't allowed in the FROM clause. That doesn't seem like
such a common case though.

What do you think?

I think it is OK if that gets a syntax error.  If that's the "worst
case" I like this approach.

This also helps keep down the size of the generated parse tables,
doesn't it?

--
Kevin Grittner
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers