Skip to content

added keyset pagination serialization #1007

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
Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

added keyset pagination serialization #1007

wants to merge 12 commits into from
+202 −65

Conversation

HayimShaul
Copy link

No description provided.

@HayimShaul
Copy link
Author

addressing issue #1002

adecaro
adecaro requested changes Jul 27, 2025
View reviewed changes
Copy link
Contributor

@adecaro adecaro left a comment

Choose a reason for hiding this comment

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

I left comments, please have a look.
In general, please, add comments to any public function added new together with a relevant unit-test.
Please, also fill the metadata fields on the right hand side of the PR web page. Thanks

@adecaro
Copy link
Contributor

adecaro commented Jul 27, 2025 •
edited by HayimShaul
Loading

for instance, we exercise pagination in platform/fabric/services/storage/vault/vaultstore_test.go by using GetAllTxStatuses. Why don't we try there the store and restore of a page?

Hayim: done

@adecaro adecaro added the db label Jul 28, 2025
@adecaro adecaro added this to the Q3 milestone Jul 28, 2025
@adecaro adecaro linked an issue Jul 28, 2025 that may be closed by this pull request
serialize keyset pagination #1002
Open
@HayimShaul HayimShaul requested review from adecaro and AkramBitar July 29, 2025 09:40
@HayimShaul HayimShaul force-pushed the hayim-jul25-keyset-saveload branch from e6e7bec to a880d59 Compare July 29, 2025 09:45
adecaro
adecaro requested changes Jul 29, 2025
View reviewed changes
alexandrosfilios
alexandrosfilios reviewed Jul 30, 2025
View reviewed changes
alexandrosfilios
alexandrosfilios reviewed Jul 30, 2025
View reviewed changes
platform/view/services/storage/driver/sql/query/pagination/keyset.go
if err != nil {
return nil, err
}
if strings.ToUpper(string(idFieldName[0])) != string(idFieldName[0]) {
Copy link
Contributor

Choose a reason for hiding this comment

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

does idfieldname need to be a slice?

Copy link
Author

Choose a reason for hiding this comment

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

Please explain

Copy link
Contributor

Choose a reason for hiding this comment

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

idFieldName is a string, why are we comparing the first character only?

Copy link
Author

Choose a reason for hiding this comment

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

good question. I copied the initialization code from KeysetWithField

Copy link
Author

Choose a reason for hiding this comment

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

@adecaro Not familiar enough with Go. I think the capital letter is needed to ensure the field is public.
Can we resolve this?

alexandrosfilios
alexandrosfilios reviewed Jul 30, 2025
View reviewed changes
platform/view/services/storage/driver/sql/query/pagination/offset.go
offset int
pageSize int
Offset int `json:"offset"`
PageSize int `json:"pageSize"`
Copy link
Contributor

Choose a reason for hiding this comment

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

some times you use camel case and some times snake case

Copy link
Author

Choose a reason for hiding this comment

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

@adecaro what is the style we use? I see both in the code

Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed, we need a linter to make sure we stick to one style.
Let's fix this in another PR.

alexandrosfilios
alexandrosfilios requested changes Jul 30, 2025
View reviewed changes
Copy link
Contributor

@alexandrosfilios alexandrosfilios left a comment

Choose a reason for hiding this comment

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

some changes to make

@HayimShaul HayimShaul force-pushed the hayim-jul25-keyset-saveload branch from 32bc96f to 9c85188 Compare July 31, 2025 12:33
@adecaro
Copy link
Contributor

adecaro commented Aug 4, 2025

@HayimShaul , I think we are essentially fine here. Let's propagate the changes to the other repos and then we merge.
Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adecaro adecaro adecaro requested changes

@alexandrosfilios alexandrosfilios alexandrosfilios requested changes

@AkramBitar AkramBitar Awaiting requested review from AkramBitar

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
db
Projects
None yet
Milestone
Q3
Development

Successfully merging this pull request may close these issues.

serialize keyset pagination
3 participants
@HayimShaul @adecaro @alexandrosfilios