{"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":6,"created_at":"2018-02-01T16:26:41Z","updated_at":"2018-02-02T03:02:00Z","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/362471875","html_url":"https://github.com/cockroachdb/cockroach/pull/22297#issuecomment-362471875","issue_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/22297","id":362471875,"user":{"login":"knz","id":642886,"avatar_url":"https://avatars3.githubusercontent.com/u/642886?v=4","gravatar_id":"","url":"https://api.github.com/users/knz","html_url":"https://github.com/knz","followers_url":"https://api.github.com/users/knz/followers","following_url":"https://api.github.com/users/knz/following{/other_user}","gists_url":"https://api.github.com/users/knz/gists{/gist_id}","starred_url":"https://api.github.com/users/knz/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/knz/subscriptions","organizations_url":"https://api.github.com/users/knz/orgs","repos_url":"https://api.github.com/users/knz/repos","events_url":"https://api.github.com/users/knz/events{/privacy}","received_events_url":"https://api.github.com/users/knz/received_events","type":"User","site_admin":false},"created_at":"2018-02-02T03:02:00Z","updated_at":"2018-02-02T03:02:00Z","author_association":"OWNER","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://beta.reviewable.io/reviews/cockroachdb/cockroach/22297#-L4IOGTC7Af4Pcal3jSs:-L4JRmiVBUiNIcl8oow8:b8luyat) ([raw file](https://github.com/cockroachdb/cockroach/blob/21b741752f693a9458479541bb3ab3d5dacd6084/pkg/sql/opt/build/scope.go#L93)):*\n<details><summary><i>Previously, petermattis (Peter Mattis) wrote…</i></summary><blockquote>\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</blockquote></details>\n\now\n\nthe handling here is incorrect I believe.\n\nThe test for e.g. GROUP BY 3 must match exactly a literal numeric constant. It's a *lexical* check which must occur before any handling by TypeCheck. In particular `GROUP BY (3)` is not to be handled like `GROUP BY 3` - only the latter of the two gets special treatment.\n\nOnce the code has determined that the lexical structure of the parameter is not a literal numeric constant, `TypeCheck` can be called as for any other expression.\n\nNote that ORDER BY has the same rule, and *also* another even weirder rule that takes place *before* this one. \n\n---\n\n\n*Comments from [Reviewable](https://beta.reviewable.io/reviews/cockroachdb/cockroach/22297)*\n<!-- Sent from Reviewable.io -->\n"}}
{"actor":{"display_login":"knz"}}