Add key prefix matching to KEYS command #237

Merged
prologic merged 5 commits from biozz/bitcask:keys-prefix-matching into master 2 years ago
biozz commented 2 years ago

Related to #234 and !236.

This is the implementation that was requested in the original issue. I updated KEYS command to be redis-valid and implemented prefix search. There is also a rather interesting test, I could you use some feedback here.

I noticed that it might not be possible to reduce the complexity of the KEYS command. Because even if you use Scan, you will have to store the counter of all found keys before you do WriteBulk of the actual keys.

@prologic here is what you probably had in mind:

s.db.Scan([]byte(prefix), func(key []byte) error {
	conn.WriteBulk(key)
	return nil
})

But there is no way to call conn.WriteArray(n) with the number of keys until you iterate through all of them, hence the second loop over found keys.

Related to #234 and !236. This is the implementation that was requested in the original issue. I updated KEYS command to be redis-valid and implemented prefix search. There is also a rather interesting test, I could you use some feedback here. I noticed that it might not be possible to reduce the complexity of the KEYS command. Because even if you use Scan, you will have to store the counter of all found keys before you do WriteBulk of the actual keys. @prologic here is what you probably had in mind: ``` s.db.Scan([]byte(prefix), func(key []byte) error { conn.WriteBulk(key) return nil }) ``` But there is no way to call `conn.WriteArray(n)` with the number of keys until you iterate through all of them, hence the second loop over found keys.
biozz added 2 commits 2 years ago
2d3634b422
Add key prefix matching to KEYS command
continuous-integration/drone/pr Build is passing Details
fc7b0b1dc5
Update authors
Owner

Yeah you're right! YOu'd have to accumulate the metched keys sadly.

Yeah you're right! YOu'd have to accumulate the metched keys sadly.
Owner

I think what you've done here in this PR is great though and it will perform much better with a database with a larger keyspace. Iteresting through hundreds or thousands of keys will be slow, so reducing that space with ascan is goo.d

I _think_ what you've done here in this PR is great though and it **will** perform much better with a database with a larger keyspace. Iteresting through hundreds or thousands of keys will be slow, so reducing that space with ascan is goo.d
prologic added 1 commit 2 years ago
continuous-integration/drone/pr Build is passing Details
826dd0ab6f
Merge branch 'master' into keys-prefix-matching
biozz added 1 commit 2 years ago
continuous-integration/drone/pr Build is passing Details
2dbf81699d
Make use of the original gitignore, rename test
biozz added 1 commit 2 years ago
continuous-integration/drone/pr Build is passing Details
48d48aea0e
Revert deleted whitespace
biozz commented 2 years ago
Poster

@prologic @taigrr would you be so kind and give this PR a round of code review?

@prologic @taigrr would you be so kind and give this PR a round of code review?
Owner

@biozz Oh yes so sorry! Work has been "flat out" 🤣

@biozz Oh yes so sorry! Work has been "flat out" 🤣
Owner

I actually looked at the code briefly before. This LGTM 👌

I _actually_ looked at the code briefly before. This LGTM 👌
prologic approved these changes 2 years ago
prologic left a comment
Owner

LGTM

LGTM
prologic merged commit 21a824e13e into master 2 years ago
prologic deleted branch keys-prefix-matching 2 years ago

Reviewers

prologic approved these changes 2 years ago
continuous-integration/drone/pr Build is passing
The pull request has been merged as 21a824e13e.
Sign in to join this conversation.
Loading…
There is no content yet.