Simplify LikeFilter implementation of getBitmapIndex, estimateSelectivity.#3910
Simplify LikeFilter implementation of getBitmapIndex, estimateSelectivity.#3910leventov merged 4 commits intoapache:masterfrom
Conversation
…vity. LikeFilter: - Reduce code duplication, and simplify methods, at the cost of incurring an extra box of ImmutableBitmap into a SingletonImmutableList. I think this is fine, since this should be cheap and the code path is not hot (just once per filter). Filters: - Make estimateSelectivity public since it seems intended that they be used by Filter implementations, and Filters from extensions may want to use them too. Removed @VisibleForTesting for the same reason. - Rename one of the estimatePredicateSelectivity overloads to estimateSelectivity, since predicates aren't involved.
|
👍 |
| * @param bitmaps the bitmaps | ||
| * | ||
| * @return unioned bitmap | ||
| */ |
There was a problem hiding this comment.
This logic should just be part of every BitmapFactory.union() implementation. No ambiguity, impossible to choose "wrong" method and miss some optimization.
There was a problem hiding this comment.
Sure, will move it in.
| } else { | ||
| // fallback | ||
| return ImmutableList.of( | ||
| Filters.matchPredicate( |
There was a problem hiding this comment.
When getBitmapIterable() is called from estimateSelectivity(), this fallback is nonsense, because the whole point of estimateSelectivity() is to avoid materializing this bitmap. It could be fixed by adding a method like Iterable<ImmutableBitmap> Filters.predicateMatchingBitmaps().
There was a problem hiding this comment.
Haha, good point. Will fix.
|
thx @leventov, pushed an update. |
| public ImmutableBitmap union(Iterable<ImmutableBitmap> b) | ||
| throws ClassCastException | ||
| { | ||
| if (b instanceof List) { |
| @Override | ||
| public ImmutableBitmap union(Iterable<ImmutableBitmap> b) | ||
| { | ||
| if (b instanceof List) { |
There was a problem hiding this comment.
Changed list to collection.
|
I'll look at this patch soon. |
|
LGTM |
4 participants
Follow up to #3848. I think the simplification is worth the extra box which should be cheap.
cc @jihoonson / @leventov -- what do you think? Similar changes could be made to BoundFilter and InFilter if this one looks good.
LikeFilter:
of ImmutableBitmap into a SingletonImmutableList. I think this is fine, since this
should be cheap and the code path is not hot (just once per filter).
Filters:
implementations, and Filters from extensions may want to use them too. Removed
@VisibleForTestingfor the same reason.predicates aren't involved.