[infinispan-dev] QueryIterator inconsistencies

[Marko Lukša]

On 24.9.2012 19:03, Sanne Grinovero wrote:
>> 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.
>
Oh yes, I was wondering about that myself when I first saw 
QueryIterator, but then figured someone needed/wanted it. If we 
deprecate the whole QueryIterator and introduce a new interface, I'd 
leave these methods out.

>> 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?
>
I think you're right. Also, instead of calling isBeforeFirst() and 
isAfterLast(), users can simply call !hasPrevious() and !hasNext(), 
which already exist.

>> 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"
Hmm, what if you want to iterate backwards. If you only had 
beforeResult(int), you wouldn't be able to jump behind the last element. 
You'd have to use afterLast(). For usecases where you always jump to the 
end and iterate backwards, afterLast() would be ok. But what if you 
needed to implement a method like iterateBackwards(int fromIndex, 
Visitor visitor). Then the method would need to have something like:  if 
(fromIndex == col.size()) it.afterLast() else it.beforeResult(fromIndex).

Of course, we could allow users to call beforeResult(col.size()), but 
that's not completely clean, since there's no element with index 
col.size(). But still, it may be cleaner than having an additional 
method (afterResult).


>> 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..

I agree. The only thing that bothers me is that CacheQuery would then 
need new methods called (lazy)resultIterator() (instead of iterator(), 
which already exists).

This reminds me. What is the difference between LazyIterator and 
EagerIterator? Both of them execute the Lucene query immediately. The 
only difference is that EagerIterator loads EntityInfos straight away, 
while LazyIterator (only) loads the DocumentExtractor and then at first 
invocation of next(), it loads the EntityInfo and cache value at the 
same time (when filling the buffer).

IMO, the difference between them is negligible and we could either scrap 
LazyResultIterator or make EagerResultIterator preload everything?


Marko