Push simple aggregates down

[onderkalaci]

This PR is intended for the following : Users should be able to exploit computational power of worker nodes in servicing aggregates. At minimum, if a GROUP BY is present which references the partition column, we should be able to create a query containing certain safe aggregates.

[jasonmp85]

I added my first round of questions and comments here. I also have some high-level questions:

• How much of this comes (mostly unmodified) from CitusDB? I kept stumbling across pieces that were also in CitusDB somewhere, so it was unclear what is yours and what is not. It might help my code review to know which pieces are entirely novel and which are reused.
• The first function I came across with the safeToPushDownAggregates flag made me think of the rule that boolean flags on methods is an antipattern. And so I was already thinking this when I came across many more methods that do the same thing. Is there any thing you can do to get rid of the boolean flag that we're pushing all the way down the stack? Passing in a boolean flag is basically the caller saying "I know you have a conditional somewhere in your implementation, please take this branch". Might as well hoist up the conditional and have two methods, where appropriate. This might be a Code Complete 2 rule as well (@ozgune)?
• Maybe instead of a flag, you classify the SortGroupClause list into those that should run locally and those that should run remotely? This seems kind of pedantic, and I haven't fully thought it through but I think it could help the code flow be less bifurcated. And it would be analogous to what we see with the restrict clauses.
• How are ORDER BY clauses affected by this change? Would the idea of splitting SortGroupClause lists into remote and local versions make more sense when we think about ORDER BY in addition to GROUP BY? Just brainstorming, I'm not actually sure.
• Do you have any thoughts about how to start splitting pg_shard.c up? If we leave this as-is, the file is now around 1.5 kloc. I wasn't sure what functions were drawing from what CitusDB files, but maybe we want to mimic that structure in some places, PostgreSQL in others? At any rate, one huge planner/executor/hooks/utility file is getting burdensome.

Let's start some conversations around these questions (and maybe you can start investigating cleaning up the boolean flag issue) and I'll add more comments in the next round.

[onderkalaci]

@jasonmp85

• There are two functions that come from CitusDB:
  • GroupedByColumn : This is exactly same with CitusDB implementation.
  • AttributeNumberMutator : This is inspired from MasterAggregateMutator
• As far as I understand from CitusDB codes, it pushes down GROUP BYs in any case (ie: does not need to be on partition column) and executes one more GROUP BY on the master table too. So, this implementation couldn't have much in common with those codes.
• For safeToPushDownAggregates flag and passing it to functions must be replaced. I totally agree with you. Code Complete 2 also states that (Section 7.2). It is mentioned as Logical Cohesion and considered as unacceptable. So, for now I tried to handle smaller items in your first review and concentrate on it next because I need to think about it first. If you have any suggestions, I would like to discuss them too.
• For this implementation, ORDER BYs never pushed down to the workers. We apply them on the local query as we do for other types of queries. Pushing down it to the workers just not work, right? But, we pull required sort columns even if it isn't on the target list. An example is on the test

[onderkalaci]

@jasonmp85 I did some changes on the code flow. In the beginning, I wrote a function whose name is ClassifyTargetList. That function, similar toClassifyRestrictions, could classify the target lists. But, then for other parts of the code (ie: create temp table or updating groupClause of queries), I had to understand whether aggregates are pushed down or not from the target lists. That seemed as weird as previous PR. (Btw, what do you think for that approach? I can switch to it back)

Thus, in the updated PR, I wrote ClassifyHasAggregates functions, then, depending on that values let the code flow accordingly. So, this time, the flag does not directly shows whether we pushed down the aggregates or not. They simple show the value of hasAgg field of distributed and local queries.

I think this time passing those values could not be considered as anti-pattern. What do you think?

Only CreateTemporaryTableStmt seems to be have an anti-pattern, but, Code complete 2 book says that if a function gets a boolean and immediately returns according to that value could not be considered as anti-pattern. So, I think CreateTemporaryTableStmt seems OK from that perspective.

Also, I remembered that I also took CreateStatement directly from CitusDB.

[jasonmp85]

I think in addition to the two you mentioned, all of these also come from CitusDB, no?

• GroupedByColumn
• ValueToStringList
• MasterTargetList
• BuildCreateStatement
• ColumnDefinitionList
• CreateStatement

Tell me whether these have any important changes, because from what I can see they're mostly similar to the CitusDB code and so I mostly need to look at how they're called, not what they contain.

[onderkalaci]

Hey @jasonmp85,

I tried to address your feedback on the changes.

My main concern whether I fell into the same situation we had previously. Could you please consider the changes with that perspective too? This time we do not pass the flag to many functions, but, we still we pass the flag to some functions. Below is your previous comments:

The first function I came across with the safeToPushDownAggregates flag made me think of the rule that boolean flags on methods is an antipattern. And so I was already thinking this when I came across many more methods that do the same thing. Is there any thing you can do to get rid of the boolean flag that we're pushing all the way down the stack? Passing in a boolean flag is basically the caller saying "I know you have a conditional somewhere in your implementation, please take this branch". Might as well hoist up the conditional and have two methods, where appropriate.

Also, I realized that the comment on targetList and/or its name should be changed. What do you think? Previously, the targetList of local/target queries were the same. But, it is not anymore the case. For example, should we change it to localTargetList? Well, it is not localTargetList as well. It basically corresponds to the temp table's schema.