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: Add support for reverse loose index scans #38459

Merged
merged 1 commit into from
Jul 1, 2019

Conversation

rohany
Copy link
Contributor

@rohany rohany commented Jun 26, 2019

Implements support for reverse scans by the IndexSkipTableReader. Supports interleaved tables, but scans on these tables will be slower than in the forward direction.

Depends on #38442

Resolves #38416

@rohany rohany requested review from solongordon and a team June 26, 2019 20:03
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Contributor

@solongordon solongordon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good stuff! Comments could use a bit of clarification but otherwise :lgtm:.

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @rohany and @solongordon)


pkg/sql/distsqlrun/index_skip_table_reader.go, line 210 at r10 (raw file):

		if !t.spans[t.currentSpan].Valid() {
			t.currentSpan++
			continue

Huh, was this continue a bug?


pkg/sql/distsqlrun/index_skip_table_reader.go, line 211 at r10 (raw file):

			t.spans[t.currentSpan].Key = key
		} else {
			// In the case of reverse, this is much easier. The reverse batcher

Did you mean fetcher?


pkg/sql/distsqlrun/index_skip_table_reader.go, line 212 at r10 (raw file):

		} else {
			// In the case of reverse, this is much easier. The reverse batcher
			// gives us the key that we got. Since ranges are exclusive, we just

"gives us the key that we got" 🤔

Also maybe better: "Since EndKey is exclusive..."

Copy link
Contributor Author

@rohany rohany left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @solongordon)


pkg/sql/distsqlrun/index_skip_table_reader.go, line 210 at r10 (raw file):

Previously, solongordon (Solon) wrote…

Huh, was this continue a bug?

yeah, it was. If the next span was invalid, we wouldn't return the row we were supposed to even though the current span was valid.


pkg/sql/distsqlrun/index_skip_table_reader.go, line 211 at r10 (raw file):

Previously, solongordon (Solon) wrote…

Did you mean fetcher?

Done.


pkg/sql/distsqlrun/index_skip_table_reader.go, line 212 at r10 (raw file):

Previously, solongordon (Solon) wrote…

"gives us the key that we got" 🤔

Also maybe better: "Since EndKey is exclusive..."

Done.

Note that due to data layout of interleaved tables, the reverse
scan will not be as efficient as in the forward direction.

Release note: None
@rohany
Copy link
Contributor Author

rohany commented Jul 1, 2019

bors r+

craig bot pushed a commit that referenced this pull request Jul 1, 2019
38459: sql: Add support for reverse loose index scans r=rohany a=rohany

Implements support for reverse scans by the IndexSkipTableReader. Supports interleaved tables, but scans on these tables will be slower than in the forward direction.

Depends on #38442

Resolves #38416

Co-authored-by: Rohan Yadav <[email protected]>
@craig
Copy link
Contributor

craig bot commented Jul 1, 2019

Build succeeded

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.

sql: Add support for reverse loose index scans
3 participants