Re: WIP: Aggregation push-down

Andy Fan <zhihui(dot)fan1213(at)gmail(dot)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 hard
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 design
> 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 which 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 about
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=target->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 this
style quite often too. I think the reason is that it allows for more complex
tests, while the "else-if style" requires all tests to take place inside the
"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