{"action":"created","issue":{"url":"https://api.github.com/repos/cockroachdb/cockroach/issues/10584","repository_url":"https://api.github.com/repos/cockroachdb/cockroach","labels_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/10584/labels{/name}","comments_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/10584/comments","events_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/10584/events","html_url":"https://github.com/cockroachdb/cockroach/pull/10584","id":188412882,"number":10584,"title":"WIP: sql: add int[] column type","user":{"login":"cuongdo","id":1564582,"avatar_url":"https://avatars.githubusercontent.com/u/1564582?v=3","gravatar_id":"","url":"https://api.github.com/users/cuongdo","html_url":"https://github.com/cuongdo","followers_url":"https://api.github.com/users/cuongdo/followers","following_url":"https://api.github.com/users/cuongdo/following{/other_user}","gists_url":"https://api.github.com/users/cuongdo/gists{/gist_id}","starred_url":"https://api.github.com/users/cuongdo/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/cuongdo/subscriptions","organizations_url":"https://api.github.com/users/cuongdo/orgs","repos_url":"https://api.github.com/users/cuongdo/repos","events_url":"https://api.github.com/users/cuongdo/events{/privacy}","received_events_url":"https://api.github.com/users/cuongdo/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":9,"created_at":"2016-11-10T03:37:16Z","updated_at":"2016-11-15T19:25:40Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/cockroachdb/cockroach/pulls/10584","html_url":"https://github.com/cockroachdb/cockroach/pull/10584","diff_url":"https://github.com/cockroachdb/cockroach/pull/10584.diff","patch_url":"https://github.com/cockroachdb/cockroach/pull/10584.patch"},"body":"\n\n<!-- Reviewable:start -->\nThis change is [<img src=\"https://reviewable.io/review_button.svg\" height=\"34\" align=\"absmiddle\" alt=\"Reviewable\"/>](https://reviewable.io/reviews/cockroachdb/cockroach/10584)\n<!-- Reviewable:end -->\n"},"comment":{"url":"https://api.github.com/repos/cockroachdb/cockroach/issues/comments/260740446","html_url":"https://github.com/cockroachdb/cockroach/pull/10584#issuecomment-260740446","issue_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/10584","id":260740446,"user":{"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},"created_at":"2016-11-15T19:25:40Z","updated_at":"2016-11-15T19:25:40Z","body":"\n\n\n\nReview status: 8 of 16 files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.\n\n---\n\n*[pkg/sql/pg_catalog.go, line 1138 at r3](https://reviewable.io:443/reviews/cockroachdb/cockroach/10584#-KWBUj1c83_iHU92vfzP:-KWdLp0XJX02D2rJn0F6:byzctu4) ([raw file](https://github.com/cockroachdb/cockroach/blob/712cc60a0f7506bca289d9a61f8efb5ee8c183bd/pkg/sql/pg_catalog.go#L1138)):*\n<details><summary><i>Previously, cuongdo (Cuong Do) wrote…</i></summary>\n> It seems to, though I'm not clear what the symptoms of this *not* working would be.\n</details>\nWon't `reflect.TypeOf(parser.TypeStringArray)` and `reflect.TypeOf(parser.TypeIntArray)` give you the same type? Same thing down below.\n\n---\n\n*[pkg/sql/parser/sql.y, line 400 at r3](https://reviewable.io:443/reviews/cockroachdb/cockroach/10584#-KWBVxN1NhEpqax2RXq8:-KWdMDh7-Yigh-iKRCGo:bobwssq) ([raw file](https://github.com/cockroachdb/cockroach/blob/712cc60a0f7506bca289d9a61f8efb5ee8c183bd/pkg/sql/parser/sql.y#L400)):*\n<details><summary><i>Previously, cuongdo (Cuong Do) wrote…</i></summary>\n> I don't understand your comment (though that's probably due to my rustiness with YACC). I'm looking at these:\n> \n> https://github.com/postgres/postgres/blob/8c48375e5f43ebd832f93c9166d1fe0e639ff806/src/backend/parser/gram.y#L364\n> https://github.com/postgres/postgres/blob/8c48375e5f43ebd832f93c9166d1fe0e639ff806/src/backend/parser/gram.y#L11158\n> \n> and seeing that in the Postgres grammar, `opt_array_bounds` are simply lists of integers. Or, am I misinterpreting something here?\n</details>\nThis should be `%type <Exprs> opt_array_bounds`, not `%type <bool> opt_array_bounds`. We're going to need that for the second branch of the rule.\n\n---\n\n*[pkg/sql/sqlbase/structured.proto, line 41 at r3](https://reviewable.io:443/reviews/cockroachdb/cockroach/10584#-KWBXBUZulFHdV1s0y-5:-KWdO8Itr5HxzimPRcjX:b7jejht) ([raw file](https://github.com/cockroachdb/cockroach/blob/712cc60a0f7506bca289d9a61f8efb5ee8c183bd/pkg/sql/sqlbase/structured.proto#L41)):*\n<details><summary><i>Previously, knz (kena) wrote…</i></summary>\n> Ok I want to react to this point specifically.\n> There is no doubt that what we want is (like pg) homogeneous arrays, where all elements have the same type and \"every row has the same number of columns\" (the general property is to be multi-dimensional rectangularity, i.e. each dimension has the same size at every position in the other dimensions).\n> \n> To achieve this, there are two implementation directions. \n> In both cases, there is a single array in memory that carries all the values.\n> The difference is how you store the \"metadata\".\n> \n> One direction is to have a single descriptor containing two pointers; one pointer to the full value array and one pointer to an array of sizes, one per dimension. \n> \n> The other direction is to have nested descriptors; each descriptor corresponds to a \"type\"; each descriptor contains 3 things, 1) a pointer to a slice, 2) a pointer to the type of the elements (which can be another array type), and a *stride*: an integer value used by the indexing function to multiply by a coordinate at that level to access one element in the slice.\n> \n> The advantage of the first method is that it is slightly cheaper in memory usage when you have a large number of dimensions.\n> The advantage of the second method is that the code for indexing is simpler.\n> \n> Time-complexity wise they come to about the same. I think at this point it is up to the person who will implement this to decide how to do it.\n</details>\nIn that case, it sounds like all we need is:\n```\nrepeated int32 arrayDimensions = 5;\n```\nWe can still use `kind` for the parameterized type of arrays, and `len(arrayDimensions) = 0` to indicate non-array types. Each dimension needs to be able to represent present and non-present size limits, so `uint32` alone won't work. If we make it an `int32`, we can use `-1` to represent no size limit.\n\n\nAlso, I'm a little concerned about making sure no-one is able to use this yet since we don't yet support persisting arrays. Could you add some tests that verify that it isn't actually possible to create a table with `[]int` columns?\n\n---\n\n\n*Comments from [Reviewable](https://reviewable.io:443/reviews/cockroachdb/cockroach/10584)*\n<!-- Sent from Reviewable.io -->\n"}}
{"actor":{"display_login":"nvanbenschoten"}}