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

distsql: revisit forward loose scan lookup key construction #39659

Closed
asubiotto opened this issue Aug 14, 2019 · 0 comments · Fixed by #39833
Closed

distsql: revisit forward loose scan lookup key construction #39659

asubiotto opened this issue Aug 14, 2019 · 0 comments · Fixed by #39833
Assignees
@rohany
Labels
A-sql-execution Relating to SQL execution. C-cleanup Tech debt, refactors, loose ends, etc. Solution not expected to significantly change behavior.

Comments

@asubiotto
Copy link
Contributor

We should:

  • See if we can use PrefixEnd to construct the lookup for the next prefix. We currently append nulls (0xff) to the key. Make sure that if we encounter a maximal key, since PrefixEnd returns that, we exit somehow (either through a validity check or explicit check).
  • Add a test for the case in which we encounter a prefix key that is all (0xff) already.

More discussion: https://reviewable.io/reviews/cockroachdb/cockroach/39636#-LmBV-cn9nb-Ey445Chv
@asubiotto asubiotto added C-cleanup Tech debt, refactors, loose ends, etc. Solution not expected to significantly change behavior. A-sql-execution Relating to SQL execution. labels Aug 14, 2019
rohany added a commit to rohany/cockroach that referenced this issue Aug 22, 2019
@rohany
sql: Bugfixes and increased tests for IndexSkipTableReader
1d42f9f 
Fixes cockroachdb#39659.

Release note: None
@rohany rohany mentioned this issue Aug 22, 2019
Merged
craig bot pushed a commit that referenced this issue Aug 23, 2019
@rohany
Merge #39833
f423e2e 
39833: sql: Bugfixes and increased tests for IndexSkipTableReader r=asubiotto a=rohany

I investigated the index skip table reader and added some additional tests for null / maximal key handling. It turns out that the for loop we had was incorrect anyway, and PrefixEnd did what was needed.

Fixes #39659.

Release note: None

Co-authored-by: Rohan Yadav <rohany@alumni.cmu.edu>
@craig craig bot closed this as completed in 1fff1d0 Aug 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@rohany rohany

Labels
A-sql-execution Relating to SQL execution. C-cleanup Tech debt, refactors, loose ends, etc. Solution not expected to significantly change behavior.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

sql: Bugfixes and increased tests for IndexSkipTableReader rohany/cockroach
2 participants
@asubiotto @rohany