From: Antonin Houska <ah@cybertec.at>
To: Andy Fan <zhihui.fan1213@gmail.com>
cc: legrand legrand <legrand_legrand@hotmail.com>,
    pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Re: WIP: Aggregation push-down
In-reply-to: 
 <CAKU4AWqK_X4L5WDsjnsFpm_Gt8Mx0+H4frC-6Pom1ro=aq44mQ@mail.gmail.com>
References: <11172.1550780283@sss.pgh.pa.us> <23468.1551369774@localhost>
 <CAN_9JTy95UMjOVDSc-4MBrA6Fc1hX2vh=t2RMHp=yGyAV=-Xsw@mail.gmail.com>
 <1897.1562680064@spoje.net>
 <CAN_9JTzC1K-Gc3p5GD=dQeJD8k_Z5996B0rEYjVGx+pXPq8JPw@mail.gmail.com>
 <13971.1562920911@spoje.net>
 <CAN_9JTzAsBwv5zr4SRVO35OtMWQBXSLcUuY65NQ0ZtFCTOrMMA@mail.gmail.com>
 <19857.1563374392@localhost> <20190904202339.GA13644@alvherre.pgsql>
 <7087.1567663858@localhost> <1582751446111-0.post@n3.nabble.com>
 <20715.1582793501@antos>
 <CAKU4AWqK_X4L5WDsjnsFpm_Gt8Mx0+H4frC-6Pom1ro=aq44mQ@mail.gmail.com>
Comments: In-reply-to Andy Fan <zhihui.fan1213@gmail.com>
   message dated "Tue, 21 Apr 2020 16:37:25 +0800."
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <50401.1587730290.1@antos>
Content-Transfer-Encoding: quoted-printable
Date: Fri, 24 Apr 2020 14:11:30 +0200
Message-ID: <50402.1587730290@antos>
Precedence: bulk

Andy Fan <zhihui.fan1213@gmail.com> wrote:

> The more tests on your patch, the more powerful I feel it is!

Thanks for the appreciation. Given the poor progress it's increasingly har=
d
for me to find motivation to work on it. I'll try to be more professional =
:-)

> At the same time, I think the most difficult part to understand your des=
ign
> is you can accept any number of generic join clauses, so I guess more
> explanation on this part may be helpful.

ok, I'll consider adding some comments, although the concept is mentioned =
in
optimizer/README

+Furthermore, extra grouping columns can be added to the partial Agg node =
if a
+join clause above that node references a column which is not in the query
+GROUP BY clause and which could not be derived using equivalence class.
+
...

> At the code level, I did some slight changes on init_grouping_targets wh=
ich may
> make the code easier to read.  You are free to to use/not use it.

I'm going to accept your change of create_rel_agg_info(), but I hesitate a=
bout
the changes to init_grouping_targets().

First, is it worth to spend CPU cycles on construction of an extra list
grouping_columns? Is there a corner case in which we cannot simply pass
grouping_columns=3Dtarget->exprs to check_functional_grouping()?

Second, it's obvious that you prefer the style

    foreach ()
    {
        if ()
           ...
        else if ()
           ...
        else
           ...
    }

over this

    foreach ()
    {
        if ()
        {
           ...
           continue;
        }

        if ()
        {
           ...
           continue;
        }

        ...
    }

I often prefer the latter and I see that the existing planner code uses th=
is
style quite often too. I think the reason is that it allows for more compl=
ex
tests, while the "else-if style" requires all tests to take place inside t=
he
"if ()" expression. However, if several (not necessarily tightly related)
tests become "compressed" this way, it's less obvious how where to put
comments.  Indeed it seems that some comments got lost due to your changes=
.

-- =

Antonin Houska
Web: https://www.cybertec-postgresql.com