{"action":"created","issue":{"url":"https://api.github.com/repos/cockroachdb/cockroach/issues/17279","repository_url":"https://api.github.com/repos/cockroachdb/cockroach","labels_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/17279/labels{/name}","comments_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/17279/comments","events_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/17279/events","html_url":"https://github.com/cockroachdb/cockroach/pull/17279","id":246371735,"number":17279,"title":"sql: add plumbing for distsql-powered csv -> BACKUP conversion","user":{"login":"danhhz","id":52528,"avatar_url":"https://avatars1.githubusercontent.com/u/52528?v=4","gravatar_id":"","url":"https://api.github.com/users/danhhz","html_url":"https://github.com/danhhz","followers_url":"https://api.github.com/users/danhhz/followers","following_url":"https://api.github.com/users/danhhz/following{/other_user}","gists_url":"https://api.github.com/users/danhhz/gists{/gist_id}","starred_url":"https://api.github.com/users/danhhz/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/danhhz/subscriptions","organizations_url":"https://api.github.com/users/danhhz/orgs","repos_url":"https://api.github.com/users/danhhz/repos","events_url":"https://api.github.com/users/danhhz/events{/privacy}","received_events_url":"https://api.github.com/users/danhhz/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":5,"created_at":"2017-07-28T14:55:14Z","updated_at":"2017-07-31T17:41:35Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/cockroachdb/cockroach/pulls/17279","html_url":"https://github.com/cockroachdb/cockroach/pull/17279","diff_url":"https://github.com/cockroachdb/cockroach/pull/17279.diff","patch_url":"https://github.com/cockroachdb/cockroach/pull/17279.patch"},"body":"NOT READY FOR REVIEW YET\r\n\r\nThis is a placeholder to get the distsql plumbing worked out while\r\nmjibson works on the new processors and router. It contains a working\r\nplaceholder LOAD statement that uses planHook and manual distsql\r\nplanning (so we can control where the processors run).\r\n\r\nTODO\r\n- [ ] Validate that it's not crazy to manually construct the plan.\r\n- [ ] It'd be nice if this all started CCL. We might relax that later.\r\n- [ ] I don't know how memory accounting works.\r\n- [ ] Decide if we want to export distSQLPlanner."},"comment":{"url":"https://api.github.com/repos/cockroachdb/cockroach/issues/comments/319141452","html_url":"https://github.com/cockroachdb/cockroach/pull/17279#issuecomment-319141452","issue_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/17279","id":319141452,"user":{"login":"mjibson","id":41181,"avatar_url":"https://avatars1.githubusercontent.com/u/41181?v=4","gravatar_id":"","url":"https://api.github.com/users/mjibson","html_url":"https://github.com/mjibson","followers_url":"https://api.github.com/users/mjibson/followers","following_url":"https://api.github.com/users/mjibson/following{/other_user}","gists_url":"https://api.github.com/users/mjibson/gists{/gist_id}","starred_url":"https://api.github.com/users/mjibson/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/mjibson/subscriptions","organizations_url":"https://api.github.com/users/mjibson/orgs","repos_url":"https://api.github.com/users/mjibson/repos","events_url":"https://api.github.com/users/mjibson/events{/privacy}","received_events_url":"https://api.github.com/users/mjibson/received_events","type":"User","site_admin":false},"created_at":"2017-07-31T17:41:35Z","updated_at":"2017-07-31T17:41:35Z","body":"\n\n\n\nReview status: 0 of 11 files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.\n\n---\n\n*[pkg/sql/planner.go, line 170 at r2](https://reviewable.io:443/reviews/cockroachdb/cockroach/17279#-KqOhsmGJVuzVg41X9BE:-KqOhsmGJVuzVg41X9BF:b-s7l385) ([raw file](https://github.com/cockroachdb/cockroach/blob/6899de4baabef983f6d6f3cadd8c3160b5b2a4fd/pkg/sql/planner.go#L170)):*\n> ```Go\n> \n> func (p *planner) DistLoader() *DistLoader {\n> \treturn &DistLoader{distSQLPlanner: p.session.distSQLPlanner}\n> ```\n\nI'm not a big fan of this approach because it does a number of things just to get around the fact that distSQLPlanner is unexported. Adding a new struct and making the csv implementation a method on that type also feels really weird. Since this is all in the SQL package anyway, we could instead have the LoadCSV function take a PlanHookState. This allows LoadCSV to take many fewer arguments and doesn't require callers to extract all that info from it before calling. I think an argument against that is it makes testing harder since you now have to build an object to call it, but we are testing the SQL statement anyway, so I think that's not a problem.\n\n---\n\n\n*Comments from [Reviewable](https://reviewable.io:443/reviews/cockroachdb/cockroach/17279)*\n<!-- Sent from Reviewable.io -->\n"}}
{"actor":{"display_login":"mjibson"}}