Skip to content
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.

Sign up for GitHub

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

Jump to bottom

sql: lookup joins should use batch limit #35950

Closed
RaduBerinde opened this issue Mar 19, 2019 · 2 comments · Fixed by #38837
Closed

sql: lookup joins should use batch limit #35950

RaduBerinde opened this issue Mar 19, 2019 · 2 comments · Fixed by #38837
Assignees
@jordanlewis
Labels
A-sql-execution Relating to SQL execution. C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. S-2-temp-unavailability Temp crashes or other availability problems. Can be worked around or resolved by restarting.

Comments

@RaduBerinde
Copy link
Member

See #35947 (comment)

Lookup joins should use a batch limit because we don't know how many rows we might get - unless we know the input forms a key (in which case we know we'll get at most one per input row).
@RaduBerinde RaduBerinde added the A-sql-execution Relating to SQL execution. label Mar 19, 2019
@RaduBerinde RaduBerinde mentioned this issue Mar 19, 2019
Closed
@RaduBerinde RaduBerinde added the S-2-temp-unavailability Temp crashes or other availability problems. Can be worked around or resolved by restarting. label Mar 19, 2019
@awoods187 awoods187 added the C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. label Apr 22, 2019
@jordanlewis jordanlewis self-assigned this May 7, 2019
@jordanlewis
Copy link
Member

I investigated this. Currently, lookup joins can't use batch limits because we restrict batch scan requests to have ordered spans. I think the reason for this is the ResumeSpan semantics, but I'm not sure...

@RaduBerinde you added this restriction way back in 2016 - do you understand why it exists?

@RaduBerinde
Copy link
Member Author

I don't remember the details, but yes I believe it had to do with the resume span semantics. Those semantics might have evolved over time though so we may not have the same restrictions today.

This was referenced Jul 10, 2019
Closed
Merged
craig bot pushed a commit that referenced this issue Jul 16, 2019
@jordanlewis @yuzefovich
Merge #38837 #38887
07732c1 
38837: distsqlrun: make joinreader use limited batches r=jordanlewis a=jordanlewis

Closes #35950.

Previously, lookup join put no limit on the number of returned rows from
each of its lookups. This could be hugely problematic. Imagine a
circumstance in which a lookup join was planned on a pair of tables that
had 1 billion match rows on the right side for every row on the left
side: in this case, the lookup join would ask the KV backend to return a
single giant buffer with all 1 billion rows.

Also, joinReader's tracing span collection was slightly broken - fix that while we're at it, to make sure we can test this change properly.

Release note (bug fix): prevent OOM conditions during lookup joins
between tables with a very large n:1 relationship.

38887: exec: check for unsupported by ArrowBatchConverter type r=yuzefovich a=yuzefovich

Previously, we would create an arrow batch converter without
paying attention to whether we supported the types which could
lead to a panic during actual conversion. Now we do this check
upfront so that we can fall back to DistSQL.

Release note: None

Co-authored-by: Jordan Lewis <jordanthelewis@gmail.com>
Co-authored-by: Yahor Yuzefovich <yahor@cockroachlabs.com>
@craig craig bot closed this as completed in #38837 Jul 16, 2019
@jordanlewis jordanlewis mentioned this issue Jul 23, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@jordanlewis jordanlewis

Labels
A-sql-execution Relating to SQL execution. C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. S-2-temp-unavailability Temp crashes or other availability problems. Can be worked around or resolved by restarting.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

distsqlrun: make joinreader use limited batches jordanlewis/cockroach
3 participants
@jordanlewis @RaduBerinde @awoods187