{"action":"closed","issue":{"url":"https://api.github.com/repos/cockroachdb/cockroach/issues/13594","repository_url":"https://api.github.com/repos/cockroachdb/cockroach","labels_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/13594/labels{/name}","comments_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/13594/comments","events_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/13594/events","html_url":"https://github.com/cockroachdb/cockroach/issues/13594","id":207808973,"number":13594,"title":"sql: revisit ordering code for ordinalityNode","user":{"login":"RaduBerinde","id":16544120,"avatar_url":"https://avatars2.githubusercontent.com/u/16544120?v=3","gravatar_id":"","url":"https://api.github.com/users/RaduBerinde","html_url":"https://github.com/RaduBerinde","followers_url":"https://api.github.com/users/RaduBerinde/followers","following_url":"https://api.github.com/users/RaduBerinde/following{/other_user}","gists_url":"https://api.github.com/users/RaduBerinde/gists{/gist_id}","starred_url":"https://api.github.com/users/RaduBerinde/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/RaduBerinde/subscriptions","organizations_url":"https://api.github.com/users/RaduBerinde/orgs","repos_url":"https://api.github.com/users/RaduBerinde/repos","events_url":"https://api.github.com/users/RaduBerinde/events{/privacy}","received_events_url":"https://api.github.com/users/RaduBerinde/received_events","type":"User","site_admin":false},"labels":[],"state":"closed","locked":false,"assignee":{"login":"knz","id":642886,"avatar_url":"https://avatars0.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://avatars0.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}],"milestone":{"url":"https://api.github.com/repos/cockroachdb/cockroach/milestones/3","html_url":"https://github.com/cockroachdb/cockroach/milestone/3","labels_url":"https://api.github.com/repos/cockroachdb/cockroach/milestones/3/labels","id":1298443,"number":3,"title":"Later","description":"","creator":{"login":"BramGruneir","id":1614265,"avatar_url":"https://avatars2.githubusercontent.com/u/1614265?v=3","gravatar_id":"","url":"https://api.github.com/users/BramGruneir","html_url":"https://github.com/BramGruneir","followers_url":"https://api.github.com/users/BramGruneir/followers","following_url":"https://api.github.com/users/BramGruneir/following{/other_user}","gists_url":"https://api.github.com/users/BramGruneir/gists{/gist_id}","starred_url":"https://api.github.com/users/BramGruneir/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/BramGruneir/subscriptions","organizations_url":"https://api.github.com/users/BramGruneir/orgs","repos_url":"https://api.github.com/users/BramGruneir/repos","events_url":"https://api.github.com/users/BramGruneir/events{/privacy}","received_events_url":"https://api.github.com/users/BramGruneir/received_events","type":"User","site_admin":false},"open_issues":392,"closed_issues":75,"state":"open","created_at":"2015-09-09T21:24:17Z","updated_at":"2017-06-13T23:23:28Z","due_on":null,"closed_at":null},"comments":3,"created_at":"2017-02-15T13:49:30Z","updated_at":"2017-06-13T23:23:28Z","closed_at":"2017-06-13T23:23:28Z","body":"During expand plan we have one-off logic for the ordering of ordinalityNode: https://github.com/cockroachdb/cockroach/blob/b02e43f9e6d497f162cdc2fc1d72b5c9364aa00c/pkg/sql/expand_plan.go#L124\r\n\r\nThis should be better encapsulated in the node itself. The logic also needs to be revisited. We try to pass through a desired ordering even if there is a desired ordering on the ordinality column (code below) but that will never end up being used - what is important is the way we update `n.ordering` afterward; we only use the ordinality ordering if there is no underlying ordering. We also aren't checking for the desired direction of the ordinality column in this code:\r\n\r\n```go\r\n// If there's a desired ordering on the ordinality column, drop it.\r\nif len(params.desiredOrdering) > 0 {\r\n\tnewDesired := make(sqlbase.ColumnOrdering, 0, len(params.desiredOrdering))\r\n\tfor _, ordInfo := range params.desiredOrdering {\r\n\t\tif ordInfo.ColIdx == len(n.columns)-1 {\r\n\t\t\tbreak\r\n\t\t}\r\n\t\tnewDesired = append(newDesired, ordInfo)\r\n\t}\r\n\tparams.desiredOrdering = newDesired\r\n}\r\n```\r\nCC @knz"}}
{"actor":{"display_login":"knz"}}