{"action":"created","issue":{"url":"https://api.github.com/repos/cockroachdb/cockroach/issues/24969","repository_url":"https://api.github.com/repos/cockroachdb/cockroach","labels_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/24969/labels{/name}","comments_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/24969/comments","events_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/24969/events","html_url":"https://github.com/cockroachdb/cockroach/pull/24969","id":316540146,"number":24969,"title":"opt: Hoist Exists operator and try to decorrelate","user":{"login":"andy-kimball","id":32096062,"avatar_url":"https://avatars0.githubusercontent.com/u/32096062?v=4","gravatar_id":"","url":"https://api.github.com/users/andy-kimball","html_url":"https://github.com/andy-kimball","followers_url":"https://api.github.com/users/andy-kimball/followers","following_url":"https://api.github.com/users/andy-kimball/following{/other_user}","gists_url":"https://api.github.com/users/andy-kimball/gists{/gist_id}","starred_url":"https://api.github.com/users/andy-kimball/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/andy-kimball/subscriptions","organizations_url":"https://api.github.com/users/andy-kimball/orgs","repos_url":"https://api.github.com/users/andy-kimball/repos","events_url":"https://api.github.com/users/andy-kimball/events{/privacy}","received_events_url":"https://api.github.com/users/andy-kimball/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":3,"created_at":"2018-04-22T01:29:53Z","updated_at":"2018-04-23T19:28:04Z","closed_at":null,"author_association":"CONTRIBUTOR","pull_request":{"url":"https://api.github.com/repos/cockroachdb/cockroach/pulls/24969","html_url":"https://github.com/cockroachdb/cockroach/pull/24969","diff_url":"https://github.com/cockroachdb/cockroach/pull/24969.diff","patch_url":"https://github.com/cockroachdb/cockroach/pull/24969.patch"},"body":"This PR has two commits for easier review. The first adds support for OuterCols as a relational property. The second uses that to implement transforming the Exists operator into a SemiJoinApply operator, and then attempting to decorrelate that."},"comment":{"url":"https://api.github.com/repos/cockroachdb/cockroach/issues/comments/383693935","html_url":"https://github.com/cockroachdb/cockroach/pull/24969#issuecomment-383693935","issue_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/24969","id":383693935,"user":{"login":"RaduBerinde","id":16544120,"avatar_url":"https://avatars1.githubusercontent.com/u/16544120?v=4","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":"2018-04-23T19:28:04Z","updated_at":"2018-04-23T19:28:04Z","author_association":"OWNER","body":"Great stuff!\n\n---\n\nReview status: 14 of 25 files reviewed at latest revision, 9 unresolved discussions.\n\n---\n\n*[pkg/sql/opt/memo/logical_props_factory.go, line 130 at r1](https://beta.reviewable.io/reviews/cockroachdb/cockroach/24969#-LAmzLcA845U3u7FDtok:-LAneVJ6DMJmzF6rQH7z:b2hz8qk) ([raw file](https://github.com/cockroachdb/cockroach/blob/3d13e289c43a5692a636adf0f6336e5bd6e71561/pkg/sql/opt/memo/logical_props_factory.go#L130)):*\n<details><summary><i>Previously, rytaft wrote…</i></summary><blockquote>\n\nShould this line be outside of the `if` statement? (Also, the `if` may not be needed at all -- `Difference` should handle it.)\n</blockquote></details>\n\nNote that `OuterCols` is `inputProps.OuterCols` already here (which is not ok to modify)\n\n---\n\n*[pkg/sql/opt/norm/factory.go, line 187 at r2](https://beta.reviewable.io/reviews/cockroachdb/cockroach/24969#-LAmxYvy7D_9kOQWlC3c:-LAmxYvy7D_9kOQWlC3d:b-hljcyv) ([raw file](https://github.com/cockroachdb/cockroach/blob/36e5213380864a8388588f4792ef269e4e640edb/pkg/sql/opt/norm/factory.go#L187)):*\n> ```Go\n> \t\t}\n> \n> \t\tnewList[i] = item\n> ```\n\n[nit] add a comment saying that if the list doesn't contain the element, we panic here\n\n---\n\n*[pkg/sql/opt/norm/testdata/select, line 821 at r2](https://beta.reviewable.io/reviews/cockroachdb/cockroach/24969#-LAnvEAb8NbGVtLpQtVf:-LAnvEAb8NbGVtLpQtVg:b-pbt99k) ([raw file](https://github.com/cockroachdb/cockroach/blob/36e5213380864a8388588f4792ef269e4e640edb/pkg/sql/opt/norm/testdata/select#L821)):*\n> ```\n>            └── variable: a.k [type=int, outer=(1)]\n> \n> # Ensure that EXISTS is hoisted even when one of several conjuncts.\n> ```\n\n[nit] it is one of\n\n---\n\n*[pkg/sql/opt/norm/testdata/select, line 827 at r2](https://beta.reviewable.io/reviews/cockroachdb/cockroach/24969#-LAnvbyR7aF6HKjn1FSE:-LAnvbyR7aF6HKjn1FSF:b-hddlt0) ([raw file](https://github.com/cockroachdb/cockroach/blob/36e5213380864a8388588f4792ef269e4e640edb/pkg/sql/opt/norm/testdata/select#L827)):*\n> ```\n> semi-join\n>  ├── columns: k:1(int!null) i:2(int) f:3(float) s:4(string) j:5(jsonb)\n>  ├── scan a\n> ```\n\nWhat is missing for `s=foo and i > 1` to be pushed down? I guess `PushFilterIntoJoinLeft` should be enabled on semi-join?\n\n---\n\n*[pkg/sql/opt/norm/testdata/select, line 918 at r2](https://beta.reviewable.io/reviews/cockroachdb/cockroach/24969#-LAnxDpT85dnvRKAdM00:-LAnxDpT85dnvRKAdM01:btoohhz) ([raw file](https://github.com/cockroachdb/cockroach/blob/36e5213380864a8388588f4792ef269e4e640edb/pkg/sql/opt/norm/testdata/select#L918)):*\n> ```\n>  │    ├── columns: b.x:6(int!null) b.y:7(int)\n>  │    └── keys: (6)\n>  └── filters [type=bool, outer=(1,2,4,6), constraints=(/2: [/2 - ]; /4: [/'foo' - /'foo'])]\n> ```\n\nI don't think this is correct. This will for example output all rows where `s <> 'foo'` (it's similar to the behavior of ON with outer joins).\n\nThe filters should stay inside the left input. We could do that by adjusting the rule:\n```\n(AntiJoinApply\n    (Select $input (Filters (RemoveListItem $list $exists)))\n    $subquery\n)\n```\n\n---\n\n*[pkg/sql/opt/ops/scalar.opt, line 66 at r2](https://beta.reviewable.io/reviews/cockroachdb/cockroach/24969#-LAnwDR6CoEpACtmM4Ag:-LAnwDR6CoEpACtmM4Ah:beav52k) ([raw file](https://github.com/cockroachdb/cockroach/blob/36e5213380864a8388588f4792ef269e4e640edb/pkg/sql/opt/ops/scalar.opt#L66)):*\n> ```\n> #     (IsNull (Variable 1))\n> #   )\n> #\n> ```\n\n[nit] maybe mention that this is equivalent to `(IsNull (Subquery (Select .. )))`\n\n---\n\n\n*Comments from [Reviewable](https://beta.reviewable.io/reviews/cockroachdb/cockroach/24969#-:-LAnyXAU7-MhmftV2RXL:b-8o237f)*\n<!-- Sent from Reviewable.io -->\n"}}
{"actor":{"display_login":"RaduBerinde"}}