{"action":"created","issue":{"url":"https://api.github.com/repos/cockroachdb/cockroach/issues/9912","repository_url":"https://api.github.com/repos/cockroachdb/cockroach","labels_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/9912/labels{/name}","comments_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/9912/comments","events_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/9912/events","html_url":"https://github.com/cockroachdb/cockroach/pull/9912","id":182565437,"number":9912,"title":"sql: split parser.Datum into Type and Datum","user":{"login":"eisenstatdavid","id":3858570,"avatar_url":"https://avatars.githubusercontent.com/u/3858570?v=3","gravatar_id":"","url":"https://api.github.com/users/eisenstatdavid","html_url":"https://github.com/eisenstatdavid","followers_url":"https://api.github.com/users/eisenstatdavid/followers","following_url":"https://api.github.com/users/eisenstatdavid/following{/other_user}","gists_url":"https://api.github.com/users/eisenstatdavid/gists{/gist_id}","starred_url":"https://api.github.com/users/eisenstatdavid/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/eisenstatdavid/subscriptions","organizations_url":"https://api.github.com/users/eisenstatdavid/orgs","repos_url":"https://api.github.com/users/eisenstatdavid/repos","events_url":"https://api.github.com/users/eisenstatdavid/events{/privacy}","received_events_url":"https://api.github.com/users/eisenstatdavid/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":{"login":"knz","id":642886,"avatar_url":"https://avatars.githubusercontent.com/u/642886?v=3","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},"assignees":[{"login":"knz","id":642886,"avatar_url":"https://avatars.githubusercontent.com/u/642886?v=3","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},{"login":"nvanbenschoten","id":5438456,"avatar_url":"https://avatars.githubusercontent.com/u/5438456?v=3","gravatar_id":"","url":"https://api.github.com/users/nvanbenschoten","html_url":"https://github.com/nvanbenschoten","followers_url":"https://api.github.com/users/nvanbenschoten/followers","following_url":"https://api.github.com/users/nvanbenschoten/following{/other_user}","gists_url":"https://api.github.com/users/nvanbenschoten/gists{/gist_id}","starred_url":"https://api.github.com/users/nvanbenschoten/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/nvanbenschoten/subscriptions","organizations_url":"https://api.github.com/users/nvanbenschoten/orgs","repos_url":"https://api.github.com/users/nvanbenschoten/repos","events_url":"https://api.github.com/users/nvanbenschoten/events{/privacy}","received_events_url":"https://api.github.com/users/nvanbenschoten/received_events","type":"User","site_admin":false}],"milestone":null,"comments":11,"created_at":"2016-10-12T16:01:30Z","updated_at":"2016-10-12T18:53:08Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/cockroachdb/cockroach/pulls/9912","html_url":"https://github.com/cockroachdb/cockroach/pull/9912","diff_url":"https://github.com/cockroachdb/cockroach/pull/9912.diff","patch_url":"https://github.com/cockroachdb/cockroach/pull/9912.patch"},"body":"Currently we use Datum to represent both types and values, with naming\r\nconventions that indicate the intended use. Having separate types is\r\nclearer and will aid future changes to the type system.\n\n<!-- Reviewable:start -->\n---\nThis change is [<img src=\"https://reviewable.io/review_button.svg\" height=\"34\" align=\"absmiddle\" alt=\"Reviewable\"/>](https://reviewable.io/reviews/cockroachdb/cockroach/9912)\n<!-- Reviewable:end -->\n"},"comment":{"url":"https://api.github.com/repos/cockroachdb/cockroach/issues/comments/253304342","html_url":"https://github.com/cockroachdb/cockroach/pull/9912#issuecomment-253304342","issue_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/9912","id":253304342,"user":{"login":"eisenstatdavid","id":3858570,"avatar_url":"https://avatars.githubusercontent.com/u/3858570?v=3","gravatar_id":"","url":"https://api.github.com/users/eisenstatdavid","html_url":"https://github.com/eisenstatdavid","followers_url":"https://api.github.com/users/eisenstatdavid/followers","following_url":"https://api.github.com/users/eisenstatdavid/following{/other_user}","gists_url":"https://api.github.com/users/eisenstatdavid/gists{/gist_id}","starred_url":"https://api.github.com/users/eisenstatdavid/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/eisenstatdavid/subscriptions","organizations_url":"https://api.github.com/users/eisenstatdavid/orgs","repos_url":"https://api.github.com/users/eisenstatdavid/repos","events_url":"https://api.github.com/users/eisenstatdavid/events{/privacy}","received_events_url":"https://api.github.com/users/eisenstatdavid/received_events","type":"User","site_admin":false},"created_at":"2016-10-12T18:53:08Z","updated_at":"2016-10-12T18:53:08Z","body":"The non-parametric types (i.e., all except Placeholder and Tuple) are all singletons. I don't want to ban == because it's less expensive than an interface method call and allows cleaner switch statements. We can't do away with .Equal() without interning parametric types, which we could do at the cost of some complication. I improved the documentation about when it's OK to use ==.\n\n---\n\nReview status: 62 of 66 files reviewed at latest revision, 7 unresolved discussions, some commit checks failed.\n\n---\n\n*[sql/create.go, line 1173 at r1](https://reviewable.io:443/reviews/cockroachdb/cockroach/9912#-KTtlMQGZE_5jqCB35kg:-KTuB9iwqih-XdoyFr5H:bbpaa0g) ([raw file](https://github.com/cockroachdb/cockroach/blob/be8ea886cbe355901b22513a55cc0c8ed9420736/sql/create.go#L1173)):*\n<details><summary><i>Previously, knz (kena) wrote…</i></summary>\n> I'd use DNull here, despite what the comment says. Can you try that and see we're not breaking any test?\n</details>\nTwo tests break:\n```\nroot@:26257> SELECT ABS(NULL);\npq: unknown signature for ABS: ABS(NULL)\n```\nI'll file an issue because this seems wrong.\n\n---\n\n*[sql/row_container.go, line 76 at r1](https://reviewable.io:443/reviews/cockroachdb/cockroach/9912#-KTtkWNjUatzcgsR2A0d:-KTuDB7xZ4_7hV_HOawj:b-p05xyq) ([raw file](https://github.com/cockroachdb/cockroach/blob/be8ea886cbe355901b22513a55cc0c8ed9420736/sql/row_container.go#L76)):*\n<details><summary><i>Previously, knz (kena) wrote…</i></summary>\n> I'd make Size() a method of the Type interface, much cleaner.\n</details>\nSince Size() isn't constant for some types, I'm concerned about unintended consequences of this change. I'd rather defer it to a subsequent PR.\n\n---\n\n*[sql/parser/overload.go, line 85 at r1](https://reviewable.io:443/reviews/cockroachdb/cockroach/9912#-KTtiy4QXXe6hSyatvAI:-KTtkL8kpDfMUO_144vY:b-896fix) ([raw file](https://github.com/cockroachdb/cockroach/blob/be8ea886cbe355901b22513a55cc0c8ed9420736/sql/parser/overload.go#L85)):*\n<details><summary><i>Previously, knz (kena) wrote…</i></summary>\n> use `i >  0` for consistency with the rest of the code base.\n</details>\nDone.\n\n---\n\n*[sql/parser/star_datum.go, line 57 at r1](https://reviewable.io:443/reviews/cockroachdb/cockroach/9912#-KTtgq4B7zOXyFNiJUQK:-KTtl1TDlr8atUejmuap:b-4fe64s) ([raw file](https://github.com/cockroachdb/cockroach/blob/be8ea886cbe355901b22513a55cc0c8ed9420736/sql/parser/star_datum.go#L57)):*\n<details><summary><i>Previously, knz (kena) wrote…</i></summary>\n> Could you double check here whether this is the right approach. I think a simple `return DInt(0)` would be valid too, as would an error perhaps (we really should never evaluate a star).\n</details>\nMindlessly transliterated from the original TypeInt.Eval(). I replaced it with the return (the error crashes something; don't want to attempt that refactor in this PR).\n\n---\n\n*[sql/parser/type_check.go, line 35 at r1](https://reviewable.io:443/reviews/cockroachdb/cockroach/9912#-KTtiWLpLix-N1h_MGmH:-KTtlZt2h0YS8C8GBp3P:b5bcmra) ([raw file](https://github.com/cockroachdb/cockroach/blob/be8ea886cbe355901b22513a55cc0c8ed9420736/sql/parser/type_check.go#L35)):*\n<details><summary><i>Previously, knz (kena) wrote…</i></summary>\n> Where do we need this exactly?\n> If it's only for StarDatum perhaps we can remove it.\n</details>\nIt gets used a couple places -- getting a size lower bound for a type is the main one, but there's another one in sql/create.go that I didn't take the time to understand.\n\n---\n\n*[sql/parser/type_check.go, line 33 at r2](https://reviewable.io:443/reviews/cockroachdb/cockroach/9912#-KTtpAXCDkcJkpTQq6-r:-KTuBfaf7aVrHWNqALwh:b-hmfq92) ([raw file](https://github.com/cockroachdb/cockroach/blob/590182b9e9723641ee07c68b2cb7b4e91467b697/sql/parser/type_check.go#L33)):*\n<details><summary><i>Previously, tamird (Tamir Duberstein) wrote…</i></summary>\n> I don't grok this - when are the semantics of this method different from `==`?\n</details>\nParameterized types (tuples and placeholders) instantiated with twice with the same parameters compare `Equal` but not `==`.\n\n---\n\n\n*Comments from [Reviewable](https://reviewable.io:443/reviews/cockroachdb/cockroach/9912#-:-KTuAWK2YKGIpXQnZYwz:bgydz87)*\n<!-- Sent from Reviewable.io -->\n"}}
{"actor":{"display_login":"eisenstatdavid"}}