-
Notifications
You must be signed in to change notification settings - Fork 3.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
sql: add support for subqueries with ANY/SOME/ALL operations #18094
Conversation
pkg/sql/parser/eval.go
Outdated
|
||
if all { | ||
if !allTrue { | ||
if res && any { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I optimized this loop through each Datum
in our slice of Datum
s such that it short circuits/early terminates when:
ANY
: result is encountered whereres = true
(formally, there exists a right such that the predicate evaluates totrue
, "any" is true)ALL
: result is encountered whereres = false
(there exists aright
such that the predicate evaluates tofalse
, not all are true)
This results in statements such as this
SELECT 1 = ALL(SELECT * FROM generate_series(1,1000000))
to terminate after evaluating 1 = 2
instead of iterating until the end.
This looks great so far! I left some comments, but @knz should probably take a look as well! Review status: 0 of 6 files reviewed at latest revision, 6 unresolved discussions, some commit checks pending. pkg/sql/logictest/testdata/logic_test/suboperators, line 1 at r1 (raw file):
So thorough 👍 could we also have a couple tests involving pkg/sql/parser/eval.go, line 1742 at r1 (raw file):
Perhaps we could just branch once at the top for pkg/sql/parser/eval.go, line 2726 at r1 (raw file):
nit: we generally put periods at the end of comments (also applies to some other comments here), also, is this comment accurate? we don't seem to branch to different functions, just different ways of extracting the pkg/sql/parser/eval.go, line 2733 at r1 (raw file):
consider adding a pkg/sql/parser/type_check.go, line 1011 at r1 (raw file):
if you replace this with a type switch you won't have to do the explicit casts below. Comments from Reviewable |
I'll let you process and iterate with Justin first. Once you've settled together I'll do a round too. I don't think it's useful to have 2+ people reviewing at the same time for now. Let me know. Please however do respect the rule 1 concern = 1 commit, see my comment below. Review status: 0 of 6 files reviewed at latest revision, 6 unresolved discussions, all commit checks successful. pkg/sql/parser/eval.go, line 1759 at r1 (raw file): Previously, richardwu (Richard Wu) wrote…
Please split this change into a different commit. Add tests that involve NULL in the subquery. Comments from Reviewable |
1b6c5a0
to
6bed659
Compare
Review status: 0 of 6 files reviewed at latest revision, 6 unresolved discussions, some commit checks pending. pkg/sql/logictest/testdata/logic_test/suboperators, line 1 at r1 (raw file): Previously, justinj (Justin Jaffray) wrote…
Done. pkg/sql/parser/eval.go, line 1742 at r1 (raw file): Previously, justinj (Justin Jaffray) wrote…
As per @knz's comment, I will move this refactor to a separate PR. pkg/sql/parser/eval.go, line 2726 at r1 (raw file): Previously, justinj (Justin Jaffray) wrote…
Done. pkg/sql/parser/eval.go, line 2733 at r1 (raw file): Previously, justinj (Justin Jaffray) wrote…
I decided to propagate an internal PG error so we don't crash the instance. pkg/sql/parser/type_check.go, line 1011 at r1 (raw file): Previously, justinj (Justin Jaffray) wrote…
Done. Comments from Reviewable |
This all looks good to me (modulo a squash and making sure the final commit name has the Review status: 0 of 6 files reviewed at latest revision, 1 unresolved discussion, some commit checks pending. pkg/sql/parser/eval.go, line 1742 at r1 (raw file): Previously, richardwu (Richard Wu) wrote…
sgtm 👍 pkg/sql/parser/eval.go, line 2733 at r1 (raw file): Previously, richardwu (Richard Wu) wrote…
sounds reasonable. I don't think we log Comments from Reviewable |
Reviewed 5 of 5 files at r1, 1 of 1 files at r2, 1 of 1 files at r3, 1 of 2 files at r4, 1 of 1 files at r5. pkg/sql/parser/eval.go, line 2733 at r1 (raw file): Previously, justinj (Justin Jaffray) wrote…
I think we do actually -- or if we don't there's an issue already for doing so, scheduled for 1.2. all good. pkg/sql/parser/type_check.go, line 998 at r1 (raw file):
"as an array" - capital not needed pkg/sql/parser/type_check.go, line 1017 at r5 (raw file):
Woah, hold your horses. We can produce tuples in SQL in other ways than using queries! I don't think you'll be getting away without special casing subqueries out of other kinds of tuples. Comments from Reviewable |
Review status: all files reviewed at latest revision, 4 unresolved discussions, some commit checks failed. pkg/sql/parser/type_check.go, line 1017 at r5 (raw file): Previously, knz (kena) wrote…
Ah, you're right, sorry for missing that @richardwu! Comments from Reviewable |
6bed659
to
1383053
Compare
Review status: 0 of 7 files reviewed at latest revision, 4 unresolved discussions, some commit checks pending. pkg/sql/parser/type_check.go, line 998 at r1 (raw file): Previously, knz (kena) wrote…
Done. pkg/sql/parser/type_check.go, line 1017 at r5 (raw file): Previously, justinj (Justin Jaffray) wrote…
Done. Comments from Reviewable |
1383053
to
d2af1a0
Compare
So I ran into a few failed tests that resulted from our use of type checking on The "lifetime" of the new I tried my best to go through all the instances of type checking for |
Reviewed 1 of 6 files at r6, 1 of 1 files at r7, 1 of 1 files at r10, 5 of 5 files at r11. pkg/sql/subquery.go, line 333 at r11 (raw file):
I don't think you can observe pkg/sql/update.go, line 310 at r11 (raw file):
I think because of your change it becomes impossible to encounter a naked pkg/sql/update.go, line 481 at r11 (raw file):
ditto pkg/sql/logictest/testdata/logic_test/suboperators, line 327 at r11 (raw file):
I fail to agree here -- it seems to me that we can compare a value with the members of a tuple, when the tuple values match in type. pkg/sql/parser/expr.go, line 737 at r11 (raw file):
This String method does not belong here. Place it at the end of pkg/sql/parser/expr.go, line 749 at r11 (raw file):
Unless you do the deferred construction thing I explained earlier, this method should do The entire Placeholder must implement the Expr interface, which in turn means it also needs an Eval and ResolvedType methods. These do recurse into pkg/sql/parser/type_check.go, line 1019 at r11 (raw file):
You need two separate pieces of logic, one for tuples generated by non-subqueries (where the RHS is the entire tuple) and one for tuples geneerated by subqueries. Comments from Reviewable |
Review status: all files reviewed at latest revision, 11 unresolved discussions, all commit checks successful. pkg/sql/parser/expr.go, line 734 at r11 (raw file):
Comments from Reviewable |
pkg/sql/logictest/testdata/logic_test/suboperators, line 327 at r11 (raw file): Previously, knz (kena) wrote…
So I was always under the impression that we try to follow Postgres semantics as much as possible (Postgres errors with "right hand side must be an array"). Do we want to introduce this feature of being able to use primitive tuples with ANY/ALL operations? If that's the case, we can defer the usage of a Comments from Reviewable |
dd19920
to
8fb4da8
Compare
pkg/sql/subquery.go, line 333 at r11 (raw file): Previously, knz (kena) wrote…
I have decided to not introduce the SubqueryPlaceholder in this PR since it is not strictly required (since we are supporting tuples with ANY/ALL, we do not need to special case subqueries as they evaluate to tuples). Comments from Reviewable |
Review status: 2 of 6 files reviewed at latest revision, 11 unresolved discussions, some commit checks pending. pkg/sql/parser/type_check.go, line 1019 at r11 (raw file): Previously, knz (kena) wrote…
Ditto above comment Comments from Reviewable |
Review status: 2 of 6 files reviewed at latest revision, 11 unresolved discussions, some commit checks pending. pkg/sql/update.go, line 310 at r11 (raw file): Previously, knz (kena) wrote…
Done. pkg/sql/update.go, line 481 at r11 (raw file): Previously, knz (kena) wrote…
Done. pkg/sql/parser/expr.go, line 734 at r11 (raw file): Previously, knz (kena) wrote…
Done. pkg/sql/parser/expr.go, line 737 at r11 (raw file): Previously, knz (kena) wrote…
Done. pkg/sql/parser/expr.go, line 749 at r11 (raw file): Previously, knz (kena) wrote…
Done. Comments from Reviewable |
8fb4da8
to
0e3424b
Compare
It's nice to see how unifying the tuple and subquery cases also simplify the code. I like where this is headed at last. One last bug to correct and you should be good to go. Reviewed 6 of 6 files at r12. pkg/sql/parser/type_check.go, line 1000 at r12 (raw file):
Add: pkg/sql/parser/type_check.go, line 1029 at r12 (raw file):
This is incorrect, and the following statement will demonstrate it is incorrect by crashing the server: What you need to do:
Comments from Reviewable |
Review status: all files reviewed at latest revision, 5 unresolved discussions, all commit checks successful. pkg/sql/parser/type_check.go, line 1000 at r12 (raw file): Previously, knz (kena) wrote…
Also, change this to use pkg/sql/parser/type_check.go, line 1029 at r12 (raw file): Previously, knz (kena) wrote…
Also, add a test -- the example I gave should properly return an error. Comments from Reviewable |
0e3424b
to
c99b1d5
Compare
pkg/sql/parser/type_check.go, line 1029 at r12 (raw file): Previously, knz (kena) wrote…
So I played around with the current implementation and it does correctly return an error for the statement If I were to check all tuple element types to be identical, then the following statement would return a type mismatch error
You can check out the logic test I've added with your example https://github.com/cockroachdb/cockroach/pull/18094/files#diff-7021787014e75d2e5d4600ab91b6475aR353. Comments from Reviewable |
c99b1d5
to
f4eb2dc
Compare
pkg/sql/parser/type_check.go, line 1000 at r12 (raw file): Previously, knz (kena) wrote…
So I did some investigation with how our I've updated the statement to simply type check I've verified we correctly type check expressions in the logs for both arrays and tuples
Comments from Reviewable |
Review status: 4 of 6 files reviewed at latest revision, 5 unresolved discussions, some commit checks failed. pkg/sql/parser/type_check.go, line 1029 at r12 (raw file): Previously, richardwu (Richard Wu) wrote…
If you use Also check with Jordan what is going on with placeholders. Right now the tests fail. Comments from Reviewable |
…datums to a follow-up PR
f4eb2dc
to
25ca38f
Compare
pkg/sql/parser/type_check.go, line 1029 at r12 (raw file): Previously, knz (kena) wrote…
As discussed offline, I've written a helper function I've also updated logic tests for the correct behavior in @knz's comment. Comments from Reviewable |
Reviewed 1 of 5 files at r13, 1 of 1 files at r17, 3 of 3 files at r18. pkg/sql/parser/type_check.go, line 917 at r18 (raw file):
lowercase "tuple" Comments from Reviewable |
25ca38f
to
8dc1301
Compare
8dc1301
to
81472a9
Compare
Previously we only supported arrays for
ANY
andALL
operations:This PR allows us to use subqueries as well in the RHS of the predicate:
Fixes #17662