Add support for keyset- and offset scrolling #2885

mp911de · 2023-03-23T14:47:59Z

We now support offset- and keyset-scrolling through query derivation, Query-by-Example and Querydsl.

interface UserRepository extends Repository<User, Long> {
  Window<User> findTop3ByFirstnameStartingWithOrderByFirstnameAscEmailAddressAsc(String firstname,
			ScrollPosition position);
}

Window<User> firstWindow = repository.findTop3ByFirstnameStartingWithOrderByFirstnameAscEmailAddressAsc("J",
				KeysetScrollPosition.initial());

String-based query methods are not supported yet because we do not rewrite the query. For HQL/JPQL, we might introduce this functionality later (see #2886). We do not intend to support keyset scrolling for stored procedures as we have no metadata over the stored procedure and its API.

schauder

Didn't make it through all the changes.

Some of my comments are just documentation of my confusion. I'd need more time to clarify if those are due to my limited brain power or a problem in the code.
But I don't know when I will find time for that so, it is what it is.

schauder · 2023-03-23T15:13:03Z

...pa/src/test/java/org/springframework/data/jpa/repository/query/CollectionUtilsUnitTests.java

+	@Test // GH-2878
+	void shouldReturnFirstItems() {
+
+		assertThat(CollectionUtils.getFirst(2, List.of(1, 2, 3))).hasSize(2).containsSequence(1, 2);


Is there a reason to use .hasSize(2).containsSequence(1, 2) instead of .containsExactly(1,2)?
Equivalent for all other asserts in this class.

...src/main/java/org/springframework/data/jpa/repository/query/JpaKeysetScrollQueryCreator.java

schauder · 2023-03-23T15:26:56Z

...ta-jpa/src/main/java/org/springframework/data/jpa/repository/query/KeysetScrollDelegate.java

+		for (Order order : sort) {
+
+			if (!keysetValues.containsKey(order.getProperty())) {
+				throw new IllegalStateException("KeysetScrollPosition does not contain all keyset values");


It would be nice to have the missing property and the actual keyset in the error message.

schauder · 2023-03-23T15:37:49Z

...ta-jpa/src/main/java/org/springframework/data/jpa/repository/query/KeysetScrollDelegate.java

+		List<P> or = new ArrayList<>();
+
+		int i = 0;
+		// progressive query building


What the heck are we doing here? Honest question, I don't get it.

It looks like
if we order by A, B, C and have keyset values A-> a, B-> b, C-> c, D->d
we create the constraint
(A > a and B>b and C>c)
or (B>b and C>c)
or (C>c)

which should be equivalent to (C>c) and be completely useless 🤔

it is: A > a || A == a && B > b || A == a && B == b && C > c assuming ORDER BY A asc, B asc, C asc

Can we have something to that effect in a comment in the code?

...ta-jpa/src/main/java/org/springframework/data/jpa/repository/query/KeysetScrollDelegate.java

schauder · 2023-03-23T15:55:57Z

spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/query/ScrollDelegate.java

+ * @author Mark Paluch
+ * @since 3.1
+ */
+public class ScrollDelegate<T> {


I think I have a problem with the name part Delegate. A delegate is something that takes care of part of a task. In that sense pretty much every class is a delegate. It makes sense in a certain role, i.e. as a field name where it becomes a hint that the Delegate pattern is used. But as part of a class name it conveys exactly zero information to me, maybe a little less.

I agree with Jens that this doesn't appear to be an implementation of the Delegate pattern so may need a more suitable name. I'd hate to say ScrollUtils as those tend to be collections of procedural static functions.

I've followed Spring Framework practices (see https://github.com/spring-projects/spring-framework/blob/18adf905a8eb33bfaaaa42d4d6d078027f8090ea/spring-context/src/main/java/org/springframework/jndi/JndiLocatorDelegate.java#L32) to create an object that implements a certain behavior that can be used from various places.

While most of the stuff that the Spring (Framework) team does is pretty awesome, in this case I'm not a fan.

I'd hate to say ScrollUtils as those tend to be collections of procedural static functions.

Or maybe just Scroller?

We have the same delegation pattern in e.g. RepositoryConfigurationDelegate where we encapsulate a subset of functionality in an object that is being used as delegate from other places.

gregturn · 2023-03-23T17:58:49Z

...ta-jpa/src/main/java/org/springframework/data/jpa/repository/query/KeysetScrollDelegate.java

+
+	/**
+	 * Adapter to construct scroll queries.
+	 *


Is this really an "adapter" pattern? Adapters typically implement implement interfaces with empty methods.

gregturn · 2023-03-23T17:59:40Z

...a/src/main/java/org/springframework/data/jpa/repository/query/KeysetScrollSpecification.java

@@ -63,7 +60,7 @@ public KeysetScrollSpecification(KeysetScrollPosition position, Sort sort, JpaEn
 	 */
 	public static Sort createSort(KeysetScrollPosition position, Sort sort, JpaEntityInformation<?, ?> entity) {

-		KeysetScrollDirector director = KeysetScrollDirector.of(position.getDirection());
+		KeysetScrollDelegate director = KeysetScrollDelegate.of(position.getDirection());



If KeysetScrollDirector is no more, then the variable name probably needs to change as well.

gregturn · 2023-03-23T18:00:43Z

spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/query/ScrollDelegate.java

@@ -80,7 +79,7 @@ public Window<T> scroll(Query query, Sort sort, ScrollPosition scrollPosition) {
 	private static <T> Window<T> createWindow(Sort sort, int limit, Direction direction,
 			JpaEntityInformation<T, ?> entity, List<T> result) {

-		KeysetScrollDirector director = KeysetScrollDirector.of(direction);
+		KeysetScrollDelegate director = KeysetScrollDelegate.of(direction);
 		List<T> resultsToUse = director.postProcessResults(result);


See prior comment about var name.

gregturn · 2023-03-23T18:03:17Z

.../main/java/org/springframework/data/jpa/repository/support/QuerydslJpaPredicateExecutor.java

+			if (scrollPosition instanceof KeysetScrollPosition keyset) {
+
+				KeysetScrollDelegate director = KeysetScrollDelegate.of(keyset.getDirection());
+				sort = KeysetScrollSpecification.createSort(keyset, sort, entityInformation);


See other comment about var name.

gregturn · 2023-03-23T18:08:10Z

spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/UserRepositoryTests.java

+				q -> q.limit(3).sortBy(Sort.by("firstname", "emailAddress")).scroll(backward));
+
+		assertThat(previousWindow).hasSize(2).containsSequence(jane1, jane2);
+


Do we need .containsExactlyInThisOrder to better assert the order of data in this Window? I'm presuming that position 3 was john1, and you went backward to jane2 and then jane1, which I'd expect to see as .containsExactlyInThisOrder(jane2, jane1) as a reflection of this backward traversal.

Quite possibly, we should assert that way for the other Windows as well, to further highlight the ordering of data and traversal.

gregturn · 2023-03-23T18:10:09Z

...ava/org/springframework/data/jpa/repository/support/FetchableFluentQueryBySpecification.java

+
+	@Override
+	public FetchableFluentQuery<R> limit(int limit) {
+		Assert.isTrue(limit >= 0, "Limit must not be negative");


schauder

Some more comments. Nothing serious.

schauder · 2023-03-28T08:45:17Z

spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/query/ScrollDelegate.java

+ * @author Mark Paluch
+ * @since 3.1
+ */
+public class ScrollDelegate<T> {


While most of the stuff that the Spring (Framework) team does is pretty awesome, in this case I'm not a fan.

I'd hate to say ScrollUtils as those tend to be collections of procedural static functions.

Or maybe just Scroller?

schauder · 2023-03-28T08:58:10Z

src/main/asciidoc/jpa.adoc

 <3> Valid `Sort` containing explicitly _unsafe_ `Order`.
 <4> Valid `Sort` expression pointing to aliased function.
 ====

+[[jpa.query-methods.scroll]]
+=== Scrolling large Query Results


Large with capital L

schauder · 2023-03-28T10:52:22Z

src/main/asciidoc/jpa.adoc

+[[jpa.query-methods.scroll]]
+=== Scrolling large Query Results
+
+When working with large data sets, <<repositories.scrolling,scrolling>> can help to process those results efficiently without loading all results into memory.


I find this section is written rather abstract and would prefer something more concrete.
Maybe something along the lines of:

When working with large data sets you cannot or don't want to load all result into memory at once. Spring Data JPA offers keyset-based and offset-based scrolling.

For more information about paging see paging
Offset-based scrolling becomes inefficient when the offset is large because the database still has to load the data, sort it just to count of the elements ignored due to the offset. See link for more information about this.

Keyset-based scrolling is based on returning a Window<T>
which apart from the actual result offers ways to access the next and previous window.

add example

While keyset-based scrolling is more efficient it has it's limitations as well.
Especially there is no concept of a numbered page, only of a next and previous window.

I extracted several impulses into spring-projects/spring-data-commons#2804 so that we can include less store-specific details here and can link to the actual elaboration on our scrolling methods.

schauder · 2023-03-28T10:55:31Z

...ng-data-jpa/src/main/java/org/springframework/data/jpa/repository/query/CollectionUtils.java

+	/**
+	 * Return the first {@code count} items from the list.
+	 *
+	 * @param count


If we don't provide a description for @param and @return we should skip them

schauder · 2023-03-28T10:55:41Z

...ng-data-jpa/src/main/java/org/springframework/data/jpa/repository/query/CollectionUtils.java

+	/**
+	 * Return the last {@code count} items from the list.
+	 *
+	 * @param count


schauder · 2023-03-28T11:08:19Z

...a/src/main/java/org/springframework/data/jpa/repository/query/KeysetScrollSpecification.java

+	/**
+	 * Create a {@link Sort} object to be used with the actual query.
+	 *
+	 * @param position


add some parameter description

schauder · 2023-03-28T11:09:04Z

...a/src/main/java/org/springframework/data/jpa/repository/query/KeysetScrollSpecification.java

+		private final CriteriaBuilder cb;
+
+		public JpaQueryStrategy(From<?, ?> from, CriteriaBuilder cb) {
+			this.from = from;


schauder · 2023-03-28T11:09:49Z

...a/src/main/java/org/springframework/data/jpa/repository/query/KeysetScrollSpecification.java

+
+		@Override
+		public Predicate compare(Order order, Expression<Comparable> propertyExpression, Object o) {
+			return order.isAscending() ? cb.greaterThan(propertyExpression, (Comparable) o)


schauder · 2023-03-28T11:09:50Z

...a/src/main/java/org/springframework/data/jpa/repository/query/KeysetScrollSpecification.java

+
+		@Override
+		public Expression<Comparable> createExpression(String property) {
+			PropertyPath path = PropertyPath.from(property, from.getJavaType());


schauder · 2023-03-28T13:25:01Z

...main/java/org/springframework/data/jpa/repository/support/JpaMetamodelEntityInformation.java

+	@Override
+	public Map<String, Object> getKeyset(Iterable<String> propertyPaths, T entity) {
+
+		// TODO: proxy business?


If we can't resolve the TODO now we should leave clearer instructions what exactly the TODO is, and potentially create a ticket for it.

mp911de · 2023-03-29T09:38:08Z

Review comments addressed. I created a few tickets out of this review and incorporated documentation bits in Spring Data Commons so we need less store-specific documentation.

Closes #2878 Original pull request #2885 See spring-projects/spring-data-commons#2151

See #2878 Original pull request #2885

schauder · 2023-03-29T11:57:46Z

That's merged.

mp911de added 4 commits March 21, 2023 14:18

Prepare issue branch.

3430acd

Initial support for Keyset scrolling.

96f5766

Hacking on PartTreeJpaQuery

0af99dd

Add support for Querydsl.

41cb75d

spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Mar 23, 2023

mp911de linked an issue Mar 23, 2023 that may be closed by this pull request

Add support for Scroll API #2878

Closed

Polishing.

60f2427

schauder reviewed Mar 23, 2023

View reviewed changes

gregturn reviewed Mar 23, 2023

View reviewed changes

Add documentation.

053cbef

mp911de marked this pull request as ready for review March 24, 2023 08:40

mp911de added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Mar 24, 2023

mp911de mentioned this pull request Mar 24, 2023

Investigate keyset-scrolling using HQL/JPQL query methods #2886

Open

mp911de assigned schauder Mar 27, 2023

schauder reviewed Mar 28, 2023

View reviewed changes

schauder assigned mp911de and unassigned schauder Mar 28, 2023

mp911de mentioned this pull request Mar 29, 2023

Refine documentation of Paging, Sorting and Scrolling spring-projects/spring-data-commons#2804

Closed

Address review comments.

ba37131

mp911de force-pushed the issue/2878 branch from 3c9becc to ba37131 Compare March 29, 2023 09:32

mp911de mentioned this pull request Mar 29, 2023

Allow reversing Sort spring-projects/spring-data-commons#2805

Closed

mp911de assigned schauder and unassigned mp911de Mar 29, 2023

schauder pushed a commit that referenced this pull request Mar 29, 2023

Support for Keyset scrolling.

1b5dabb

Closes #2878 Original pull request #2885 See spring-projects/spring-data-commons#2151

schauder added a commit that referenced this pull request Mar 29, 2023

Polishing.

30511fe

See #2878 Original pull request #2885

schauder closed this Mar 29, 2023

schauder deleted the issue/2878 branch March 29, 2023 11:57

schauder added this to the 3.1 RC1 (2023.0.0) milestone Mar 29, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for keyset- and offset scrolling #2885

Add support for keyset- and offset scrolling #2885

mp911de commented Mar 23, 2023 •

edited

Loading

schauder left a comment

schauder Mar 23, 2023

schauder Mar 23, 2023

schauder Mar 23, 2023

mp911de Mar 24, 2023

schauder Mar 28, 2023

schauder Mar 23, 2023

gregturn Mar 23, 2023 •

edited

Loading

mp911de Mar 24, 2023

schauder Mar 28, 2023

mp911de Mar 29, 2023

gregturn Mar 23, 2023

gregturn Mar 23, 2023

gregturn Mar 23, 2023

gregturn Mar 23, 2023

gregturn Mar 23, 2023 •

edited

Loading

gregturn Mar 23, 2023

schauder left a comment

schauder Mar 28, 2023

schauder Mar 28, 2023

schauder Mar 28, 2023

mp911de Mar 29, 2023

schauder Mar 28, 2023

schauder Mar 28, 2023

schauder Mar 28, 2023

schauder Mar 28, 2023

schauder Mar 28, 2023

schauder Mar 28, 2023

schauder Mar 28, 2023

mp911de commented Mar 29, 2023

schauder commented Mar 29, 2023

		q -> q.limit(3).sortBy(Sort.by("firstname", "emailAddress")).scroll(backward));

		assertThat(previousWindow).hasSize(2).containsSequence(jane1, jane2);

Add support for keyset- and offset scrolling #2885

Add support for keyset- and offset scrolling #2885

Conversation

mp911de commented Mar 23, 2023 • edited Loading

schauder left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gregturn Mar 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gregturn Mar 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schauder left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mp911de commented Mar 29, 2023

schauder commented Mar 29, 2023

mp911de commented Mar 23, 2023 •

edited

Loading

gregturn Mar 23, 2023 •

edited

Loading

gregturn Mar 23, 2023 •

edited

Loading