{"action":"created","comment":{"url":"https://api.github.com/repos/apache/calcite/pulls/comments/55896699","id":55896699,"diff_hunk":"@@ -158,11 +160,66 @@ RelSubset getOrCreateSubset(\n       final VolcanoPlanner planner =\n           (VolcanoPlanner) cluster.getPlanner();\n \n+      // Converters from newly introduced subset to all the remaining one, only if possible to\n+      // convert.  No point adding converters if it is not possible.\n+      for (RelSubset other : subsets) {\n+        if (other.getConvention() == Convention.NONE) {\n+          continue;\n+        }\n+\n+        assert other.getTraitSet().size() == subset.getTraitSet().size();\n+\n+        final ImmutableList<RelTrait> otherTrait =\n+                other.getTraitSet().difference(subset.getTraitSet());\n+        final ImmutableList<RelTrait> thisTrait =\n+                subset.getTraitSet().difference(other.getTraitSet());\n+        assert otherTrait.size() == thisTrait.size();\n+\n+        if (otherTrait.size() == 1 && thisTrait.size() == 1) {\n+          RelTraitDef traitDef = otherTrait.get(0).getTraitDef();\n+          boolean canConvert = traitDef.canConvert(\n+                  cluster.getPlanner(), thisTrait.get(0), otherTrait.get(0));\n+\n+          if (canConvert) {\n+            final AbstractConverter converter =\n+                    new AbstractConverter(cluster, subset, traitDef, other.getTraitSet());\n+            planner.register(converter, other);\n+          }\n+        }\n+      }\n+\n       subsets.add(subset);\n \n-      if (planner.root != null\n-          && planner.root.set == this) {\n-        planner.ensureRootConverters();\n+      // Converters from newly introduced subset to all the remaining one, only if possible to\n+      // convert.  No point adding converters if it is not possible.\n+      for (RelSubset other : subsets) {\n+        if (other == subset) {\n+          continue;\n+        }\n+\n+        if (other.getConvention() == Convention.NONE) {","path":"core/src/main/java/org/apache/calcite/plan/volcano/RelSet.java","position":57,"original_position":57,"commit_id":"8af445027f798647b60003915d60dde678f4389d","original_commit_id":"8af445027f798647b60003915d60dde678f4389d","user":{"login":"jacques-n","id":183350,"avatar_url":"https://avatars.githubusercontent.com/u/183350?v=3","gravatar_id":"","url":"https://api.github.com/users/jacques-n","html_url":"https://github.com/jacques-n","followers_url":"https://api.github.com/users/jacques-n/followers","following_url":"https://api.github.com/users/jacques-n/following{/other_user}","gists_url":"https://api.github.com/users/jacques-n/gists{/gist_id}","starred_url":"https://api.github.com/users/jacques-n/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/jacques-n/subscriptions","organizations_url":"https://api.github.com/users/jacques-n/orgs","repos_url":"https://api.github.com/users/jacques-n/repos","events_url":"https://api.github.com/users/jacques-n/events{/privacy}","received_events_url":"https://api.github.com/users/jacques-n/received_events","type":"User","site_admin":false},"body":"This doesn't seem necessary since the ConventionTraitDef finds only existing ConverterRule paths and thus should fail canConvert below. Note that this wasn't previously the case so with that fix, this can probably be removed.","created_at":"2016-03-11T22:13:33Z","updated_at":"2016-03-11T22:13:33Z","html_url":"https://github.com/apache/calcite/pull/210#discussion_r55896699","pull_request_url":"https://api.github.com/repos/apache/calcite/pulls/210","_links":{"self":{"href":"https://api.github.com/repos/apache/calcite/pulls/comments/55896699"},"html":{"href":"https://github.com/apache/calcite/pull/210#discussion_r55896699"},"pull_request":{"href":"https://api.github.com/repos/apache/calcite/pulls/210"}}},"pull_request":{"url":"https://api.github.com/repos/apache/calcite/pulls/210","id":62605160,"html_url":"https://github.com/apache/calcite/pull/210","diff_url":"https://github.com/apache/calcite/pull/210.diff","patch_url":"https://github.com/apache/calcite/pull/210.patch","issue_url":"https://api.github.com/repos/apache/calcite/issues/210","number":210,"state":"open","locked":false,"title":"Fix RelTrait conversion (e.g. distribution, collation), new tests","user":{"login":"minji-kim","id":9218757,"avatar_url":"https://avatars.githubusercontent.com/u/9218757?v=3","gravatar_id":"","url":"https://api.github.com/users/minji-kim","html_url":"https://github.com/minji-kim","followers_url":"https://api.github.com/users/minji-kim/followers","following_url":"https://api.github.com/users/minji-kim/following{/other_user}","gists_url":"https://api.github.com/users/minji-kim/gists{/gist_id}","starred_url":"https://api.github.com/users/minji-kim/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/minji-kim/subscriptions","organizations_url":"https://api.github.com/users/minji-kim/orgs","repos_url":"https://api.github.com/users/minji-kim/repos","events_url":"https://api.github.com/users/minji-kim/events{/privacy}","received_events_url":"https://api.github.com/users/minji-kim/received_events","type":"User","site_admin":false},"body":"Summary of the fixes.\r\n1) In RelSet.getOrCreateSubset(), only add AbstractConverters if RelTraitDef can convert.  This means that the difference should be of size 1 (i.e. only one trait is different as RelTraitDef defines canConvert() and convert() on only one RelTrait), and RelTraitDef.canConvert() should return true.  This cuts back on the number of AbstractConverters added signifincantly if RelTraitDef.canConvert() is written properly.\r\n\r\n2) In RelCollationTraitDef, canConvert() should really only return true if there is a need for conversion.  If the fromTrait already satisfies toTrait, there is no need to change it.  Perhaps \"canConvert()\" should be named \"shouldConvert()\", but I left the name as it is.  Without this change, many of the JdbcTests (joins) were taking a lot longer to plan or go on infinite-planning-state.\r\n\r\n3)  Remaining changes are test changes.","created_at":"2016-03-11T21:28:28Z","updated_at":"2016-03-11T22:13:33Z","closed_at":null,"merged_at":null,"merge_commit_sha":"d83714a98669373f1a190dbaf7ee7ad664570e89","assignee":null,"milestone":null,"commits_url":"https://api.github.com/repos/apache/calcite/pulls/210/commits","review_comments_url":"https://api.github.com/repos/apache/calcite/pulls/210/comments","review_comment_url":"https://api.github.com/repos/apache/calcite/pulls/comments{/number}","comments_url":"https://api.github.com/repos/apache/calcite/issues/210/comments","statuses_url":"https://api.github.com/repos/apache/calcite/statuses/8af445027f798647b60003915d60dde678f4389d","head":{"label":"minji-kim:CALCITE-1148","ref":"CALCITE-1148","sha":"8af445027f798647b60003915d60dde678f4389d","user":{"login":"minji-kim","id":9218757,"avatar_url":"https://avatars.githubusercontent.com/u/9218757?v=3","gravatar_id":"","url":"https://api.github.com/users/minji-kim","html_url":"https://github.com/minji-kim","followers_url":"https://api.github.com/users/minji-kim/followers","following_url":"https://api.github.com/users/minji-kim/following{/other_user}","gists_url":"https://api.github.com/users/minji-kim/gists{/gist_id}","starred_url":"https://api.github.com/users/minji-kim/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/minji-kim/subscriptions","organizations_url":"https://api.github.com/users/minji-kim/orgs","repos_url":"https://api.github.com/users/minji-kim/repos","events_url":"https://api.github.com/users/minji-kim/events{/privacy}","received_events_url":"https://api.github.com/users/minji-kim/received_events","type":"User","site_admin":false},"repo":{"id":52418526,"name":"calcite","full_name":"minji-kim/calcite","owner":{"login":"minji-kim","id":9218757,"avatar_url":"https://avatars.githubusercontent.com/u/9218757?v=3","gravatar_id":"","url":"https://api.github.com/users/minji-kim","html_url":"https://github.com/minji-kim","followers_url":"https://api.github.com/users/minji-kim/followers","following_url":"https://api.github.com/users/minji-kim/following{/other_user}","gists_url":"https://api.github.com/users/minji-kim/gists{/gist_id}","starred_url":"https://api.github.com/users/minji-kim/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/minji-kim/subscriptions","organizations_url":"https://api.github.com/users/minji-kim/orgs","repos_url":"https://api.github.com/users/minji-kim/repos","events_url":"https://api.github.com/users/minji-kim/events{/privacy}","received_events_url":"https://api.github.com/users/minji-kim/received_events","type":"User","site_admin":false},"private":false,"html_url":"https://github.com/minji-kim/calcite","description":"Mirror of Apache Calcite","fork":true,"url":"https://api.github.com/repos/minji-kim/calcite","forks_url":"https://api.github.com/repos/minji-kim/calcite/forks","keys_url":"https://api.github.com/repos/minji-kim/calcite/keys{/key_id}","collaborators_url":"https://api.github.com/repos/minji-kim/calcite/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/minji-kim/calcite/teams","hooks_url":"https://api.github.com/repos/minji-kim/calcite/hooks","issue_events_url":"https://api.github.com/repos/minji-kim/calcite/issues/events{/number}","events_url":"https://api.github.com/repos/minji-kim/calcite/events","assignees_url":"https://api.github.com/repos/minji-kim/calcite/assignees{/user}","branches_url":"https://api.github.com/repos/minji-kim/calcite/branches{/branch}","tags_url":"https://api.github.com/repos/minji-kim/calcite/tags","blobs_url":"https://api.github.com/repos/minji-kim/calcite/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/minji-kim/calcite/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/minji-kim/calcite/git/refs{/sha}","trees_url":"https://api.github.com/repos/minji-kim/calcite/git/trees{/sha}","statuses_url":"https://api.github.com/repos/minji-kim/calcite/statuses/{sha}","languages_url":"https://api.github.com/repos/minji-kim/calcite/languages","stargazers_url":"https://api.github.com/repos/minji-kim/calcite/stargazers","contributors_url":"https://api.github.com/repos/minji-kim/calcite/contributors","subscribers_url":"https://api.github.com/repos/minji-kim/calcite/subscribers","subscription_url":"https://api.github.com/repos/minji-kim/calcite/subscription","commits_url":"https://api.github.com/repos/minji-kim/calcite/commits{/sha}","git_commits_url":"https://api.github.com/repos/minji-kim/calcite/git/commits{/sha}","comments_url":"https://api.github.com/repos/minji-kim/calcite/comments{/number}","issue_comment_url":"https://api.github.com/repos/minji-kim/calcite/issues/comments{/number}","contents_url":"https://api.github.com/repos/minji-kim/calcite/contents/{+path}","compare_url":"https://api.github.com/repos/minji-kim/calcite/compare/{base}...{head}","merges_url":"https://api.github.com/repos/minji-kim/calcite/merges","archive_url":"https://api.github.com/repos/minji-kim/calcite/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/minji-kim/calcite/downloads","issues_url":"https://api.github.com/repos/minji-kim/calcite/issues{/number}","pulls_url":"https://api.github.com/repos/minji-kim/calcite/pulls{/number}","milestones_url":"https://api.github.com/repos/minji-kim/calcite/milestones{/number}","notifications_url":"https://api.github.com/repos/minji-kim/calcite/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/minji-kim/calcite/labels{/name}","releases_url":"https://api.github.com/repos/minji-kim/calcite/releases{/id}","deployments_url":"https://api.github.com/repos/minji-kim/calcite/deployments","created_at":"2016-02-24T06:02:48Z","updated_at":"2016-02-24T06:02:53Z","pushed_at":"2016-03-11T21:22:46Z","git_url":"git://github.com/minji-kim/calcite.git","ssh_url":"git@github.com:minji-kim/calcite.git","clone_url":"https://github.com/minji-kim/calcite.git","svn_url":"https://github.com/minji-kim/calcite","homepage":"","size":24960,"stargazers_count":0,"watchers_count":0,"language":"Java","has_issues":false,"has_downloads":true,"has_wiki":false,"has_pages":false,"forks_count":0,"mirror_url":null,"open_issues_count":0,"forks":0,"open_issues":0,"watchers":0,"default_branch":"master"}},"base":{"label":"apache:master","ref":"master","sha":"b8d271b94e7b855e8b4bf1bd355248b4fe62e68a","user":{"login":"apache","id":47359,"avatar_url":"https://avatars.githubusercontent.com/u/47359?v=3","gravatar_id":"","url":"https://api.github.com/users/apache","html_url":"https://github.com/apache","followers_url":"https://api.github.com/users/apache/followers","following_url":"https://api.github.com/users/apache/following{/other_user}","gists_url":"https://api.github.com/users/apache/gists{/gist_id}","starred_url":"https://api.github.com/users/apache/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/apache/subscriptions","organizations_url":"https://api.github.com/users/apache/orgs","repos_url":"https://api.github.com/users/apache/repos","events_url":"https://api.github.com/users/apache/events{/privacy}","received_events_url":"https://api.github.com/users/apache/received_events","type":"Organization","site_admin":false},"repo":{"id":21193524,"name":"calcite","full_name":"apache/calcite","owner":{"login":"apache","id":47359,"avatar_url":"https://avatars.githubusercontent.com/u/47359?v=3","gravatar_id":"","url":"https://api.github.com/users/apache","html_url":"https://github.com/apache","followers_url":"https://api.github.com/users/apache/followers","following_url":"https://api.github.com/users/apache/following{/other_user}","gists_url":"https://api.github.com/users/apache/gists{/gist_id}","starred_url":"https://api.github.com/users/apache/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/apache/subscriptions","organizations_url":"https://api.github.com/users/apache/orgs","repos_url":"https://api.github.com/users/apache/repos","events_url":"https://api.github.com/users/apache/events{/privacy}","received_events_url":"https://api.github.com/users/apache/received_events","type":"Organization","site_admin":false},"private":false,"html_url":"https://github.com/apache/calcite","description":"Mirror of Apache Calcite","fork":false,"url":"https://api.github.com/repos/apache/calcite","forks_url":"https://api.github.com/repos/apache/calcite/forks","keys_url":"https://api.github.com/repos/apache/calcite/keys{/key_id}","collaborators_url":"https://api.github.com/repos/apache/calcite/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/apache/calcite/teams","hooks_url":"https://api.github.com/repos/apache/calcite/hooks","issue_events_url":"https://api.github.com/repos/apache/calcite/issues/events{/number}","events_url":"https://api.github.com/repos/apache/calcite/events","assignees_url":"https://api.github.com/repos/apache/calcite/assignees{/user}","branches_url":"https://api.github.com/repos/apache/calcite/branches{/branch}","tags_url":"https://api.github.com/repos/apache/calcite/tags","blobs_url":"https://api.github.com/repos/apache/calcite/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/apache/calcite/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/apache/calcite/git/refs{/sha}","trees_url":"https://api.github.com/repos/apache/calcite/git/trees{/sha}","statuses_url":"https://api.github.com/repos/apache/calcite/statuses/{sha}","languages_url":"https://api.github.com/repos/apache/calcite/languages","stargazers_url":"https://api.github.com/repos/apache/calcite/stargazers","contributors_url":"https://api.github.com/repos/apache/calcite/contributors","subscribers_url":"https://api.github.com/repos/apache/calcite/subscribers","subscription_url":"https://api.github.com/repos/apache/calcite/subscription","commits_url":"https://api.github.com/repos/apache/calcite/commits{/sha}","git_commits_url":"https://api.github.com/repos/apache/calcite/git/commits{/sha}","comments_url":"https://api.github.com/repos/apache/calcite/comments{/number}","issue_comment_url":"https://api.github.com/repos/apache/calcite/issues/comments{/number}","contents_url":"https://api.github.com/repos/apache/calcite/contents/{+path}","compare_url":"https://api.github.com/repos/apache/calcite/compare/{base}...{head}","merges_url":"https://api.github.com/repos/apache/calcite/merges","archive_url":"https://api.github.com/repos/apache/calcite/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/apache/calcite/downloads","issues_url":"https://api.github.com/repos/apache/calcite/issues{/number}","pulls_url":"https://api.github.com/repos/apache/calcite/pulls{/number}","milestones_url":"https://api.github.com/repos/apache/calcite/milestones{/number}","notifications_url":"https://api.github.com/repos/apache/calcite/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/apache/calcite/labels{/name}","releases_url":"https://api.github.com/repos/apache/calcite/releases{/id}","deployments_url":"https://api.github.com/repos/apache/calcite/deployments","created_at":"2014-06-25T07:00:07Z","updated_at":"2016-03-08T08:28:48Z","pushed_at":"2016-03-11T21:28:28Z","git_url":"git://github.com/apache/calcite.git","ssh_url":"git@github.com:apache/calcite.git","clone_url":"https://github.com/apache/calcite.git","svn_url":"https://github.com/apache/calcite","homepage":"","size":28240,"stargazers_count":185,"watchers_count":185,"language":"Java","has_issues":false,"has_downloads":true,"has_wiki":false,"has_pages":false,"forks_count":159,"mirror_url":"git://git.apache.org/calcite.git","open_issues_count":11,"forks":159,"open_issues":11,"watchers":185,"default_branch":"master"}},"_links":{"self":{"href":"https://api.github.com/repos/apache/calcite/pulls/210"},"html":{"href":"https://github.com/apache/calcite/pull/210"},"issue":{"href":"https://api.github.com/repos/apache/calcite/issues/210"},"comments":{"href":"https://api.github.com/repos/apache/calcite/issues/210/comments"},"review_comments":{"href":"https://api.github.com/repos/apache/calcite/pulls/210/comments"},"review_comment":{"href":"https://api.github.com/repos/apache/calcite/pulls/comments{/number}"},"commits":{"href":"https://api.github.com/repos/apache/calcite/pulls/210/commits"},"statuses":{"href":"https://api.github.com/repos/apache/calcite/statuses/8af445027f798647b60003915d60dde678f4389d"}}}}