{"action":"created","comment":{"url":"https://api.github.com/repos/prestodb/presto/pulls/comments/87259792","pull_request_review_id":7874257,"id":87259792,"diff_hunk":"@@ -79,17 +77,114 @@ public Rewriter(SymbolAllocator allocator, PlanNodeIdAllocator idAllocator)\n         }\n \n         @Override\n-        public PlanNode visitAggregation(AggregationNode node, RewriteContext<AggregationNode> context)\n+        public PlanNode visitAggregation(AggregationNode node, RewriteContext<Void> context)\n         {\n+            PlanNode child = node.getSource();\n+\n+            if (!(child instanceof ExchangeNode)) {\n+                return context.defaultRewrite(node);\n+            }\n+\n+            // partial aggregation can only be pushed through exchange that doesn't change\n+            // the cardinality of the stream (i.e., gather or repartition)\n+            ExchangeNode exchange = (ExchangeNode) child;\n+            if (exchange.getType() != GATHER && exchange.getType() != REPARTITION) {\n+                return context.defaultRewrite(node);\n+            }\n+\n+            if (exchange.getType() == REPARTITION) {\n+                if (exchange.getPartitioningScheme().isReplicateNulls()) {\n+                    return context.defaultRewrite(node);\n+                }\n+\n+                // if partitioning columns are not a subset of grouping keys,\n+                // we can't push this through\n+                List<Symbol> partitioningColumns = exchange.getPartitioningScheme()\n+                        .getPartitioning()\n+                        .getArguments()\n+                        .stream()\n+                        .map(Partitioning.ArgumentBinding::getColumn)\n+                        .collect(Collectors.toList());\n+\n+                if (!node.getGroupingKeys().containsAll(partitioningColumns)) {\n+                    return context.defaultRewrite(node);\n+                }\n+            }\n+\n+            // currently, we only support plans that don't use pre-computed hash functions\n+            if (node.getHashSymbol().isPresent() || exchange.getPartitioningScheme().getHashColumn().isPresent()) {\n+                return context.defaultRewrite(node);\n+            }\n+\n             boolean decomposable = node.getFunctions().values().stream()\n                     .map(functionRegistry::getAggregateFunctionImplementation)\n                     .allMatch(InternalAggregationFunction::isDecomposable);\n-            checkState(node.getStep() == SINGLE, \"aggregation should be SINGLE, but it is %s\", node.getStep());\n-            checkState(context.get() == null, \"context is not null: %s\", context);\n-            if (!decomposable || !allowPushThrough(node.getSource())) {\n+\n+            if (!decomposable) {\n                 return context.defaultRewrite(node);\n             }\n \n+            if (node.getStep() == SINGLE) {\n+                return context.rewrite(split(node));\n+            }\n+            else if (node.getStep() == PARTIAL) {\n+                return context.rewrite(pushPartial(node, exchange));\n+            }\n+\n+            return context.defaultRewrite(node);\n+        }\n+\n+        private PlanNode pushPartial(AggregationNode aggregation, ExchangeNode exchange)\n+        {\n+            ImmutableList.Builder<List<Symbol>> exchangeAssignments = ImmutableList.builder();\n+            ImmutableList.Builder<PlanNode> partials = ImmutableList.builder();","path":"presto-main/src/main/java/com/facebook/presto/sql/planner/optimizations/PartialAggregationPushDown.java","position":135,"original_position":135,"commit_id":"c813a217be63c90ac6859e17df7657ac87fc7d1a","original_commit_id":"c813a217be63c90ac6859e17df7657ac87fc7d1a","user":{"login":"dain","id":58298,"avatar_url":"https://avatars.githubusercontent.com/u/58298?v=3","gravatar_id":"","url":"https://api.github.com/users/dain","html_url":"https://github.com/dain","followers_url":"https://api.github.com/users/dain/followers","following_url":"https://api.github.com/users/dain/following{/other_user}","gists_url":"https://api.github.com/users/dain/gists{/gist_id}","starred_url":"https://api.github.com/users/dain/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/dain/subscriptions","organizations_url":"https://api.github.com/users/dain/orgs","repos_url":"https://api.github.com/users/dain/repos","events_url":"https://api.github.com/users/dain/events{/privacy}","received_events_url":"https://api.github.com/users/dain/received_events","type":"User","site_admin":false},"body":"If you make the changes I suggest below, the code will be easier if this is a regular `List`","created_at":"2016-11-09T19:23:57Z","updated_at":"2016-11-09T19:27:44Z","html_url":"https://github.com/prestodb/presto/pull/6571#discussion_r87259792","pull_request_url":"https://api.github.com/repos/prestodb/presto/pulls/6571","_links":{"self":{"href":"https://api.github.com/repos/prestodb/presto/pulls/comments/87259792"},"html":{"href":"https://github.com/prestodb/presto/pull/6571#discussion_r87259792"},"pull_request":{"href":"https://api.github.com/repos/prestodb/presto/pulls/6571"}}},"pull_request":{"url":"https://api.github.com/repos/prestodb/presto/pulls/6571","id":92896118,"html_url":"https://github.com/prestodb/presto/pull/6571","diff_url":"https://github.com/prestodb/presto/pull/6571.diff","patch_url":"https://github.com/prestodb/presto/pull/6571.patch","issue_url":"https://api.github.com/repos/prestodb/presto/issues/6571","number":6571,"state":"open","locked":false,"title":"Simplify aggregation pushdown","user":{"login":"martint","id":14387,"avatar_url":"https://avatars.githubusercontent.com/u/14387?v=3","gravatar_id":"","url":"https://api.github.com/users/martint","html_url":"https://github.com/martint","followers_url":"https://api.github.com/users/martint/followers","following_url":"https://api.github.com/users/martint/following{/other_user}","gists_url":"https://api.github.com/users/martint/gists{/gist_id}","starred_url":"https://api.github.com/users/martint/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/martint/subscriptions","organizations_url":"https://api.github.com/users/martint/orgs","repos_url":"https://api.github.com/users/martint/repos","events_url":"https://api.github.com/users/martint/events{/privacy}","received_events_url":"https://api.github.com/users/martint/received_events","type":"User","site_admin":false},"body":"This is a rewrite of the partial aggregation pushdown\r\noptimizer to make the code easier to follow and reason\r\nabout.\r\n\r\nThe approach is as follows:\r\n1. Determine whether the optimization is applicable.\r\n   At a minimum, there must be an aggregation on top\r\n   of an exchange.\r\n2. If the aggregation is SINGLE, split it into a FINAL\r\n   on top of a PARTIAL and reprocess the resulting plan.\r\n3. If the aggregation is a PARTIAL, push it underneath\r\n   each branch of the exchange.\r\n\r\nWe use a couple of tricks to avoid having to juggle\r\nand rename field names as the nodes are rewired:\r\n\r\n1. When pushing the partial aggregation through the exchange,\r\n   the names of the outputs of the aggregation are preserved.\r\n2. If the input->output mappings in the exchange are not\r\n   simple identity projections without rename, we introduce\r\n   a projection under the partial aggregation. This helps\r\n   avoid having to rewrite all the aggregation functions\r\n   to refer to new names.\r\n\r\nIt also fixes a planning issue under certain scenarios\r\ninvolving aggregation subqueries and partitioned tables.\r\n\r\nE.g.,\r\n\r\n    SELECT *\r\n    FROM (\r\n        SELECT count(*)\r\n        FROM tpch.tiny.orders\r\n        HAVING count(DISTINCT custkey) > 1\r\n    )\r\n    CROSS JOIN t\r\n\r\nwhere \"t\" is a partitioned Hive table.\r\n\r\nFixes https://github.com/prestodb/presto/issues/6554","created_at":"2016-11-09T02:20:12Z","updated_at":"2016-11-09T19:27:44Z","closed_at":null,"merged_at":null,"merge_commit_sha":"79a41c4297d74a3c7cedb9724e2b3bf0985f86c2","assignee":{"login":"erichwang","id":737851,"avatar_url":"https://avatars.githubusercontent.com/u/737851?v=3","gravatar_id":"","url":"https://api.github.com/users/erichwang","html_url":"https://github.com/erichwang","followers_url":"https://api.github.com/users/erichwang/followers","following_url":"https://api.github.com/users/erichwang/following{/other_user}","gists_url":"https://api.github.com/users/erichwang/gists{/gist_id}","starred_url":"https://api.github.com/users/erichwang/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/erichwang/subscriptions","organizations_url":"https://api.github.com/users/erichwang/orgs","repos_url":"https://api.github.com/users/erichwang/repos","events_url":"https://api.github.com/users/erichwang/events{/privacy}","received_events_url":"https://api.github.com/users/erichwang/received_events","type":"User","site_admin":false},"assignees":[{"login":"erichwang","id":737851,"avatar_url":"https://avatars.githubusercontent.com/u/737851?v=3","gravatar_id":"","url":"https://api.github.com/users/erichwang","html_url":"https://github.com/erichwang","followers_url":"https://api.github.com/users/erichwang/followers","following_url":"https://api.github.com/users/erichwang/following{/other_user}","gists_url":"https://api.github.com/users/erichwang/gists{/gist_id}","starred_url":"https://api.github.com/users/erichwang/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/erichwang/subscriptions","organizations_url":"https://api.github.com/users/erichwang/orgs","repos_url":"https://api.github.com/users/erichwang/repos","events_url":"https://api.github.com/users/erichwang/events{/privacy}","received_events_url":"https://api.github.com/users/erichwang/received_events","type":"User","site_admin":false}],"milestone":null,"commits_url":"https://api.github.com/repos/prestodb/presto/pulls/6571/commits","review_comments_url":"https://api.github.com/repos/prestodb/presto/pulls/6571/comments","review_comment_url":"https://api.github.com/repos/prestodb/presto/pulls/comments{/number}","comments_url":"https://api.github.com/repos/prestodb/presto/issues/6571/comments","statuses_url":"https://api.github.com/repos/prestodb/presto/statuses/c813a217be63c90ac6859e17df7657ac87fc7d1a","head":{"label":"martint:agg","ref":"agg","sha":"c813a217be63c90ac6859e17df7657ac87fc7d1a","user":{"login":"martint","id":14387,"avatar_url":"https://avatars.githubusercontent.com/u/14387?v=3","gravatar_id":"","url":"https://api.github.com/users/martint","html_url":"https://github.com/martint","followers_url":"https://api.github.com/users/martint/followers","following_url":"https://api.github.com/users/martint/following{/other_user}","gists_url":"https://api.github.com/users/martint/gists{/gist_id}","starred_url":"https://api.github.com/users/martint/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/martint/subscriptions","organizations_url":"https://api.github.com/users/martint/orgs","repos_url":"https://api.github.com/users/martint/repos","events_url":"https://api.github.com/users/martint/events{/privacy}","received_events_url":"https://api.github.com/users/martint/received_events","type":"User","site_admin":false},"repo":{"id":5498382,"name":"presto","full_name":"martint/presto","owner":{"login":"martint","id":14387,"avatar_url":"https://avatars.githubusercontent.com/u/14387?v=3","gravatar_id":"","url":"https://api.github.com/users/martint","html_url":"https://github.com/martint","followers_url":"https://api.github.com/users/martint/followers","following_url":"https://api.github.com/users/martint/following{/other_user}","gists_url":"https://api.github.com/users/martint/gists{/gist_id}","starred_url":"https://api.github.com/users/martint/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/martint/subscriptions","organizations_url":"https://api.github.com/users/martint/orgs","repos_url":"https://api.github.com/users/martint/repos","events_url":"https://api.github.com/users/martint/events{/privacy}","received_events_url":"https://api.github.com/users/martint/received_events","type":"User","site_admin":false},"private":false,"html_url":"https://github.com/martint/presto","description":null,"fork":true,"url":"https://api.github.com/repos/martint/presto","forks_url":"https://api.github.com/repos/martint/presto/forks","keys_url":"https://api.github.com/repos/martint/presto/keys{/key_id}","collaborators_url":"https://api.github.com/repos/martint/presto/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/martint/presto/teams","hooks_url":"https://api.github.com/repos/martint/presto/hooks","issue_events_url":"https://api.github.com/repos/martint/presto/issues/events{/number}","events_url":"https://api.github.com/repos/martint/presto/events","assignees_url":"https://api.github.com/repos/martint/presto/assignees{/user}","branches_url":"https://api.github.com/repos/martint/presto/branches{/branch}","tags_url":"https://api.github.com/repos/martint/presto/tags","blobs_url":"https://api.github.com/repos/martint/presto/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/martint/presto/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/martint/presto/git/refs{/sha}","trees_url":"https://api.github.com/repos/martint/presto/git/trees{/sha}","statuses_url":"https://api.github.com/repos/martint/presto/statuses/{sha}","languages_url":"https://api.github.com/repos/martint/presto/languages","stargazers_url":"https://api.github.com/repos/martint/presto/stargazers","contributors_url":"https://api.github.com/repos/martint/presto/contributors","subscribers_url":"https://api.github.com/repos/martint/presto/subscribers","subscription_url":"https://api.github.com/repos/martint/presto/subscription","commits_url":"https://api.github.com/repos/martint/presto/commits{/sha}","git_commits_url":"https://api.github.com/repos/martint/presto/git/commits{/sha}","comments_url":"https://api.github.com/repos/martint/presto/comments{/number}","issue_comment_url":"https://api.github.com/repos/martint/presto/issues/comments{/number}","contents_url":"https://api.github.com/repos/martint/presto/contents/{+path}","compare_url":"https://api.github.com/repos/martint/presto/compare/{base}...{head}","merges_url":"https://api.github.com/repos/martint/presto/merges","archive_url":"https://api.github.com/repos/martint/presto/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/martint/presto/downloads","issues_url":"https://api.github.com/repos/martint/presto/issues{/number}","pulls_url":"https://api.github.com/repos/martint/presto/pulls{/number}","milestones_url":"https://api.github.com/repos/martint/presto/milestones{/number}","notifications_url":"https://api.github.com/repos/martint/presto/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/martint/presto/labels{/name}","releases_url":"https://api.github.com/repos/martint/presto/releases{/id}","deployments_url":"https://api.github.com/repos/martint/presto/deployments","created_at":"2012-08-21T17:22:50Z","updated_at":"2016-03-18T00:24:35Z","pushed_at":"2016-11-09T06:35:02Z","git_url":"git://github.com/martint/presto.git","ssh_url":"git@github.com:martint/presto.git","clone_url":"https://github.com/martint/presto.git","svn_url":"https://github.com/martint/presto","homepage":null,"size":63668,"stargazers_count":0,"watchers_count":0,"language":"Java","has_issues":false,"has_downloads":true,"has_wiki":true,"has_pages":true,"forks_count":1,"mirror_url":null,"open_issues_count":0,"forks":1,"open_issues":0,"watchers":0,"default_branch":"master"}},"base":{"label":"prestodb:master","ref":"master","sha":"4193710b61ab7f16340ec6ea777c0f1a91127207","user":{"login":"prestodb","id":6882181,"avatar_url":"https://avatars.githubusercontent.com/u/6882181?v=3","gravatar_id":"","url":"https://api.github.com/users/prestodb","html_url":"https://github.com/prestodb","followers_url":"https://api.github.com/users/prestodb/followers","following_url":"https://api.github.com/users/prestodb/following{/other_user}","gists_url":"https://api.github.com/users/prestodb/gists{/gist_id}","starred_url":"https://api.github.com/users/prestodb/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/prestodb/subscriptions","organizations_url":"https://api.github.com/users/prestodb/orgs","repos_url":"https://api.github.com/users/prestodb/repos","events_url":"https://api.github.com/users/prestodb/events{/privacy}","received_events_url":"https://api.github.com/users/prestodb/received_events","type":"Organization","site_admin":false},"repo":{"id":5349565,"name":"presto","full_name":"prestodb/presto","owner":{"login":"prestodb","id":6882181,"avatar_url":"https://avatars.githubusercontent.com/u/6882181?v=3","gravatar_id":"","url":"https://api.github.com/users/prestodb","html_url":"https://github.com/prestodb","followers_url":"https://api.github.com/users/prestodb/followers","following_url":"https://api.github.com/users/prestodb/following{/other_user}","gists_url":"https://api.github.com/users/prestodb/gists{/gist_id}","starred_url":"https://api.github.com/users/prestodb/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/prestodb/subscriptions","organizations_url":"https://api.github.com/users/prestodb/orgs","repos_url":"https://api.github.com/users/prestodb/repos","events_url":"https://api.github.com/users/prestodb/events{/privacy}","received_events_url":"https://api.github.com/users/prestodb/received_events","type":"Organization","site_admin":false},"private":false,"html_url":"https://github.com/prestodb/presto","description":"Distributed SQL query engine for big data","fork":false,"url":"https://api.github.com/repos/prestodb/presto","forks_url":"https://api.github.com/repos/prestodb/presto/forks","keys_url":"https://api.github.com/repos/prestodb/presto/keys{/key_id}","collaborators_url":"https://api.github.com/repos/prestodb/presto/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/prestodb/presto/teams","hooks_url":"https://api.github.com/repos/prestodb/presto/hooks","issue_events_url":"https://api.github.com/repos/prestodb/presto/issues/events{/number}","events_url":"https://api.github.com/repos/prestodb/presto/events","assignees_url":"https://api.github.com/repos/prestodb/presto/assignees{/user}","branches_url":"https://api.github.com/repos/prestodb/presto/branches{/branch}","tags_url":"https://api.github.com/repos/prestodb/presto/tags","blobs_url":"https://api.github.com/repos/prestodb/presto/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/prestodb/presto/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/prestodb/presto/git/refs{/sha}","trees_url":"https://api.github.com/repos/prestodb/presto/git/trees{/sha}","statuses_url":"https://api.github.com/repos/prestodb/presto/statuses/{sha}","languages_url":"https://api.github.com/repos/prestodb/presto/languages","stargazers_url":"https://api.github.com/repos/prestodb/presto/stargazers","contributors_url":"https://api.github.com/repos/prestodb/presto/contributors","subscribers_url":"https://api.github.com/repos/prestodb/presto/subscribers","subscription_url":"https://api.github.com/repos/prestodb/presto/subscription","commits_url":"https://api.github.com/repos/prestodb/presto/commits{/sha}","git_commits_url":"https://api.github.com/repos/prestodb/presto/git/commits{/sha}","comments_url":"https://api.github.com/repos/prestodb/presto/comments{/number}","issue_comment_url":"https://api.github.com/repos/prestodb/presto/issues/comments{/number}","contents_url":"https://api.github.com/repos/prestodb/presto/contents/{+path}","compare_url":"https://api.github.com/repos/prestodb/presto/compare/{base}...{head}","merges_url":"https://api.github.com/repos/prestodb/presto/merges","archive_url":"https://api.github.com/repos/prestodb/presto/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/prestodb/presto/downloads","issues_url":"https://api.github.com/repos/prestodb/presto/issues{/number}","pulls_url":"https://api.github.com/repos/prestodb/presto/pulls{/number}","milestones_url":"https://api.github.com/repos/prestodb/presto/milestones{/number}","notifications_url":"https://api.github.com/repos/prestodb/presto/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/prestodb/presto/labels{/name}","releases_url":"https://api.github.com/repos/prestodb/presto/releases{/id}","deployments_url":"https://api.github.com/repos/prestodb/presto/deployments","created_at":"2012-08-09T01:03:37Z","updated_at":"2016-11-09T09:33:44Z","pushed_at":"2016-11-09T19:26:17Z","git_url":"git://github.com/prestodb/presto.git","ssh_url":"git@github.com:prestodb/presto.git","clone_url":"https://github.com/prestodb/presto.git","svn_url":"https://github.com/prestodb/presto","homepage":"https://prestodb.io","size":171033,"stargazers_count":5272,"watchers_count":5272,"language":"Java","has_issues":true,"has_downloads":true,"has_wiki":true,"has_pages":false,"forks_count":1645,"mirror_url":null,"open_issues_count":728,"forks":1645,"open_issues":728,"watchers":5272,"default_branch":"master"}},"_links":{"self":{"href":"https://api.github.com/repos/prestodb/presto/pulls/6571"},"html":{"href":"https://github.com/prestodb/presto/pull/6571"},"issue":{"href":"https://api.github.com/repos/prestodb/presto/issues/6571"},"comments":{"href":"https://api.github.com/repos/prestodb/presto/issues/6571/comments"},"review_comments":{"href":"https://api.github.com/repos/prestodb/presto/pulls/6571/comments"},"review_comment":{"href":"https://api.github.com/repos/prestodb/presto/pulls/comments{/number}"},"commits":{"href":"https://api.github.com/repos/prestodb/presto/pulls/6571/commits"},"statuses":{"href":"https://api.github.com/repos/prestodb/presto/statuses/c813a217be63c90ac6859e17df7657ac87fc7d1a"}}}}
{"actor":{"display_login":"dain"}}