Add empty and stacked indexes #346
Conversation
|
Draft for now, because of the 3.2.0 target. Other than that, this should be ready. |
Ladicek
force-pushed
the
empty-stacked-index
branch
from
e40e07f to
f0ee2bd
Compare
| * | ||
| * @since 3.2.0 | ||
| */ | ||
| public final class EmptyIndex implements IndexView { |
There was a problem hiding this comment.
Ehm, what is this for? I mean it's not even used internally, is it? And what's the benefit of this optimized empty version? Right now, there are other ways to obtain an empty index such as Index.of(Collections.emptyList()). Shouldn't we use the EmptyIndex in similar scenarios? Maybe we should add something like Index.empty()?
There was a problem hiding this comment.
So there's a difference between empty Index (which is fairly heavy-weight) and empty IndexView (which is super light-weight).
I don't have a use case for it myself, but apparently Hibernate Search people do: https://github.com/hibernate/hibernate-search/blob/main/util/common/src/main/java/org/hibernate/search/util/common/jar/impl/JandexUtils.java#L51 And it feels right to have a dedicated implementation directly in Jandex.
There was a problem hiding this comment.
If so, then we should probably promote the API a bit more, i.e. mention/link it in the IndexView, maybe even add IndexView.empty() etc.
There was a problem hiding this comment.
I did contemplate about a public static IndexView empty() method on IndexView vs. a public class EmptyIndex and didn't feel strongly about either. I can switch to the static method for sure.
There was a problem hiding this comment.
Added IndexView.empty().
There was a problem hiding this comment.
I think that it's ok to have both.
There was a problem hiding this comment.
Yeah, agree, I didn't know why I didn't make EmptyIndex.INSTANCE public. Fixed.
| } | ||
|
|
||
| /** | ||
| * Creates a stacked index from given {@code indexes}. The first element of the list is the bottom-most index |
There was a problem hiding this comment.
This means that when you build the stack you need to add the indexes with "lower priority" first which is IMO not exactly intuitive and practical. I wonder if we should offer some more flexible way of building the stack.
There was a problem hiding this comment.
I actually had a push(IndexView) method implemented (that would return a new stacked index with the given argument on top of the stack), but I felt like a solution in search of a problem, so I ultimately removed it. It should be easy to re-add.
There was a problem hiding this comment.
Added StackedIndex.pushIndex().
Ladicek
force-pushed
the
empty-stacked-index
branch
2 times, most recently
from
9244e6e to
f21ce6b
Compare
Ladicek
force-pushed
the
empty-stacked-index
branch
from
f21ce6b to
d1701ea
Compare
A stacked index is a composition of multiple indices with overlay semantics. Overlaying is done on class granularity. When the composition doesn't contain duplicate classes (multiple classes with the same name), a `StackedIndex` behaves just like a `CompositeIndex`, but when duplicates appear, the behavior of `StackedIndex` is actually well defined. This commit also adds a test for index navigation methods from `IndexView`, as those have not been tested at all. Some tests reveal inconsistencies in how `CompositeIndex` behaves in presence of duplicates, which is not nice, but this commit intentionally doesn't modify `CompositeIndex` behavior.
Ladicek
force-pushed
the
empty-stacked-index
branch
from
d1701ea to
772a528
Compare
Fixes #142