segment metadata fallback analysis if no bitmaps#7116
Conversation
| max = NullHandling.nullToEmptyIfNeeded(bitmapIndex.getValue(cardinality - 1)); | ||
| } | ||
| } else if (capabilities.isDictionaryEncoded()) { | ||
| // fallback if no bitmap index |
There was a problem hiding this comment.
I dunno, maybe we should just remove the "size" analysisType. It is nothing but trouble.
There was a problem hiding this comment.
Agree. I think it would be interesting to have size be representative of "estimated size in memory" for incremental index, or size in bytes of column in the segment file that is mapped, but what it's computing here seems like basically nonsense for all column types.
There was a problem hiding this comment.
or size in bytes of column in the segment file that is mapped
dictionary size + bitmap size I vote for that too.
There was a problem hiding this comment.
Raised proposal #7124, which I will address in a follow up PR to not block this fix.
There was a problem hiding this comment.
Until #7124 is sorted, what you're doing here (setting size to 0 when there's no bitmap index) sounds good to me.
| { | ||
| QueryableIndex index = rollup ? TestIndex.getMMappedTestIndex() : TestIndex.getNoRollupMMappedTestIndex(); | ||
| QueryableIndex index = bitmaps | ||
| ? rollup |
There was a problem hiding this comment.
Nested ternaries = 😭. Please consider using normal ifs.
There was a problem hiding this comment.
I know, these tests were already painful to slip in a test for this since parameterized, will see if I can clean up. Maybe removing size will help.
| if (noBitmapRealtimeIndex != null) { | ||
| return noBitmapRealtimeIndex; | ||
| } | ||
| } |
There was a problem hiding this comment.
I know this synchronization pattern was before the change, I'm just confused what it tries to accomplish? ie, if noBitmapRealtimeIndex is not created then all threads can initialize noBitmapRealtimeIndex simultaneously, doesn't it look strange?
There was a problem hiding this comment.
Good point, I wasn't much attention, will fix.
There was a problem hiding this comment.
Updated to use Suppliers.memoize instead. I looked into it, and this code is quite old! It appears to be from the PR that added timeseries queries to Druid! Thanks for review 👍
gianm
left a comment
gianm
left a comment
There was a problem hiding this comment.
LGTM - had a couple minor comments but nothing blocking.
| String value = bitmapIndex.getValue(i); | ||
| if (value != null) { | ||
| size += StringUtils.estimatedBinaryLengthAsUTF8(value) * bitmapIndex.getBitmap(bitmapIndex.getIndex(value)).size(); | ||
| int cardinality = 0; |
There was a problem hiding this comment.
My aesthetic sensibility would be to have this be final and add a closer else { } that sets it to zero, or something. Minor comment though.
There was a problem hiding this comment.
Changed to do that, thanks for review 🤘
| max = NullHandling.nullToEmptyIfNeeded(bitmapIndex.getValue(cardinality - 1)); | ||
| } | ||
| } else if (capabilities.isDictionaryEncoded()) { | ||
| // fallback if no bitmap index |
There was a problem hiding this comment.
Until #7124 is sorted, what you're doing here (setting size to 0 when there's no bitmap index) sounds good to me.
* segment metadata fallback analysis if no bitmaps * remove accidental line * remove nonsense size estimation * less ternary * fix it * do the thing
5 participants
Instead of returning an "error" analysis, instead handle string dimensions without bitmaps by using facilities of
DictionaryEncodedColumn. Fixes #7114.