-
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
rowexec: allow ordered joinReader to stream matches to the first row #85731
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.
Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @DrewKimball)
pkg/sql/rowexec/joinreader_strategies.go
line 516 at r1 (raw file):
// on. This could significantly decrease the overhead of buffering looked // up rows. unbufferedRow rowenc.EncDatumRow
nit: "unbuffered" reads unusual to me, maybe "notBuffered" would be better?
pkg/sql/rowexec/joinreader_strategies.go
line 654 at r1 (raw file):
if !s.isPartialJoin { if inputRowIdx == 0 { // Don't add to inputRowIdxToLookedUpRowIndices in order to avoid
nit: it'd be good to update the comment on inputRowIdxToLookedUpRowIndices
accordingly.
fe14b62
to
e8c846c
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.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @yuzefovich)
pkg/sql/rowexec/joinreader_strategies.go
line 516 at r1 (raw file):
Previously, yuzefovich (Yahor Yuzefovich) wrote…
nit: "unbuffered" reads unusual to me, maybe "notBuffered" would be better?
Done.
pkg/sql/rowexec/joinreader_strategies.go
line 654 at r1 (raw file):
Previously, yuzefovich (Yahor Yuzefovich) wrote…
nit: it'd be good to update the comment on
inputRowIdxToLookedUpRowIndices
accordingly.
Done.
Currently the `joinReaderOrderingStrategy` implementation buffers all looked up rows before matching them with input rows and emitting them. This is necessary because the looked up rows may not be received in input order (which must be maintained). However, rows that match the first input row can be emitted immediately. In the case when there are many rows that match the first input row, this can decrease overhead of the buffer. Additionally, this change can allow a limit to be satisfied earlier, which can significantly decrease latency. This is especially advantageous in the case when there is only one input row, since all lookups can then be rendered and returned in streaming fashion. Release note (performance improvement): The execution engine can now short-circuit execution of lookup joins in more cases, which can decrease latency for queries with limits.
e8c846c
to
d413fe7
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.
TFTR!
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @yuzefovich)
bors r+ |
Test failure looks like an unrelated flake. |
Build succeeded: |
Currently the
joinReaderOrderingStrategy
implementation buffers all looked uprows before matching them with input rows and emitting them. This is necessary
because the looked up rows may not be received in input order (which must be
maintained). However, rows that match the first input row can be emitted
immediately. In the case when there are many rows that match the first input
row, this can decrease overhead of the buffer. Additionally, this change can
allow a limit to be satisfied earlier, which can significantly decrease
latency. This is especially advantageous in the case when there is only one
input row, since all lookups can then be rendered and returned in streaming
fashion.
Release note (performance improvement): The execution engine can now
short-circuit execution of lookup joins in more cases, which can decrease
latency for queries with limits.