{"action":"created","issue":{"url":"https://api.github.com/repos/cockroachdb/cockroach/issues/22297","repository_url":"https://api.github.com/repos/cockroachdb/cockroach","labels_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/22297/labels{/name}","comments_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/22297/comments","events_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/22297/events","html_url":"https://github.com/cockroachdb/cockroach/pull/22297","id":293594904,"number":22297,"title":"opt: Support building a memo with aggregates and GROUP BY","user":{"login":"rytaft","id":3288732,"avatar_url":"https://avatars2.githubusercontent.com/u/3288732?v=4","gravatar_id":"","url":"https://api.github.com/users/rytaft","html_url":"https://github.com/rytaft","followers_url":"https://api.github.com/users/rytaft/followers","following_url":"https://api.github.com/users/rytaft/following{/other_user}","gists_url":"https://api.github.com/users/rytaft/gists{/gist_id}","starred_url":"https://api.github.com/users/rytaft/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/rytaft/subscriptions","organizations_url":"https://api.github.com/users/rytaft/orgs","repos_url":"https://api.github.com/users/rytaft/repos","events_url":"https://api.github.com/users/rytaft/events{/privacy}","received_events_url":"https://api.github.com/users/rytaft/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":4,"created_at":"2018-02-01T16:26:41Z","updated_at":"2018-02-02T01:51:33Z","closed_at":null,"author_association":"CONTRIBUTOR","pull_request":{"url":"https://api.github.com/repos/cockroachdb/cockroach/pulls/22297","html_url":"https://github.com/cockroachdb/cockroach/pull/22297","diff_url":"https://github.com/cockroachdb/cockroach/pull/22297.diff","patch_url":"https://github.com/cockroachdb/cockroach/pull/22297.patch"},"body":"This is the second commit of several needed to build a \"memo\"\r\nstructure from a parsed SQL query. The memo is the primary data\r\nstructure in the query optimizer, used for maintaining the forest of\r\nquery plan trees that the optimizer considers as part of its search\r\nfor the optimal plan.\r\n\r\nThis commit includes support for building a memo for queries\r\nwhich include aggregates or a `GROUP BY` clause. The scoping rules\r\nfor grouping are complex, so the complexity in this commit is mostly\r\nfor the purpose of enforcing these rules. The test cases have been\r\nadapted from the aggregate test in logictests.\r\n\r\nFuture commits will add support for `DISTINCT` and `HAVING`, as well\r\nas `VALUES`, `UNION` and complex joins.\r\n\r\nThis code is based on code originally written by @petermattis and\r\n@andy-kimball in the opttoy project.\r\n\r\nRelease note: None"},"comment":{"url":"https://api.github.com/repos/cockroachdb/cockroach/issues/comments/362461102","html_url":"https://github.com/cockroachdb/cockroach/pull/22297#issuecomment-362461102","issue_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/22297","id":362461102,"user":{"login":"petermattis","id":6201335,"avatar_url":"https://avatars0.githubusercontent.com/u/6201335?v=4","gravatar_id":"","url":"https://api.github.com/users/petermattis","html_url":"https://github.com/petermattis","followers_url":"https://api.github.com/users/petermattis/followers","following_url":"https://api.github.com/users/petermattis/following{/other_user}","gists_url":"https://api.github.com/users/petermattis/gists{/gist_id}","starred_url":"https://api.github.com/users/petermattis/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/petermattis/subscriptions","organizations_url":"https://api.github.com/users/petermattis/orgs","repos_url":"https://api.github.com/users/petermattis/repos","events_url":"https://api.github.com/users/petermattis/events{/privacy}","received_events_url":"https://api.github.com/users/petermattis/received_events","type":"User","site_admin":false},"created_at":"2018-02-02T01:51:33Z","updated_at":"2018-02-02T01:51:33Z","author_association":"CONTRIBUTOR","body":"\n\n\n\nReview status: 0 of 8 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.\n\n---\n\n*[pkg/sql/opt/build/scope.go, line 93 at r1](https://reviewable.io:443/reviews/cockroachdb/cockroach/22297#-L4IOGTC7Af4Pcal3jSs:-L4JC2tJUh1uG--UIhcS:b-cwht4) ([raw file](https://github.com/cockroachdb/cockroach/blob/21b741752f693a9458479541bb3ab3d5dacd6084/pkg/sql/opt/build/scope.go#L93)):*\n<details><summary><i>Previously, rytaft wrote…</i></summary><blockquote>\n\nIt changes the semantics for `GROUP BY`, apparently. \n\nFor example, `SELECT COUNT(*) FROM t.kv GROUP BY 1+2` is different from `SELECT COUNT(*) FROM t.kv GROUP BY 3`. The first query is grouping on the constant expression \"1+2\", while the second one is grouping on the third column in the select list (it throws an error since there is only one column).\n</blockquote></details>\n\nDoes current Cockroach get this wrong? I worry that not doing the constant folding will change our typing behavior as constant folding is an integral part of our type inference. Let's expand this comment to capture your reasoning here and add a TODO as I'm pretty sure we'll need to revisit this. Cc @nvanbenschoten and @knz. \n\n---\n\n\n*Comments from [Reviewable](https://reviewable.io:443/reviews/cockroachdb/cockroach/22297)*\n<!-- Sent from Reviewable.io -->\n"}}
{"actor":{"display_login":"petermattis"}}