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.

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

Supports scrolling base on keyset without id #3013

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

quaff
Copy link
Contributor

@quaff quaff commented Jun 12, 2023

id shouldn't be added to sort if provided sort contains unique property.
See GH-2996

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jun 12, 2023
@mp911de mp911de added the status: on-hold We cannot start working on this issue yet label Jun 12, 2023
@mp911de
Copy link
Member

mp911de commented Jun 12, 2023

Removing the primary key isn't going to work, as Keyset scrolling must enforce uniqueness. Just accepting Sort isn't sufficient. What we could do is verify whether a unique property is a part of Sort. If not, we fall back to the primary key.

@quaff
Copy link
Contributor Author

quaff commented Jun 12, 2023

Removing the primary key isn't going to work, as Keyset scrolling must enforce uniqueness. Just accepting Sort isn't sufficient. What we could do is verify whether a unique property is a part of Sort. If not, we fall back to the primary key.

IMO, we should add a section in document to elaborate advantage and limitation, guide developers to make best decision, avoid making code more complex.

@mp911de
Copy link
Member

mp911de commented Jun 12, 2023

I agree that we should improve the documentation in Commons to explain the relationship of Sort to the query and how we amend the Sort object.

@quaff
Copy link
Contributor Author

quaff commented Jun 12, 2023

I mean it is the responsibility of developer to make sure keys combination unique if the document emphasize that.

@quaff quaff marked this pull request as draft June 13, 2023 01:34
@quaff quaff marked this pull request as ready for review June 13, 2023 05:08
@quaff
Copy link
Contributor Author

quaff commented Jun 13, 2023

Test improved to cover such cases:

  1. unsorted (implicit sorted by id asc)
  2. explicit sorted by id asc and desc
  3. explicit sorted by seqNo asc and desc
  4. explicit sorted by (id, seqNo) asc and desc

all cases are tested against FORWARD and BACKWARD

@mp911de
Copy link
Member

mp911de commented Jun 13, 2023

Please, let's keep pull requests focused instead of mixing concerns within a single pull request.

@quaff
Copy link
Contributor Author

quaff commented Jun 13, 2023

Please, let's keep pull requests focused instead of mixing concerns within a single pull request.

Sorry to disturb you, those changes are related and necessary to make the test pass.

@mp911de
Copy link
Member

mp911de commented Jun 13, 2023

We don't work towards making tests pass that combine various aspects but rather include coordinated changes. I applied the directional retention via #2999 already. Including unique sort arguments and checking for these requires a bit more changes than we have in this PR.

Towards the sort requirements, we want the calling code to specify sort properties without the requirement to include always properties to make rows unique. Making rows unique is primarily a concern of our keyset scrolling code. If calling code wants to specify sort properties to make the result unique, then we can do so in the course of this pull request.

@mp911de mp911de added type: enhancement A general enhancement and removed status: on-hold We cannot start working on this issue yet status: waiting-for-triage An issue we've not yet triaged labels Jun 13, 2023
@quaff quaff force-pushed the patch-16 branch 3 times, most recently from 071f270 to 1324b29 Compare June 16, 2023 01:56
@quaff
Copy link
Contributor Author

quaff commented Jun 16, 2023

KeysetScrollIntegrationTests is extracted to #3030 after #2999 resolved.

@quaff
Copy link
Contributor Author

quaff commented Jun 18, 2023

A viable proposal is adding a boolean property (eg. keysetQualified) to Sort, let developer hint that based on unique columns.

@quaff
Copy link
Contributor Author

quaff commented Jul 3, 2023

We could introduce a global property indicate that sort is unique ensured by developer.

@mp911de mp911de self-assigned this Jul 19, 2023
@quaff quaff marked this pull request as draft October 24, 2023 03:48
@quaff quaff marked this pull request as ready for review October 24, 2023 05:01
@quaff
Copy link
Contributor Author

quaff commented Oct 24, 2023

Commit updated, I introduce method JpaEntityInformation.isKeysetQualified() and default implementation is check whether unique property present, then skip include id in keyset, please take a look @mp911de

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants