Skip to content

bug: correct confusing parameter order in AggregationStream.limit() #646

@SylvainAssemat

Description

@SylvainAssemat

Hi Redis Om Spring Team

It's me again :-)

Just find a little issue on pagination (in aggregation query) that take me some time to understand :

The documentation below say : // Skip 5, take 10

// Pagination with offset and limit
List<Pair<Integer, Long>> page2Departments = entityStream.of(Person.class)
    .groupBy(Person$.DEPARTMENT_NUMBER)
    .reduce(COUNT)
    .sorted(Order.asc("@count"))
    .limit(5, 10)                                      // Skip 5, take 10
    .toList(Integer.class, Long.class);

Well not really...

In fact the method is inverted :

https://github.com/redis/redis-om-spring/blob/v1.0.0/redis-om-spring/src/main/java/com/redis/om/spring/search/stream/AggregationStreamImpl.java

  @Override
  public AggregationStream<T> limit(int limit, int offset) {
    applyCurrentGroupBy();
    aggregation.limit(offset, limit);
    limitSet = true;
    return this;
  }

So in a simple unit test by my side :

            List<String> listIds = entityStream
                    .of(RDContact.class)
                    .groupBy(RDContact$.ID)
                    .sorted(RDContact$.ID.asc())
                    .limit(15000, 10000)
                    .toList(String.class);

            log.info("COUNT SIZE : " + listIds.size());

COUNT SIZE : 15000

So it take 15000 and skip 10000

Maybe the naming could be improved in the interface AggregationStream from :
AggregationStream<T> limit(int var1, int var2);
to
AggregationStream<T> limit(int limit, int offset);

What do you think ?

Thanks

Metadata

Metadata

Assignees

Labels

bugSomething isn't working

Type

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions