-
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 JOIN LATERAL syntax #40945
Conversation
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.
🎉 awesome!! couple nit things
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @chrisseto, @justinj, @RaduBerinde, and @rytaft)
pkg/sql/logictest/testdata/logic_test/subquery_correlated, line 1177 at r1 (raw file):
query ITT rowsort SELECT c_id, bill, states
nit: spaces over tabs, if you use sqlfmt you can add the --use-spaces
flag to enforce this, I have this in my vimrc to sqlfmt whatever I have visually selected:
noremap ,s :!cockroach sqlfmt --print-width 80 --use-spaces<cr><cr>
pkg/sql/opt/optbuilder/join.go, line 37 at r1 (raw file):
isLateral := false inScopeRight := inScope // Detect if this is a lateral join, upgrade scope to leftScope need be
nit: if need be
? also our style guide requires periods at the end of comments
maybe also worth including in the comment something like "the right side of a LATERAL join has in scope the columns produced from the left side" to make explicit why we have to do this?
pkg/sql/opt/optbuilder/join.go, line 370 at r1 (raw file):
jb.filters, &memo.JoinPrivate{Flags: jb.joinFlags}, false,
nit: false /* isLateral */,
pkg/sql/opt/optbuilder/testdata/lateral, line 292 at r1 (raw file):
build SELECT * FROM x JOIN LATERAL (SELECT * FROM y WHERE b = x.a) ON true
probably should also test LEFT JOIN LATERAL
, and we might also need to error appropriately on use of OUTER JOIN LATERAL
and RIGHT JOIN LATERAL
(I assume we just let those through at the moment)
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.
after you address @justinj's comments
Reviewed 3 of 3 files at r1.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @chrisseto and @RaduBerinde)
c96898f
to
aa05126
Compare
Feedback addressed!
I went with detail message and the syntax code as it seemed more appropriate. |
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.
with some nits, though reminder that this can't be merged until post-20.1-branching.
Reviewable status: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @chrisseto, @RaduBerinde, and @rytaft)
pkg/sql/opt/optbuilder/join.go, line 201 at r2 (raw file):
case sqlbase.RightOuterJoin: if isLateral { panic(pgerror.New(pgcode.Syntax, "The combining JOIN type must be INNER or LEFT for a LATERAL reference."))
nit: hoist this error object into a top-level var
(like this) and omit the period in the error message (i think lint will complain about the period)
aa05126
to
b362236
Compare
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.
Reviewed 1 of 3 files at r2, 2 of 2 files at r3.
Reviewable status: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @chrisseto and @RaduBerinde)
pkg/sql/opt/optbuilder/join.go, line 181 at r3 (raw file):
} var invalidLateralJoin = pgerror.New(pgcode.Syntax, "The combining JOIN type must be INNER or LEFT for a LATERAL reference")
[nit] Is this the error Postgres uses? The word "combining" seems unnecessary here and is a bit confusing. I'd just say "The JOIN type must be..."
pkg/sql/opt/optbuilder/join.go, line 181 at r3 (raw file): Previously, rytaft (Rebecca Taft) wrote…
Never mind just saw your update above |
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.
Now that we've branched I think this is good to go after a rebase!
Reviewable status: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @chrisseto and @RaduBerinde)
b362236
to
6ee4fee
Compare
As correlated subqueries are already supported, only minor scoping changes are required to fully support Postgres' JOIN LATERAL syntax. h/t @justinj for the support Release note (sql change): Adds support for JOIN LATERAL syntax.
6ee4fee
to
6331eb0
Compare
bors r+ |
Build failed (retrying...) |
bors r+ |
40945: sql: Add support for JOIN LATERAL syntax r=chrisseto a=chrisseto As correlated subqueries are already supported, only minor scoping changes are required to fully support Postgres' JOIN LATERAL syntax. h/t @justinj for the support Release note (sql change): Adds support for JOIN LATERAL syntax. Co-authored-by: Chris Seto <[email protected]>
Build succeeded |
As correlated subqueries are already supported, only minor scoping
changes are required to fully support Postgres' JOIN LATERAL syntax.
h/t @justinj for the support
Release note (sql change): Adds support for JOIN LATERAL syntax.