[GitHub] ilooner edited a comment on issue #1508: DRILL-6804: Simplify usage of OperatorPhase in HashAgg.

[GitBox <gi...@apache.org>]

ilooner edited a comment on issue #1508: DRILL-6804: Simplify usage of OperatorPhase in HashAgg.
URL: https://github.com/apache/drill/pull/1508#issuecomment-431527205
 
 
   @Ben-Zvi the idea is to use an object oriented approach for structuring the code. Instead of computing values like is2ndPhase from the OperatorPhase object within HashAgg, we can make the values a property of the OperatorPhase itself.
   
   It also helps reduce the number of variables present in HashAgg template from these three
   
   ```
     private boolean isTwoPhase = false;
     private boolean is2ndPhase = false;	
     private boolean is1stPhase = false;
   ```
   
   to just a single operatorPhase variable. 
   
   This is useful when we move the code in HashAggTemplate into separate HashAggPartition, PartitionEviction, and HashAggMemoryCalculator classes since it minimizes the number of variables that need to be passed to these classes. This change was done in preparation for that restructuring.
   
   Perhaps I could changes the names to from `operatorPhase.isSecondPhase()` to something like this `phase.is2nd()`. Similarly I could change the line you pointed out from `popConfig.getAggPhase().isSecondPhase()` to two separate lines like
   
   ```
   OperatorPhase phase = popConfig.getAggPhase();
   phase.is2nd()
   ```
   
   Let me know your thoughts.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services