Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[
Improve
]: scan operations forIReadOnlyStoreView
and fix ofFind
with all 0xffkeyPrefix
#3688[
Improve
]: scan operations forIReadOnlyStoreView
and fix ofFind
with all 0xffkeyPrefix
#3688Changes from 13 commits
3b7cd98
6ccb8ca
5628012
a81b8f6
16c9ef2
86a8d75
77fb843
7477f87
1bb8715
0c7767a
3dde742
746b2ad
c06a38d
1eb7e1e
eb26415
bf905fe
b5bc0e7
976dd32
5beabbc
4d7f58a
46c28a8
a417172
f959ca4
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just use
comparer
. Just forget about these direction things for a min.dotnet
usescomparer
why can't we? There are good reasons to usecomparer
instead ofenums
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't have to do this if you use
comparer
. Just look atLevelDbStore
andMemoryStore
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These lines aren't new.
I moved these from
DataCache.cs
to here.https://github.com/neo-project/neo/blob/master/src/Neo/Persistence/DataCache.cs#L242
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copying/Moving bugs doesn't make it ok.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR didn't change this.
You can submit a new PR to fix it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You want the the key. If the prefix is a key?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
exclusiveEndKey
because of exclusive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why go to all the trouble to edit or change this. If your not going to fix the issues with the class?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because this is
ScanRange
/FindRange
, and range is [StartKey, EndKey)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
start key and end key are not prefix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/neo-project/neo/blob/master/src/Neo/Persistence/DataCache.cs#L264
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like I said,
DataCache
has bugs.LevelDBStore
neo/src/Plugins/LevelDBStore/IO/Data/LevelDB/Helper.cs
Lines 21 to 43 in 84a9fd5
RocksDbStore
neo/src/Plugins/RocksDBStore/Plugins/Storage/Store.cs
Lines 41 to 52 in 84a9fd5
MemoryStore
neo/src/Neo/Persistence/MemoryStore.cs
Lines 51 to 63 in 84a9fd5
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If
DataCache
has bugs, you can submit a PR to fix it.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not
**Find**
.ScanRange
/FindRange
return items within range [StartKey(inclusive), EndKey(exclusive) )There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can read this:
neo/src/Neo/Persistence/DataCache.cs
Lines 251 to 258 in 84a9fd5
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is inconsistent with
MemoryStore
,LevelDbStore
andRocksDbStore
. Why?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to fix it. #3680
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in #3701, but that PR isn't going to get merge. So, if you want you can create new PR and use my work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I tried to fix it.
But a requirement I received is to maintain consistent interface/api behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
who required this? Well don't listen them. As long as two developer approve a PR. It gets merged (with no issues and change requests).