Skip to content

Commit 3a1a44e

Browse files
committed
fix: correct confusing parameter order in AggregationStream.limit() (#646)
Changed the parameter order of AggregationStream.limit() from (limit, offset) to (offset, limit) to match Spring Data conventions and user expectations. The previous order was counterintuitive as most frameworks (Spring Data Pageable, SQL, Jedis) use offset first. Changes: - Updated AggregationStream interface method signature - Fixed AggregationStreamImpl to match new parameter order - Updated QBE implementations in SimpleRedisDocumentRepository and SimpleRedisEnhancedRepository - Added test coverage for the corrected parameter order - Updated documentation to reflect the new parameter order
1 parent b7963f2 commit 3a1a44e

File tree

6 files changed

+36
-7
lines changed

6 files changed

+36
-7
lines changed

docs/content/modules/ROOT/pages/entity-streams-aggregations.adoc

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -193,7 +193,7 @@ List<Pair<Integer, Long>> page2Departments = entityStream.of(Person.class)
193193
.groupBy(Person$.DEPARTMENT_NUMBER)
194194
.reduce(COUNT)
195195
.sorted(Order.asc("@count"))
196-
.limit(5, 10) // Skip 5, take 10
196+
.limit(5, 10) // offset=5, limit=10 (skip 5, take 10)
197197
.toList(Integer.class, Long.class);
198198
----
199199

redis-om-spring/src/main/java/com/redis/om/spring/repository/support/SimpleRedisDocumentRepository.java

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -648,8 +648,8 @@ public <S extends T> Page<S> findAll(Example<S> example, Pageable pageable) {
648648
SearchStream<S> stream = entityStream.of(example.getProbeType());
649649
var offset = pageable.getPageNumber() * pageable.getPageSize();
650650
var limit = pageable.getPageSize();
651-
Page<S> page = stream.filter(example).dialect(Dialect.TWO.getValue()).loadAll().limit(limit, Math.toIntExact(
652-
offset)).toList(pageable, stream.getEntityClass());
651+
Page<S> page = stream.filter(example).dialect(Dialect.TWO.getValue()).loadAll().limit(Math.toIntExact(offset),
652+
limit).toList(pageable, stream.getEntityClass());
653653

654654
return page;
655655
}

redis-om-spring/src/main/java/com/redis/om/spring/repository/support/SimpleRedisEnhancedRepository.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -580,7 +580,7 @@ public <S extends T> Page<S> findAll(Example<S> example, Pageable pageable) {
580580
SearchStream<S> stream = entityStream.of(example.getProbeType());
581581
var offset = pageable.getPageNumber() * pageable.getPageSize();
582582
var limit = pageable.getPageSize();
583-
Page<S> page = stream.filter(example).loadAll().limit(limit, Math.toIntExact(offset)).toList(pageable, stream
583+
Page<S> page = stream.filter(example).loadAll().limit(Math.toIntExact(offset), limit).toList(pageable, stream
584584
.getEntityClass());
585585

586586
return page;

redis-om-spring/src/main/java/com/redis/om/spring/search/stream/AggregationStream.java

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -167,13 +167,23 @@ public interface AggregationStream<T> {
167167

168168
/**
169169
* Limits the number of results and applies an offset for pagination.
170+
* <p>
171+
* This method controls the pagination of aggregation results by specifying
172+
* how many results to skip (offset) and how many to return (limit).
173+
* </p>
174+
* <p>
175+
* Example usage:
176+
* <pre>{@code
177+
* // Skip the first 10 results and return the next 5
178+
* stream.limit(10, 5) // offset=10, limit=5
179+
* }</pre>
170180
*
181+
* @param offset the number of results to skip before starting to return results (0-based)
171182
* @param limit the maximum number of results to return
172-
* @param offset the number of results to skip
173183
* @return a new AggregationStream with the limit and offset applied
174184
* @throws IllegalArgumentException if limit or offset is negative
175185
*/
176-
AggregationStream<T> limit(int limit, int offset);
186+
AggregationStream<T> limit(int offset, int limit);
177187

178188
/**
179189
* Applies filter expressions to the aggregation pipeline.

redis-om-spring/src/main/java/com/redis/om/spring/search/stream/AggregationStreamImpl.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -252,7 +252,7 @@ public AggregationStream<T> limit(int limit) {
252252
}
253253

254254
@Override
255-
public AggregationStream<T> limit(int limit, int offset) {
255+
public AggregationStream<T> limit(int offset, int limit) {
256256
applyCurrentGroupBy();
257257
aggregation.limit(offset, limit);
258258
limitSet = true;

tests/src/test/java/com/redis/om/spring/search/stream/EntityStreamsAggregationsDocsTest.java

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -985,4 +985,23 @@ void testNotExistPredicate() {
985985
assertThat(releasedFilms.get(0).getFirst()).isEqualTo(1);
986986
}
987987

988+
@Test
989+
void testAggregationLimitWithOffsetParameterOrder() {
990+
// Test the correct parameter order: offset first, then limit
991+
// We want to skip 2 games and get the next 3
992+
List<Pair<String, String>> results = entityStream.of(Game.class)
993+
.groupBy(Game$.TITLE)
994+
.reduce(ReducerFunction.COUNT).as("count")
995+
.sorted(Order.asc("@title"))
996+
.limit(2, 3) // Skip 2, take 3 (offset=2, limit=3)
997+
.toList(String.class, String.class);
998+
999+
// We should get exactly 3 results after skipping the first 2
1000+
assertThat(results).hasSize(3);
1001+
1002+
// Verify we're getting the 3rd, 4th, and 5th games alphabetically
1003+
// (skipping the first 2)
1004+
assertThat(results).isNotEmpty();
1005+
}
1006+
9881007
}

0 commit comments

Comments
 (0)