Skip to content

Don't union with empty bitmap in the end in Filters.matchPredicate()#3754

Merged
jon-wei merged 1 commit intoapache:masterfrom
metamx:filters-matchPredicate-bug-fix
Dec 7, 2016
Merged

Don't union with empty bitmap in the end in Filters.matchPredicate()#3754
jon-wei merged 1 commit intoapache:masterfrom
metamx:filters-matchPredicate-bug-fix

Conversation

@leventov
Copy link
Copy Markdown
Member

@leventov leventov commented Dec 7, 2016

Problem
I've inspected the graph of bitmap manipulations which are done before processing the query on Druid historical nodes and seen something like this:

union {
  intersection {
    union {dimValue=14, empty=1}=1,
    union {dimValue=3, empty=1}=1
  }=1,
  union {dimValue=7, empty=1}=1
}

(BTW if you want such intronspection capabilities to be commited to Druid sooner please review this: #3693)

It means that we often make unions of something + empty bitmap.

How it is fixed
Optimized Iterator<ImmutableBitmap> implementation inside Filters.matchPredicate() so that it doesn't emit empty bitmap in the end of the iteration, and make it to follow Iterator contract, that is throw NoSuchElementException from next() if there are no more bitmaps.

BTW disadvantage of Druid's "anonymous class style" is that it's impossible to add tests to such anonymous interface implementations.

…hPredicate() so that it doesn't emit empty bitmap in the end of the iteration, and make it to follow Iterator contract, that is throw NoSuchElementException from next() if there are no more bitmaps
@gianm
Copy link
Copy Markdown
Contributor

gianm commented Dec 7, 2016

@leventov have you measured the improvement here?

@gianm gianm added this to the 0.9.3 milestone Dec 7, 2016
@gianm gianm assigned fjy and gianm Dec 7, 2016
Copy link
Copy Markdown
Contributor

@gianm gianm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 LGTM

@leventov
Copy link
Copy Markdown
Member Author

leventov commented Dec 7, 2016

@gianm no, I didn't. I don't think improvement is significant, because empty bitmaps in compressed representations (concise and roaring) doesn't take much space and cheap to union with. However for roaring it saves one allocation/merge cycle because roaring doesn't support union (and intersection) of more than 2 bitmaps, for making union of many bitmaps it merges them one-by-one.

@leventov
Copy link
Copy Markdown
Member Author

leventov commented Dec 7, 2016

@gianm thank you very much for quick reviews!

@jon-wei
Copy link
Copy Markdown
Contributor

jon-wei commented Dec 7, 2016

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants