Re: [infinispan-dev] QueryIterator inconsistencies
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