Thomas Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/10394 )
Change subject: IMPALA-110 (part 2): Refactor PartitionedAggregationNode ...................................................................... Patch Set 2: > > (14 comments) > > > > While developing this patch, I maintained a branch with a series > of > > commits separating the work out into several mostly mechanical > > steps to keep everything straight. > > > > Does each step function on it's own? it might be easier to just > review and commit each step individually (or at least some grouping > of the steps) so that big mechanical changes are easier to review. They don't really make much sense on their own (eg. the first step just copies partitioned-aggregation-node.(h|cc) like 5 times) though since its mostly all in new files it would be easy to not add them to cmake until the final step so that the intermediate steps will still compile and pass tests. I think that's probably unnecessary (hopefully this shouldn't be too terrible to review, since the logic is entirely unchanged), but if there's s desire for that, I would suggest at least that I squash it down to two commits: mechanical changes and then interesting changes, rather than the 6 commits it is now. -- To view, visit http://gerrit.cloudera.org:8080/10394 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9e7bb583f54aa4add3738bde7f57cf3511ac567e Gerrit-Change-Number: 10394 Gerrit-PatchSet: 2 Gerrit-Owner: Thomas Marshall <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Thomas Marshall <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Reviewer: Vuk Ercegovac <[email protected]> Gerrit-Comment-Date: Mon, 18 Jun 2018 19:30:30 +0000 Gerrit-HasComments: No
