-
Notifications
You must be signed in to change notification settings - Fork 14
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
paginated range queries #135
Conversation
cfe66b1
to
af7bf11
Compare
pkg/bcdb/data_query.go
Outdated
if i.currentLoc == len(i.kvs) && i.pendingResult && !i.limitReached { | ||
kvs, pending, next, err := i.q.getDataByRange(i.dbName, i.nextStartKey, i.endKey, i.limit) | ||
if err != nil { | ||
return nil, false, err | ||
} | ||
i.kvs = kvs | ||
i.pendingResult = pending | ||
i.nextStartKey = next | ||
i.currentLoc = 0 | ||
} |
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 fetches the next batch before returning the last item of the last batch. I would change that and return the last item, changing the condition in line 95 to recognize that there is another batch to bring. So the Next() that is supposed to bring the first element in the next batch would be the one to get 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.
Done
for i := 0; i < 13; i++ { | ||
putKeySync(t, "testDB", fmt.Sprintf("key%d", i), fmt.Sprintf("value%d", i), "alice", userSession) | ||
} |
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.
How many keys do you need to go over the pagination limit (50B), and is there a way to introspect, maybe looking at the logs of some of there tests below, and verify that pendingResult
and nextKey
are what you expect?
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.
Currently, I use the code-coverage and approximately know that 2 to 3 records results in > 50B. Yes, it is good to add logger and capture the flow. I will do it in a subsequent PR. #137
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.
Fine by me
This commit adds a range query API to the data query executor.1 Signed-off-by: senthil <[email protected]>
No description provided.