{"action":"created","issue":{"url":"https://api.github.com/repos/cockroachdb/cockroach/issues/6735","repository_url":"https://api.github.com/repos/cockroachdb/cockroach","labels_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/6735/labels{/name}","comments_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/6735/comments","events_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/6735/events","html_url":"https://github.com/cockroachdb/cockroach/pull/6735","id":155145701,"number":6735,"title":"sql: refactor selectNode to separate initialization from plan expansion and execution","user":{"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},"labels":[{"url":"https://api.github.com/repos/cockroachdb/cockroach/labels/docs-todo","name":"docs-todo","color":"5319e7"}],"state":"open","locked":false,"assignee":{"login":"RaduBerinde","id":16544120,"avatar_url":"https://avatars.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},"milestone":null,"comments":11,"created_at":"2016-05-16T23:33:14Z","updated_at":"2016-05-23T15:19:40Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/cockroachdb/cockroach/pulls/6735","html_url":"https://github.com/cockroachdb/cockroach/pull/6735","diff_url":"https://github.com/cockroachdb/cockroach/pull/6735.diff","patch_url":"https://github.com/cockroachdb/cockroach/pull/6735.patch"},"body":"The main contribution of this patch is to separate the following phase for`selectNode` (and the other nodes generated by `planner.SelectClause`):\r\n\r\n- initialization, where type checking occurs, necessary to provide  placeholder types during pgwire's prepare phase.\r\n- query plan expansion, including index selection, which needs to know  the placeholder values and thus cannot occur during initialization. This  is needed for `EXPLAIN`.\r\n- query plan execution, where KV requests are sent to the database.\r\n\r\n**A side benefit is that sub-queries are now allowed in LIMIT, OFFSET and RETURNING expressions.**\r\n\r\n**This patch also introduces a new EXPLAIN option, EXPLAIN(TYPES,NOEXPAND) which shows the types in the query plan before it was expanded.**\r\n\r\n**This also introduces a regression on a corner case feature: #6852. This will be addressed by a subsequent PR.**\r\n\r\nThis improvement was facing three main obstacles, which in turn justify three additional changes which are co-dependent with the main change above and thus must be included in the same commit:\r\n\r\n- most expressions may contain sub-queries. These need to be analyzed   in 3 phases as well. Although the `planNode` interface already   separates the 3 phases (`newPlan`, `expandPlan`, `Start`/`Next`),  `parser.Expr` does not do so.\r\n\r\n  Therefore a new `parser.Expr` node is introduced, `sql.subquery`,  which carries both the original parser.Subquery node and its query   plan. The `subqueryVisitor` is repurposed to replace parser.Subquery  expression nodes by sql.subquery nodes (`expandSubqueries` thus  renamed to `replaceSubqueries`). A new `subqueryPlanVisitor` is  introduced to conditionally expand (`expandSubqueryPlans`) or start  (`startSubqueryPlans`) planNodes embedded in sql.subquery expression  nodes.\r\n\r\n  The `Eval` method of the new `subquery` node is then responsible for  returning its results to the surrounding expression context.\r\n\r\n  Consequently, all statement nodes where expressions may occur must  undergo separate processing for subqueries in their initialization,  expansion and execution phases. The nodes impacted are:\r\n\r\n  - deleteNode, insertNode, updateNode: expressions in the returningHelper.\r\n  - insertNode: expressions in the upsertHelper.\r\n  - valuesNode: expressions in the tuples.\r\n  - selectNode and its friends, as described below.\r\n\r\n- the `SelectClause` constructor would conditionally stack a `limit`  node depending on the result of evaluation of the limit  clause. However since the limit clause contains expressions it must  wait until execution to actually know the count and offset values.\r\n\r\n  This patch thus always stacks a limitNode if there is a limit clause  and evaluates the count/offset expression during Start().\r\n\r\n- the `SelectClause` constructor would conditionally stack a  `sortNode` node *and return that* if an order by clause is present  and the natural order as decided by an index is different from the\r\n  requested order. In order to make this choice it must have performed  index selection already.   Meanwhile, this patch needed to delay index selection to the expansion  phase. Since `planNode`'s  `expandPlan` interface cannot replace the  node it's working on, `SelectClause''s original strategy to change   the plan node type depending on circumstances cannot be used any more.\r\n\r\n  This patch thus introduces a new top-level \"select clause node\" that  wraps the effective query plan.\r\n\r\nAlthough the patch may be relatively complex to grok, the upside is that it simplifies many plan nodes and improves readability / maitainability of the query logic.\r\n\r\nFixes #6670.\r\nFixes #6167.\r\n\r\nAnd There Was Much Rejoicing.\r\n\r\n<!-- Reviewable:start -->\r\n---\r\nThis change is [<img src=\"https://reviewable.io/review_button.svg\" height=\"35\" align=\"absmiddle\" alt=\"Reviewable\"/>](https://reviewable.io/reviews/cockroachdb/cockroach/6735)\r\n<!-- Reviewable:end -->\r\n"},"comment":{"url":"https://api.github.com/repos/cockroachdb/cockroach/issues/comments/221008525","html_url":"https://github.com/cockroachdb/cockroach/pull/6735#issuecomment-221008525","issue_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/6735","id":221008525,"user":{"login":"RaduBerinde","id":16544120,"avatar_url":"https://avatars.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},"created_at":"2016-05-23T15:19:40Z","updated_at":"2016-05-23T15:19:40Z","body":"\n\n\n\n\n\nReview status: 27 of 30 files reviewed at latest revision, 23 unresolved discussions, all commit checks successful.\n\n---\n\n*[sql/testdata/update, line 153 \\[r6\\]](https://reviewable.io:443/reviews/cockroachdb/cockroach/6735#-KIT-Ct8-Ca_9hU51FL9:-KITAeRF2yz8B3Pou0yb:-1740278336) ([raw file](https://github.com/cockroachdb/cockroach/blob/6fc326832600de3b7ce7527e296042aa1961cbdb/sql/testdata/update#L153)):*\n<details><summary><i>Previously, tamird (Tamir Duberstein) wrote…</i></summary>\n> Looks like the subquery-in-update tests just moved to testdata/subquery?\n</details>\nThis case just went away..  We have a similar case (where we return a single tuple) but we should probably retain this simpler case as well.\n\n---\n\n\n*Comments from [Reviewable](https://reviewable.io:443/reviews/cockroachdb/cockroach/6735)*\n<!-- Sent from Reviewable.io -->\n"}}
{"actor":{"display_login":"RaduBerinde"}}