[infinispan-dev] QueryIterator inconsistencies

[Sanne Grinovero]

Hi Marko,
great observations. More comments inline:

On 24 September 2012 13:30, Marko Lukša <marko.luksa at gmail.com> wrote:
>
> Hey
>
> Infinispan's implementations of QueryIterator don't conform to the
> contract of ListIterator and are pretty buggy (see
> https://issues.jboss.org/browse/ISPN-2337). The most important bug is
> the fact that calling next() and then previous() should return the same
> element, but it doesn't.

+1, that's wrong, fix is very welcome.
Funny, one of my first contributions to Hibernate Search was rewriting
the Scrollable implementation to fix a similar problem.

> I'm trying to fix the bugs, but have now realized that even the
> interface QueryIterator is problematic.
>
> 1.  QueryIterator.first()
> This one isn't as problematic as the next ones, but it's strange that
> calling first() and then next() returns the first element of the
> collection. It would be clearer if the method were called
> "beforeFirst()" or something similar.

+1 for "beforeFirst()"
(Hibernate's Scrollable uses the same name)

>
> 2. QueryIterator.last()
> What should the iterator return when you call last() and then call
> next()? What about if you call last() and then previous()? A user would
> probably call last() as the first step of iterating over the results
> backwards. So calling previous() should return the last element. The
> method should probably be renamed to "afterLast()".

+1 for the rename


> 3. QueryIterator.afterFirst() & beforeLast()
> These two are OK. Calling next() after afterFirst() should return the
> second element, while calling previous() should return the first element.

Are you sure they are ok? I'm actually wondering why they exist at
all; they don't look like very useful to me.
Why should someone want a method to directly jump to the second element?
Users already have the option to "beforeFirst && next" or use the
"jumpTo" / seek operation.


> 4. isFirst(), isLast()
> ListIterator does not have the concept of "current element", but the
> javadoc of all the is*() methods talk about the current element, which
> is ambiguous. Therefore isFirst() and isLast() should probably be
> renamed to isBeforeFirst() and isAfterLast().

I would remove these too. Am I missing some use case?


> 5. jumpToResult(int index)
> When jumping to index=1, what does next() return? What about previous()?
> We should probably replace the method with two others: beforeResult(int
> index) and afterResult(int index)

I would add only "beforeResult(int)", again I don't see why we should
have both versions.
Since you have to do "next()" to retrieve a value, I'd consider
"beforeResults" more user friendly than "afterResults"

>
> Any comments / other ideas?
>
> Marko

That's a lot of changes needed. One would say we could deprecate all
methods and rewrite the new ones, but wouldn't it be better to
deprecate the interface and introduce a brand new one?
What about "ResultsIterator" ? after all, we're not iterating on queries..

Sanne