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.

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: use new batch limit to limit all scans #5214

Merged
merged 2 commits into from
Mar 15, 2016

Conversation

RaduBerinde
Copy link
Member

We change the kvFetcher code to use the new per-batch limit instead of limiting
only single-span scans.

The scan_test code is modified to work with multiple spans (previous code
assumed OR branches would result in spans but that's not the case)

The benchmarks use a table with 10 * 1000 rows. The query involves 4 spans of
1000 rows each, with the filter passing on only the first 10,30,50,70 rows
respectively.

name                                old time/op  new time/op  delta
Scan10000FilterLimit1_Cockroach-4   10.1ms ±51%   0.3ms ±23%  -97.43%  (p=0.000 n=10+9)
Scan10000FilterLimit10_Cockroach-4  9.07ms ±17%  0.27ms ± 2%  -97.05%   (p=0.000 n=9+9)
Scan10000FilterLimit50_Cockroach-4  10.5ms ± 8%  10.8ms ± 9%     ~     (p=0.211 n=10+9)

This change is Review on Reviewable

@petermattis
Copy link
Collaborator

Why does the Scan10000FilterLimit50_Cockroach benchmark not show improvement from this change?

:lgtm:


Reviewed 1 of 1 files at r1.
Review status: 1 of 3 files reviewed at latest revision, all discussions resolved.


Comments from the review on Reviewable.io

@RaduBerinde
Copy link
Member Author

We will retrieve 50 rows but that will not be enough to return 50 results because of the b < 10*a filter. We then continue fetching with the regular batch size. There are various things we can improve here (multiply the initial batch size by a factor depending on filter, or grow the batch size more slowly, e.g. double it each time) , but this is expected for now.


Review status: 1 of 3 files reviewed at latest revision, all discussions resolved, all commit checks successful.


Comments from the review on Reviewable.io

The benchmarks use a table with 10 * 1000 rows. The query involves 4 spans of
1000 rows each, with the filter passing on only the first 10,30,50,70 rows
respectively.
We change the kvFetcher code to use the new per-batch limit instead of limiting
only single-span scans.

The scan_test code is modified to work with multiple spans (previous code
assumed OR branches would result in spans but that's not the case)

```
name                                old time/op  new time/op  delta
Scan10000FilterLimit1_Cockroach-4   10.1ms ±51%   0.3ms ±23%  -97.43%  (p=0.000 n=10+9)
Scan10000FilterLimit10_Cockroach-4  9.07ms ±17%  0.27ms ± 2%  -97.05%   (p=0.000 n=9+9)
Scan10000FilterLimit50_Cockroach-4  10.5ms ± 8%  10.8ms ± 9%     ~     (p=0.211 n=10+9)
```
@petermattis
Copy link
Collaborator

Ah, that makes sense.


Review status: 1 of 3 files reviewed at latest revision, all discussions resolved, all commit checks successful.


Comments from the review on Reviewable.io

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants